Comments (29)
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.
from cufinufft.
Another question (just out of curiosity), why static as opposed to dynamic linking here?
from cufinufft.
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.
from cufinufft.
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.
from cufinufft.
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?
from cufinufft.
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
from cufinufft.
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
from cufinufft.
Thanks. I'll look into this as soon as I have some spare time.
from cufinufft.
from cufinufft.
from cufinufft.
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…
from cufinufft.
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).
from cufinufft.
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.
from cufinufft.
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.
from cufinufft.
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
from cufinufft.
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?
from cufinufft.
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
from cufinufft.
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.
from cufinufft.
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.
from cufinufft.
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.
Lines 232 to 250 in c17b3a9
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)
from cufinufft.
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
from cufinufft.
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.
from cufinufft.
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.
from cufinufft.
Agree that we should try to do the same type of cleanup here in cufinufft.
from cufinufft.
from cufinufft.
Dear Alex,
Yes, I confirm is fixed. At least in my use case.
Thanks,
Marco
from cufinufft.
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
from cufinufft.
great. We will bring into master shortly then. And close this issue (although headers need to be cleaned on cufinufft too).
from cufinufft.
from cufinufft.
Related Issues (20)
- Move from manylinux2010 to manylinux2014 HOT 6
- finishing up the 1D functionality HOT 2
- Rapidly degrading adjoint test in 3D HOT 4
- gpuNUFFT is faster than cufiNUFFT in 3D HOT 28
- Python timing tests
- Pass/fail checks for tests HOT 1
- Implementing 1.25 upsampling factor with precomputed Horner kernel HOT 4
- Issue with interpolation only option with python wrapper HOT 1
- src/precision_independent.cu needs to include <thrust/extrema.h> HOT 1
- make headers/macros/lib symbols safe. eg FLT currently clashes with a typename in boost::ublas. HOT 2
- Help with possible pts dependent bug HOT 14
- pip install cufinufft throws: No such file or directory: 'python/cufinufft/requirements.txt' HOT 4
- Why is the result different from the nufft function in matlab? HOT 3
- compiling error HOT 2
- Compatibility with Pytorch HOT 2
- Thrust `max_element` header change HOT 1
- Running on AMD HOT 7
- Revisit shared memory limitations
- [cufinufft] modeord parameter HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from cufinufft.