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

ompi_setup_fc.m4: use -Wl,-ld_classic if supported #12650

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

jsquyres
Copy link
Member

@jsquyres jsquyres commented Jul 2, 2024

Per #12427, on MacOS, add -Wl,-ld_classic to the Fortran wrapper compiler if that flag is needed.

Specifically, Open MPI has used -Wl,-commons,use_dylibs for decades to support common symbols (e.g., MPI_BOTTOM) in the Fortran bindings. There is a window of Xcode versions where this switch was effectively disabled; it effectively required the additional -Wl,-ld_classic switch to force the use of the "old" Apple linker (that still supported -Wl,-commons,use_dylibs). Update the configury to test whether we need -Wl,-ld_classic or not.

Refs #12427

FYI @jeffhammond @fxcoudert @ggouaillardet

@fxcoudert
Copy link

I think that, with Xcode 16 beta, the support of -commons use_dylibs was added to the new linker. So -ld_classic really should only be passed if -commons use_dylibs is otherwise rejected.

@jsquyres
Copy link
Member Author

jsquyres commented Jul 6, 2024

@fxcoudert Ah -- interesting point! That certainly makes things more complicated. I'll push an update here that seems to work for me -- can you check it as well?

I'm going to switch this PR back to draft status because I'm not entirely happy with how I coded up the configury; @bwbarrett can you chime in with any better suggestions on how to build the shared library in _OMPI_SETUP_FC_XCODE_COMMONS_LDFLAGS_BACKEND? (the libtool executable won't exist yet)

@jsquyres jsquyres marked this pull request as draft July 6, 2024 13:48
@jsquyres jsquyres force-pushed the pr/macos-fortran-ld-classic branch 2 times, most recently from a083686 to 9ef1042 Compare July 7, 2024 14:38
@jsquyres
Copy link
Member Author

jsquyres commented Jul 7, 2024

The above force-push was to update/clarify some of the explanation comments -- no code or logic changed.

@jsquyres jsquyres marked this pull request as ready for review July 8, 2024 19:41
@jsquyres
Copy link
Member Author

jsquyres commented Jul 8, 2024

@bwbarrett and I talked through this on the RM call today; this PR seems to be a reasonable compromise. Move this PR out of Draft state.

@fxcoudert and/or anyone else: can you test this on your systems and see if it fixes the issue for you?

config/ompi_setup_fc.m4 Outdated Show resolved Hide resolved
config/ompi_setup_fc.m4 Outdated Show resolved Hide resolved
config/ompi_setup_fc.m4 Show resolved Hide resolved
config/ompi_setup_fc.m4 Outdated Show resolved Hide resolved
config/ompi_setup_fc.m4 Show resolved Hide resolved
@jsquyres jsquyres marked this pull request as draft July 8, 2024 23:01
Per open-mpi#12427, on MacOS, add
-Wl,-ld_classic to the Fortran wrapper compiler if that flag is
needed.

Specifically, Open MPI has used -Wl,-commons,use_dylibs for decades to
support common symbols (e.g., MPI_BOTTOM) in the Fortran bindings.
There is a window of Xcode versions where this switch was effectively
disabled; it effectively required the additional -Wl,-ld_classic
switch to force the use of the "old" Apple linker (that still
supported -Wl,-commons,use_dylibs).  Update the configury to test
whether we need -Wl,-ld_classic or not.

Signed-off-by: Jeff Squyres <jeff@squyres.com>
@jsquyres jsquyres force-pushed the pr/macos-fortran-ld-classic branch from 9ef1042 to 84555f0 Compare July 9, 2024 19:05
@jsquyres jsquyres marked this pull request as ready for review July 9, 2024 19:06
@jsquyres
Copy link
Member Author

jsquyres commented Jul 9, 2024

@bwbarrett All issues have been addressed; please re-review.

@jsquyres jsquyres merged commit b8bd82b into open-mpi:main Jul 10, 2024
13 checks passed
@jsquyres jsquyres deleted the pr/macos-fortran-ld-classic branch July 10, 2024 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants