cufinufft static incompatible with finufft static about cufinufft

[janden]

Thanks for bringing this up. I'm guessing you're not the only one who has this problem.

I'm not familiar enough with how namespaces work in C++, but given that these functions are not part of the public API, are there other alternatives to hiding them? I'm asking because we'd like to keep the public API C-compatible, so introducing namespaces would break that.

[janden]

Another question (just out of curiosity), why static as opposed to dynamic linking here?

[DiamonDinoia]

I am using the libraries to create a mexfile for matlab. If I use shared libraries I have to ship the .so together with the mexfile. I prefer having a one file library instead with no attached so.

This problem arises because I compile with -fPIC since mexfiles are shared libraries.

[DiamonDinoia]

The problem would be solved if CNTime is moved to an internal namespace

#include <sys/time.h>
namespace cufinufft {
    namespace internal { // pick your own
    class CNTime {
     public:
      void start();
      double restart();
      double elapsedsec();
     private:
      struct timeval initial;
    };
}; // end internal
};  // end cufinufft

then you can call:

cufinufft::internal::CNTime time;

Other alternatives might be to move the internal functions in a separate static library and link (privately) against it.

[janden]

I am using the libraries to create a mexfile for matlab. If I use shared libraries I have to ship the .so together with the mexfile. I prefer having a one file library instead with no attached so.

Ok I see.

The problem would be solved if CNTime is moved to an internal namespace

Ok so the public interface would remain that same, got it.

Other alternatives might be to move the internal functions in a separate static library and link (privately) against it.

Ok interesting. So in this case the symbols would not be exposed by the main library in the same way?

[DiamonDinoia]

It depends on how is is linked against it.

cmake solves this problem nicely by using the PRIVATE keyword: https://cmake.org/cmake/help/latest/command/target_link_libraries.html

in makefiles you could specify the visibiliy -fvisibility=hidden and __attribute__((visibility("default")))) documentation: https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html

more details:
https://stackoverflow.com/questions/22102470/link-a-static-library-to-a-shared-library-and-hide-exported-symbols
https://stackoverflow.com/questions/435352/limiting-visibility-of-symbols-when-linking-shared-libraries/452955#452955

[DiamonDinoia]

As a good C++ practise, it would be good to wrap the API in a namespace and do something like:

#ifdef __cplusplus
extern "C" {
#endif

To mantain C retrocompatibility, e.g:
cufinufft.h =

#ifdef __cplusplus
#include "cufinufft_cpp.h"
#else
#include "cufinufft_c.h"
#endif

[janden]

Thanks. I'll look into this as soon as I have some spare time.

[ahbarnett]

Dear Marco (& Joakim), We had a meeting 2 days ago to discuss professionalizing certain such aspects, and I learned about -fvisibility=hidden. Eg https://labjack.com/news/simple-cpp-symbol-visibility-demo Would that solution work for now, or is it too tied to GNU compilers? I'm looping in @lu1and10 @wendazhou @blackwer to check. Then we would in FINUFFT make visible only the user-facing routines here https://finufft.readthedocs.io/en/latest/c.html https://finufft.readthedocs.io/en/latest/fortran.html Professionalizing cufinufft will inevitably come later. The problem with finufft:: namespace is that it is not compatible with C (one of our goals), right? Best, Alex

…

On Thu, Mar 31, 2022 at 10:59 AM Joakim Andén ***@***.***> wrote: Thanks. I'll look into this as soon as I have some spare time. — Reply to this email directly, view it on GitHub <#138 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACNZRSWOEUMBGXEXQWBIDHTVCW4UHANCNFSM5SFFERXA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- *---------------------------------------------------------------------~^`^~._.~' |\ Alex H. Barnett Center for Computational Mathematics, Flatiron Institute | \ http://users.flatironinstitute.org/~ahb 646-876-5942

[blackwer]

All, Here's my requested 2 cents :) The issue is already specifically with the GNU compilers (and presumably clang and the intel compilers). So I don't think there's an issue there. I should say that I think namespacing is a completely valid alternative though. You won't have issues with exceptions and unit tests, and it is automatically portable. You just namespace literally everything except your public API. Best, Robert

…

On Thu, Mar 31, 2022 at 09:42:37AM -0700, Alex Barnett wrote: Dear Marco (& Joakim), We had a meeting 2 days ago to discuss professionalizing certain such aspects, and I learned about -fvisibility=hidden. Eg https://labjack.com/news/simple-cpp-symbol-visibility-demo Would that solution work for now, or is it too tied to GNU compilers? I'm looping in @lu1and10 @wendazhou @blackwer to check. Then we would in FINUFFT make visible only the user-facing routines here https://finufft.readthedocs.io/en/latest/c.html https://finufft.readthedocs.io/en/latest/fortran.html Professionalizing cufinufft will inevitably come later. The problem with finufft:: namespace is that it is not compatible with C (one of our goals), right? Best, Alex On Thu, Mar 31, 2022 at 10:59 AM Joakim Andén ***@***.***> wrote: > Thanks. I'll look into this as soon as I have some spare time. > > — > Reply to this email directly, view it on GitHub > <#138 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACNZRSWOEUMBGXEXQWBIDHTVCW4UHANCNFSM5SFFERXA> > . > You are receiving this because you are subscribed to this thread.Message > ID: ***@***.***> > -- *---------------------------------------------------------------------~^`^~._.~' |\ Alex H. Barnett Center for Computational Mathematics, Flatiron Institute | \ http://users.flatironinstitute.org/~ahb 646-876-5942 -- Reply to this email directly or view it on GitHub: #138 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

[janden]

We had a meeting 2 days ago to discuss professionalizing certain such aspects, and I learned about -fvisibility=hidden.

Ok cool.

The issue is already specifically with the GNU compilers (and presumably clang and the intel compilers). So I don't think there's an issue there.

So are you saying that this issues with symbols clashing only occurs with GNU compilers? I don't quite follow.

The problem with finufft:: namespace is that it is not compatible with C (one of our goals), right?

I should say that I think namespacing is a completely valid alternative though. You won't have issues with exceptions and unit tests, and it is automatically portable. You just namespace literally everything except your public API.

I think we're talking about two possible ways to apply namespaces: for all the internal code (to hide the symbols from prying eyes) and for the public API. From what I understand, the latter can be made C-compatible through __cplusplus ifdefs per @DiamonDinoia. That being said, since the public API (for both Finufft and cuFinufft) is all prefixed, I don't think this is too much of an issue.

Professionalizing cufinufft will inevitably come later.

Yes maybe it makes sense to start with Finufft and see what works, then carry it over to cuFinufft. Either way it would solve @DiamonDinoia's predicament…

[blackwer]

So are you saying that this issues with symbols clashing only occurs with GNU compilers? I don't quite follow.

Kinda, yeah. GNU defaults to exposing all symbols. MSVC and some other compilers don't. So we'd have to explicitly hide all the symbols except the API ones (which if they're in different compilation units, would be trivial to do with the flag suggested).

[ahbarnett]

Ok, I did some reading. This is annoying. There is no way to prevent internal symbols from being visible in a static library (.a) because it's just a pile of .o's. I checked this: eg using -fvisibility=hidden for the build of FINUFFT:src/utils_precindep.o turns the nm output for libfinufft.so from T to t for the CNTime functions. But, it has no effect on the libfinufft.a. So this won't help you if you are trying to link static cufinufft and finufft together.
I also checked it with the useful toy repo:
https://github.com/rolsen/demo_symbol_visibility

The only way to do it seems to be namespace, and since we want to keep a C-compatible interface in FINUFFT, I propose only to namespace the functions we want "hidden" (not really hidden, just namespaced to the library).
I really don't like nested :: stuff though, not my idea of fun. I would prefer a single finufft name for all internal routines to finufft.
Does that work?

I will try this with a FINUFFT branch.

[janden]

The only way to do it seems to be namespace, and since we want to keep a C-compatible interface in FINUFFT, I propose only to namespace the functions we want "hidden" (not really hidden, just namespaced to the library).

Right. That makes sense.

I wonder how this is done elsewhere (e.g., FFTW). Clearly they don't have namespaces there (since it's C++-only, IIRC). Maybe they just rely on prefixes to save them from any potential clashes.

[DiamonDinoia]

In fftw they do this: https://github.com/amd/amd-fftw/blob/2b0bbb5924c9e31cb58f34e10832d236bbc51af6/api/api.h#L30
if header-only libraries like eigen they have internal namespaces

[janden]

Huh. So somehow libtool is filtering the symbols and deciding which to export? I guess since we're not using libtool, we don't have that functionality. Also this is only for Windows, right?

[DiamonDinoia]

Huh. So somehow libtool is filtering the symbols and deciding which to export?

This is my guess as well.

I guess since we're not using libtool, we don't have that functionality.

I agree.

Also this is only for Windows, right?

I am not sure. I think gcc uses ranlib but I never checked exactly what happens.
target_link_libraries PRIVATE does the trick for me. I create and internal static library and link against it. This avoids me to use ranlib, libtool or ar specific flags.

https://cmake.org/pipermail/cmake/2016-May/063400.html

[janden]

Ok got it. Couldn't help digging deeper down the rabbit hole and found this: https://stackoverflow.com/a/44674115

So it looks like another option is to use -fvisibility=hidden (or declaring the functions themselves to be hidden manually), then run objcopy --localize-hidden --strip-unneeded. The first will mark the symbols as hidden, which prevents dynamic, but not static linking (as @ahbarnett points out). The second command, however, will remove these hidden symbols from the file, which is what we want.

[janden]

As for FFTW, it looks like the static lib exports all sorts of internal symbols (e.g., fftw_plan_destroy_internal, fftw_mkproblem_unsolvable, fftw_tensor_inplace_strides2, and so on, which don't seem to be available in the public API). Looks like they're just relying on the fact that no one's going to define too many functions starting with fftw_ in their own code.

[mortenwp]

Just a comment from the side because I potentially can end in the same scenario (I need to have a shared library that links in both finufft libraries static) :

If its only the CNTime class that is playing up could the name of that class simply be changed in one of the two libraries, either manually or by using #define's or typedefs which is pretty similar to how the float/double interfaces are handled. I.e I changed my makefile to include a -DCNTime=AltTime, and all CNTime symbols in my archives were now AltTime. This is just a poor mans solution to the problem similar to prefixing of function and class names.

Secondly I see that the CNTime is mainly used in the tests, but also in cufinufft.cu, where it is created and started (Line 238) but only used (Line 249) if compiled with TIME defined. Maybe the construction/start (at line 238) should also be inside the preceding #ifdef TIME block.

cufinufft/src/cufinufft.cu

Lines 232 to 250 in c17b3a9

	#ifdef TIME
	cudaEventRecord(stop);
	cudaEventSynchronize(stop);
	cudaEventElapsedTime(&milliseconds, start, stop);
	printf("[time ] \tCUFFT Plan\t\t %.3g s\n", milliseconds/1000);
	#endif
	CNTime timer; timer.start();
	complex<double> a[3*MAX_NQUAD];
	FLT f[3*MAX_NQUAD];
	onedim_fseries_kernel_precomp(nf1, f, a, d_plan->spopts);
	if(dim > 1){
	onedim_fseries_kernel_precomp(nf2, f+MAX_NQUAD, a+MAX_NQUAD, d_plan->spopts);
	}
	if(dim > 2){
	onedim_fseries_kernel_precomp(nf3, f+2*MAX_NQUAD, a+2*MAX_NQUAD, d_plan->spopts);
	}
	#ifdef TIME
	printf("[time ] \tkernel fser (1st half on CPU):\t %.3g s\n", timer.elapsedsec());
	#endif

The duplicate symbols will still end up in the respective static libraries, but I don't know if it would cause a clash in the combined shared library or not . None of the library code would never try to construct a time object, unless compiled with TIME defined, but I think there is still a fair chance that the shared library would try to resolve them :/

I should probably not try to comment on the visibility stuff . It is the probably the right route, but all still very mysterious to me :o)

[mortenwp]

Along the same lines: the FLT in finufft defines clashes with a typename in boost::ublas. (and I suspect that the same issue would exists in cufinufft)
The preprocessor changes the FLT to either float or double,so ublas suddenly has some of its tempates defined as
template ⟨typename double⟩
I think that suggests that FLT maybe should be safeguarded as FINUFFT_FLT? For the time being I seem to be able to get around by using an undef FLT before including the ublas headers, so its not show stopper

[ahbarnett]

I have PR ready in FINUFFT that cleans up the library symbols so that they are all safe (see convo):
flatironinstitute/finufft#208

I will pull into master in next day or two.
This will of course prevent a clash with cuFINUFFT. It also should be done for cuFINUFFT itself, which I leave as a self-contained task for one of us soon.

[DiamonDinoia]

Dear @ahbarnett,

Sorry for the delay I was busy with other things lately.
I just checked and I can use both finufft and cufinufft static together without issues!

Thanks for looking into this.

[janden]

Agree that we should try to do the same type of cleanup here in cufinufft.

[ahbarnett]

Dear Marco, That's great, so to confirm this is the lib symbol namespacing (finufft:: etc) in master that fixed it? Dear Mortenwp, The name-safe public macros (ie, brand new finufft.h), I finished yesterday and are not master yet - give me another day to bring in the PR (then merge in other recent PRs): flatironinstitute/finufft#217 If you have any comments or can test it now works compiling against boost, now is the time to let us know... thanks! Best wishes & thanks for your help professionalizing this! Alex

…

On Wed, Jun 8, 2022 at 9:20 AM Joakim Andén ***@***.***> wrote: Agree that we should try to do the same type of cleanup here in cufinufft. — Reply to this email directly, view it on GitHub <#138 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACNZRSW2OFXSE6Q4O6LFGQTVOCMYHANCNFSM5SFFERXA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- *---------------------------------------------------------------------~^`^~._.~' |\ Alex H. Barnett Center for Computational Mathematics, Flatiron Institute | \ http://users.flatironinstitute.org/~ahb 646-876-5942

[DiamonDinoia]

Dear Alex,

Yes, I confirm is fixed. At least in my use case.

Thanks,
Marco

[mortenwp]

It looks like its a big step in the right direction 👍 I will pull down the changes, and see how it works where I had a conflict with boost

Update: yes I can confirm that it fixes the issue that I had with the boost headers

[ahbarnett]

great. We will bring into master shortly then. And close this issue (although headers need to be cleaned on cufinufft too).

[blackwer]

So are you saying that this issues with symbols clashing only occurs with GNU compilers? I don't quite follow.

Kinda, yeah. GNU defaults to exposing all symbols. MSVC and some other compilers don't. So we'd have to explicitly hide all the symbols _except_ the API ones (which if they're in different compilation units, would be trivial to do with the flag suggested).

…

On Thu, Mar 31, 2022 at 10:56:22AM -0700, Joakim Andén wrote: > We had a meeting 2 days ago to discuss professionalizing certain such aspects, and I learned about -fvisibility=hidden. Ok cool. > The issue is already specifically with the GNU compilers (and presumably clang and the intel compilers). So I don't think there's an issue there. So are you saying that this issues with symbols clashing only occurs with GNU compilers? I don't quite follow. > The problem with finufft:: namespace is that it is not compatible with C (one of our goals), right? > I should say that I think namespacing is a completely valid alternative though. You won't have issues with exceptions and unit tests, and it is automatically portable. You just namespace literally everything except your public API. I think we're talking about two possible ways to apply namespaces: for all the internal code (to hide the symbols from prying eyes) and for the public API. From what I understand, the latter can be made C-compatible through `__cplusplus` ifdefs per @DiamonDinoia. That being said, since the public API (for both Finufft and cuFinufft) is all prefixed, I don't think this is too much of an issue. > Professionalizing cufinufft will inevitably come later. Yes maybe it makes sense to start with Finufft and see what works, then carry it over to cuFinufft. Either way it would solve @DiamonDinoia's predicament… -- Reply to this email directly or view it on GitHub: #138 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>