Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

makefile improvements, enable dynamic library #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

i0ntempest
Copy link

No description provided.

@i0ntempest
Copy link
Author

A question - I had to disable -mavx2 when building on my system (arm64 macOS), are there similar optimization flags for arm64?

@IlyaGrebnov
Copy link
Owner

IlyaGrebnov commented Feb 4, 2025

A question - I had to disable -mavx2 when building on my system (arm64 macOS), are there similar optimization flags for arm64?

ARM Neon optimizations are enabled by default for Arm64 targets. I will look into PR later today. Thank you for contribution.

@IlyaGrebnov
Copy link
Owner

I tried a new Makefile and encountered the following error:
g_QlfcStatisticalModel1' cannot be used when creating a shared object; recompile with -fPIC.
Additionally, there are other issues:

  • Clang is not being used for compilation; instead, GCC is used.
    
  • OpenMP support is not enabled.
    

@i0ntempest
Copy link
Author

i0ntempest commented Feb 5, 2025

I tried a new Makefile and encountered the following error: g_QlfcStatisticalModel1' cannot be used when creating a shared object; recompile with -fPIC. Additionally, there are other issues:

Where is this g_QlfcStatisticalModel1? I might need to add -fPIC when compiling that file. I'm not very familiar with this flag tho, is it recommended to add it to all object files?

  • Clang is not being used for compilation; instead, GCC is used.
    
  • OpenMP support is not enabled.
    

I have added the ability to specify these as args. You want CC=clang CXX=clang++ or whatever your compiler filenames are, and OPENMP=1. I added these as they make it easier for automated builds (I'm using MacPorts).

@IlyaGrebnov
Copy link
Owner

Clang and OpenMP is needed for optimal performance, so it would be great if we can use it by default without need of additional make parameters. As of PIC (position independent code) I am not sure myself, but likely we just missing -fPIC somewhere as suggested by compiler.

@barracuda156
Copy link

@i0ntempest Please also drop hardcoding of libc++, that is unnecessary when that runtime is used and breaking when libstdc++ is used instead (which is the default with GCC outside of MacPorts, as well as the default on < 10.9).

@i0ntempest
Copy link
Author

@i0ntempest Please also drop hardcoding of libc++, that is unnecessary when that runtime is used and breaking when libstdc++ is used instead (which is the default with GCC outside of MacPorts, as well as the default on < 10.9).

@barracuda156 It is necessary on my end, when not using Xcode clang. Without it I get undefined symbols. Also it’s hardcoded in the portfile only.

@barracuda156
Copy link

Also it’s hardcoded in the portfile only.

Well that is easy to fix then, just do it as

if {${configure.cxx_stdlib} eq "libc++"} {
 . . .
}

So that this does not break GCC builds.


bsc: libbsc.a bsc.cpp
$(CC) $(CFLAGS) bsc.cpp -o bsc -L. -lbsc
$(CXX) $(CFLAGS) $(LDFLAGS) bsc.cpp -o $@ $<

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all places where CXX is used to compile C++ code, CXXFLAGS should be used, not CFLAGS.

install: libbsc.a bsc
libbsc.$(LIBEXT): $(OBJS)
rm -f $@
$(CC) $(CFLAGS) -fPIC -shared $(LDFLAGS) -o $@ $(OBJS)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Darwin you should use -dynamiclib instead of -shared, and you must use -install_name to set the absolute path where the library will be installed, and you should set -current_version and -compatibility_version to appropriate values.

If these objects are compiled from C++ code, you must use CXX to link, not CC, otherwise the C++ standard library will not be linked in.

Use CXXFLAGS when linking C++ code since that's where a user (or MacPorts, in our case) will put important flags like -stdlib=libc++ that are also needed at link time. (MacPorts can't know whether a port is linking objects built from C or C++ code so it does not put that flag in LDFLAGS).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one object that is compiled from C in this lib tho (see line 115), will switching to CXX and CXXFLAGS break anything?
Also do I just set those versions to the current project version?

Copy link

@ryandesign ryandesign Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using CXX to link C code works fine.

The only part of CXXFLAGS of relevance when linking, as far as I know, is the -stdlib flag. Since CXX accepts that flag it's fine. As far as I know, there's nothing in CFLAGS of interest at link time so there wouldn't be a need to use that.

Library versioning is a complicated topic about which you can find much written on the Internet, and specifically with regard to how macOS handles it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since CXX accepts that flag it's fine.

@ryandesign Not necessarily so, AFAIK. With gcc it is a non-default config option, which is not supported by default. (Yes, I know that MacPorts enables it on x86 and arm.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not relevant. If a user is using a C++ compiler that doesn't support a flag they obviously wouldn't add that flag to CXXFLAGS.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure.

Comment on lines +106 to +107
cp -f libbsc.$(LIBEXT) $(DESTDIR)$(PREFIX)/lib
chmod a+r $(DESTDIR)$(PREFIX)/lib/libbsc.$(LIBEXT)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically one installs files using install instead of cp and chmod.


bwt.o: libbsc/bwt/bwt.cpp
$(CC) $(CFLAGS) -c libbsc/bwt/bwt.cpp
$(CXX) $(CFLAGS) -c libbsc/bwt/bwt.cpp

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be an awful lot of code duplication here. Typically there is a single rule that arranges for all source code files (of one language) to be compiled into objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants