-
Notifications
You must be signed in to change notification settings - Fork 94
Removes vestiges of exported symbols #737
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
Removes vestiges of exported symbols #737
Conversation
|
Maybe I will add a FINUFFT_EXPORT_TEST and remove the duplication. |
|
I like the fact that ducc0 symbols are cleaned up (gone).
Is the point with the other symbols that BUILD_TESTS will be OFF for many
users? I always build tests then use the resulting lib.
Now, here comes the problem: have you tried this with our GNU make route?
This is still the build method of choice for many users, and we have to
support it.
Since FINUFFT_BUILD_TESTS will not be set in that case, I imagine "make
lib" or "make test" breaks.
Could you check and handle this, or we discuss what the route to fix this
before you implement?
Thx, Alex
…On Mon, Oct 20, 2025 at 10:58 PM Marco Barbone ***@***.***> wrote:
*DiamonDinoia* left a comment (flatironinstitute/finufft#737)
<#737 (comment)>
Maybe I will add a FINUFFT_EXPORT_TEST and remove the duplication.
CUDA has two wait.
—
Reply to this email directly, view it on GitHub
<#737 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACNZRSVGGI6TA4QBQITBG6T3YWOOPAVCNFSM6AAAAACJXTNEESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTIMRUGQ4DAMRSGQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
--
*-------------------------------------------------------------------~^`^~._.~'
|\ Alex Barnett Center for Computational Mathematics, Flatiron Institute
| \ http://users.flatironinstitute.org/~ahb 646-876-5942
|
|
If this approach works, I don't think there's any point in merging #710 :) |
|
Hi @mreineck, @ahbarnett & @lu1and10,
Most users don’t build tests when installing FINUFFT. I follow the same principle—when using a dependency, I avoid building its tests. We compile FINUFFT with A future PR can also remove the I am contemplating whether I want to build two libraries for tests vs examples do that even if the user enables finufft tests by mistake, none of the internal symbols bleed into the binary.
On the make side. I do not think there is anything complicated here. We just need to pass the default visibility to be hidden and use a couple of macros to export the required symbols. I could just define the export macro here and #710 can take over and clean them. Cheers, |
|
Ok, why don't you have a go at editing the makefile to make this PR build
via make... Happy to help if you get stuck. Meanwhile, sure, tidy away
#710. Thanks, Alex
…On Wed, Oct 22, 2025 at 11:10 AM Marco Barbone ***@***.***> wrote:
*DiamonDinoia* left a comment (flatironinstitute/finufft#737)
<#737 (comment)>
Hi @mreineck <https://github.com/mreineck>, @ahbarnett
<https://github.com/ahbarnett> & @lu1and10 <https://github.com/lu1and10>,
Thanks for the suggestions.
I like the fact that ducc0 symbols are cleaned up (gone). Is the point
with the other symbols that BUILD_TESTS will be OFF for many users? I
always build tests then use the resulting lib. Now, here comes the problem:
have you tried this with our GNU make route? This is still the build method
of choice for many users, and we have to support it. Since
FINUFFT_BUILD_TESTS will not be set in that case, I imagine "make lib" or
"make test" breaks. Could you check and handle this, or we discuss what the
route to fix this before you implement? Thx, Alex
Most users don’t build tests when installing FINUFFT. I follow the same
principle—when using a dependency, I avoid building its tests. We compile
FINUFFT with -fPIC or -shared, which forbids inlining exported symbols
because that would violate the ELF rules (reference
<https://www.akkadia.org/drepper/dsohowto.pdf>).
Despite this, compilers may still inline via no semantic interposition
<https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-fsemantic-interposition>.
Even with semantic interposition disabled, exported symbols remain emitted,
increasing binary size unnecessarily (example
<https://marco.godbolt.org/z/dz6zsfx46>). Hence, it’s beneficial also to
hide functions used only for tests for both a performance and a binary size
perspective.
A future PR can also remove the static and anonymous namespace reducing
boilerplate.
I am contemplating whether I want to build two libraries for tests vs
examples do that even if the user enables finufft tests by mistake, none of
the internal symbols bleed into the binary.
If this approach works, I don't think there's any point in merging #710
<#710> :)
On the make side. I do not think there is anything complicated here. We
just need to pass the default visibility to be hidden and use a couple of
macros to export the required symbols. I could just define the export macro
here and #710 <#710> can
take over and clean them.
Cheers,
Marco
—
Reply to this email directly, view it on GitHub
<#737 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACNZRSTDDHOIBUBEV2U37EL3Y6M4ZAVCNFSM6AAAAACJXTNEESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTIMZSHEYDQOJRGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
*-------------------------------------------------------------------~^`^~._.~'
|\ Alex Barnett Center for Computational Mathematics, Flatiron Institute
| \ http://users.flatironinstitute.org/~ahb 646-876-5942
|
a2bb6d5 to
aab1bb9
Compare
mreineck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks gopd! Only two very small remarks from my side
include/finufft/spreadinterp.h
Outdated
| const UBIGINT N3, T *data_uniform, const UBIGINT M, T *FINUFFT_RESTRICT kx, | ||
| T *FINUFFT_RESTRICT ky, T *FINUFFT_RESTRICT kz, T *FINUFFT_RESTRICT data_nonuniform, | ||
| const finufft_spread_opts &opts, int did_sort, bool adjoint); | ||
| FINUFFT_EXPORT int spreadinterp(UBIGINT N1, UBIGINT N2, UBIGINT N3, T *data_uniform, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would FINUFFT_EXPORT_TEST be enough here? I think that spreadinterp is not part of the public interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
makefile
Outdated
| CFLAGS := -O3 -funroll-loops -march=native -fcx-limited-range -ffp-contract=fast\ | ||
| -fno-math-errno -fno-signed-zeros -fno-trapping-math -fassociative-math\ | ||
| -freciprocal-math -fmerge-all-constants -ftree-vectorize $(CFLAGS) -Wfatal-errors | ||
| -freciprocal-math -fmerge-all-constants -ftree-vectorize $(CFLAGS) -Wfatal-errors \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trailing backslash seems unnecessary.
779b1aa to
13bc881
Compare
…ntly Resolved inconsistent symbol exports for Fortran wrapper entry points and test helpers across Windows and macOS. Adjusted default visibility in the makefile, added FINUFFT_EXPORT_TEST for test-only APIs, and linked FFT backends via a new finufft_fftlibs interface target. It also adds the LTO option to makefile to achieve parity with cmake. Co-authored-by: Marco Barbone [email protected] Co-authored-by: Martin Reinecke [email protected]
13bc881 to
a6e2f38
Compare
|
@mreineck I merged more constness here. Could you double check that I did not drop any of the const qualifiers? It was a bit of a mess and something might have slipped through. |
a0ae526 to
c9c7131
Compare
|
Since |
|
Looks good! |
|
@ahbarnett would you like to have a look? Otherwise I think it can be merged. |
lu1and10
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, looks good, ci should be good once merged to master.
I'll rebase on Monday and then merge |
btw, the same illegal instruction might happen to other ci using march=native, e.g., valgrind.yml |
I do not think these use sccache but I will double check. |
If it doesn't use sccache, it should be good, though seldom breaks. the new changes in last week, seems to make a hard time for the matlab ci, https://github.com/flatironinstitute/finufft/actions/runs/18810288961/job/53670334663 Your fork's master branch(hasn't pull the updates yet) still works I guess it's because something changed in last week. It seems not about matlab, mostly about apple clang(I tried xcode 16.0,16.1,16.2,16.3 and 16.4 apple clang all breaks) + current flags breaks the fft.cpp front end apple clang complilation. Maybe should use llvm clang as in cmake ci. |
I will have a look though I am not sure what the problem might. Maybe we sit together with your mac to find the root cause? |
|
Sure, I can debug together some time on my mac, I was able to reproduce the failing on my mac. |
To generate:
After this changes the exported symbol list is:
before: