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

feat(build): disable semantic interposition #601

Closed
wants to merge 3 commits into from

Conversation

aws-nslick
Copy link
Contributor

@aws-nslick aws-nslick commented Sep 13, 2024

Description of changes:

feat(build): disable semantic interposition

Tell the compiler to treat symbols marked explicitly as
NCCL_OFI_EXPORT_SYMBOL as preemptible, but not others. This gives the
compiler the freedom to assume that no external LD_PRELOAD'd library can
inject itself as an implementation of any given function in the library,
which means the compiler has the freedom to optimize across function.

Signed-off-by: Nicholas Sielicki <[email protected]>

1 file changed, 10 insertions(+), 1 deletion(-)
configure.ac | 11 ++++++++++-
fix(build): add missing AC_PROG_RANLIB

Automake manual:

> This is required if any libraries are built in the package. See
> Particular Program Checks in The Autoconf Manual.

add this for correctness.

Signed-off-by: Nicholas Sielicki <[email protected]>

1 file changed, 1 insertion(+)
configure.ac | 1 +
fix(build): ensure -pthread is passed

Linking against pthread and adding the compiler option for pthread are
two different things. Without the compiler option, various builtin
'#define's are not set. Notably, this should change the behavior around
errno in a multithreaded context within glibc; _REENTRANT will now be
defined and errno is now thread-local.

Signed-off-by: Nicholas Sielicki <[email protected]>

1 file changed, 3 insertions(+)
configure.ac | 3 +++

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@aws-nslick aws-nslick requested review from bwbarrett and a team as code owners September 13, 2024 17:45
@aws-nslick
Copy link
Contributor Author

aws-nslick commented Sep 13, 2024

I was hoping to avoid putting this into the stack, however, it's also hitting the issue fixed in #589 so no such luck.

edit: actually, this reintroduces the bug that #589 fixed -- unclear to me whether or not we can just move this line below the dependency resolution or not.

@aws-nslick aws-nslick marked this pull request as draft September 13, 2024 17:51
Copy link
Contributor

@bwbarrett bwbarrett left a comment

Choose a reason for hiding this comment

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

I think all the flags are safe (some useless, but safe) for executable, but do be careful because these are all going to end up in the compile and link commands for the tests.

configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
@aws-nslick aws-nslick changed the title feat(build): enable lto, disable semantic interposition feat(build): disable semantic interposition Sep 26, 2024
Linking against pthread and adding the compiler option for pthread are
two different things. Without the compiler option, various builtin
'#define's are not set. Notably, this should change the behavior around
errno in a multithreaded context within glibc; _REENTRANT will now be
defined and errno is now thread-local.

Signed-off-by: Nicholas Sielicki <[email protected]>
Automake manual:

> This is required if any libraries are built in the package. See
> Particular Program Checks in The Autoconf Manual.

add this for correctness.

Signed-off-by: Nicholas Sielicki <[email protected]>
Tell the compiler to treat symbols marked explicitly as
NCCL_OFI_EXPORT_SYMBOL as preemptible, but not others. This gives the
compiler the freedom to assume that no external LD_PRELOAD'd library can
inject itself as an implementation of any given function in the library,
which means the compiler has the freedom to optimize across function.

Signed-off-by: Nicholas Sielicki <[email protected]>
@aws-nslick
Copy link
Contributor Author

@bwbarrett I know you hate this and I am sorry to split a review, again, but once we land this change, it gives the compiler visibility into the fact that there are uninitialized stack variables that are not guarded with NULL checks, and so the build fails due to new compiler warnings.

I didn't notice it previously, because it was fixed with #594

So I've rebased this commit on that PR, which means that means that the source branch for this PR needs to move from my fork into the github stack suffix on the main repo. I'll repost your comments on the previous PR, but they've been addressed.

Closing this PR and moving over to #624

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.

2 participants