Page MenuHomePhabricator

[libc++] Link against libatomic when it is found
ClosedPublic

Authored by ldionne on Jun 4 2020, 12:38 PM.

Details

Reviewers
jfb
__simt__
Group Reviewers
Restricted Project
Commits
rGe0184357fc78: [libc++] Link against libatomic when it is found
Summary

Before this patch, we tried detecting whether small atomics were available
without linking against libatomic. However, that's not really what we want
to know -- instead, we want to know what's required in order to support
atomics fully, which is to link against libatomic when it's provided.

That is both much simpler, and it doesn't suffer the problem that we would
not link against libatomic when small atomics didn't require it, which
lead to non-lockfree atomics never working.

Furthermore, because we understand that some platforms might not want to
(or be able to) ship non-lockfree atomics, we add that notion to the test
suite, independently of a potential extern library.

After this patch, we therefore:
(1) Link against libatomic when it is provided
(2) Independently detect whether non-lockfree atomics are supported in

the test suite, regardless of whether that means we're linking against
an external library or not (which is an implementation detail).

Diff Detail

Event Timeline

ldionne created this revision.Jun 4 2020, 12:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2020, 12:38 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne marked an inline comment as done.
ldionne added a subscriber: phosek.
ldionne added inline comments.
libcxx/cmake/config-ix.cmake
101

@phosek Is this right for Fuchsia?

I'm not an expert in the details but this looks right to me.

When this goes in, I will be adding non-lock-free atomic tests.

These tests would fail today, meaning we have no coverage today.

griwes added a subscriber: griwes.Jun 4 2020, 2:31 PM

FWIW this looks good to me as well.

jfb added inline comments.Jun 4 2020, 3:06 PM
libcxx/test/libcxx/selftest/dsl/dsl.sh.py
105

Here and for the others, int main(int, char**) 😄

libcxx/utils/libcxx/test/dsl.py
71

I think your filenames need to be quoted properly otherwise spaces in filenames will bork stuff (above and below).

libcxx/utils/libcxx/test/features.py
39

int main(int, char**)

ldionne updated this revision to Diff 268770.Jun 5 2020, 6:27 AM
ldionne marked 4 inline comments as done.

Address JF's comments

libcxx/utils/libcxx/test/dsl.py
71

Lit will expand filenames like %t and directories like %T, the assumption is that it will quote it properly for shell evaluation.

ldionne accepted this revision as: Restricted Project.Jun 5 2020, 6:28 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 5 2020, 6:37 AM
This revision was automatically updated to reflect the committed changes.
jfb added a comment.Jun 5 2020, 8:27 AM

Thanks for this!

On which platform does this commit work ?

On Linux, the test program would still need an explicit -latomic, since indirect dependencies are not used to resolve undefined symbols from the program.
For this to work, it would need PUBLIC atomic instead of PRIVATE atomic, so that libcxx/cmake/Modules/DefineLinkerScript.cmake would add it to the linker script (libc++.so).
It would be even better with AS_NEEDED in the linker script, and -Wl,--as-needed for the lib, in order to only load the library when it is actually used.

On which platform does this commit work ?

On Linux, the test program would still need an explicit -latomic, since indirect dependencies are not used to resolve undefined symbols from the program.
For this to work, it would need PUBLIC atomic instead of PRIVATE atomic, so that libcxx/cmake/Modules/DefineLinkerScript.cmake would add it to the linker script (libc++.so).
It would be even better with AS_NEEDED in the linker script, and -Wl,--as-needed for the lib, in order to only load the library when it is actually used.

Does this commit not work on your system? It has been working on our Linux bots since it was committed.

Regarding adding -latomic to the linker script, I don't know that we actually want to do that. I think the Clang driver should probably be the one to pass the correct -latomic or -l<library-for-builtins> depending on what builtin library we're using.

Does this commit not work on your system? It has been working on our Linux bots since it was committed.

I don't remember if I found a case which needed the library.
My question was more : on which system does linking libc++ with libatomic help ?

If the platform doesn't need libatomic for the particular usecase, then the -latomic for libc++.so means the commit triggers a load a libatomic.so at runtime.
So the commit wouldn't break anything, but it would add a useless dependency.

But if libatomic is actually required, I think there must be a -latomic when linking the binary which uses the symbols (then also linking libc++.so with it doesn't change anything).
On Linux, when linking an executable (or a shared library with -Wl,--no-undefined), the linker checks all symbols referenced in the object files are actually resolved by the direct dependencies.
So while loading libatomic.so indirectly would be enough to resolve the symbols at runtime, on Linux at least the linker would mandate a direct link anyway.

Regarding adding -latomic to the linker script, I don't know that we actually want to do that. I think the Clang driver should probably be the one to pass the correct -latomic or -l<library-for-builtins> depending on what builtin library we're using.

The idea of a using the linker script was that any user of libc++.so would automatically link to libatomic.so when it's required (but not if it isn't), as if it were specified as -Wl,--push-state -Wl,--as-needed -latomic -Wl,--pop-state.
This would even apply to gcc using libc++ for example.