This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ][ZOS] Prefer -nostdlib++ as opposed to -nodefaultlibs when building c++ libraries
ClosedPublic

Authored by zibi on Feb 2 2021, 8:44 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rG5f9be2c3e37c: [SystemZ][ZOS] Prefer -nostdlib++ as opposed to -nodefaultlibs when building…
Summary

Let's use -nostdlib++ rather than -nodefaultlibs when building libc++/libc++abi/libunwind libraries. The default is -nostdlib++ if supported by a build compiler like it is the case with clang, otherwise -nodefaultlibs is used as before.

This change is needed to avoid additional changes at the link step and not to increase the maintenance costs. If clang with -nodefaultlibs is used all the libraries which are removed but required would have to be manually added in. This set of libraries are unique and will send out.

The propose change will allow to make the link step simple for other platforms as well.

Diff Detail

Event Timeline

zibi created this revision.Feb 2 2021, 8:44 AM
zibi requested review of this revision.Feb 2 2021, 8:44 AM
ldionne requested changes to this revision.Feb 2 2021, 9:10 AM
ldionne added a subscriber: ldionne.

The general direction looks alright to me. Can you please use arc diff to upload patches so that the proper metadata is added to the Phabricator revision and CI can run?

libcxx/cmake/config-ix.cmake
28

typo: otherwise -nodefaultlibs

This revision now requires changes to proceed.Feb 2 2021, 9:10 AM
zibi updated this revision to Diff 320849.Feb 2 2021, 10:42 AM
zibi marked an inline comment as done.
zibi edited the summary of this revision. (Show Details)

pached created by arc diff

Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptFeb 2 2021, 10:42 AM
zibi updated this revision to Diff 320857.Feb 2 2021, 11:02 AM

typo + patch with arc diff

ldionne accepted this revision.Feb 2 2021, 12:08 PM

Please wait for CI to pass, and then feel free to ship it. Thanks!

ldionne requested changes to this revision.Feb 2 2021, 1:03 PM

Well, it looks like there are issues to resolve before this can be shipped, as shown by the CI.

This revision now requires changes to proceed.Feb 2 2021, 1:03 PM
zibi updated this revision to Diff 320901.Feb 2 2021, 1:48 PM

Default libunwind to be build with -nodefaultlibs on MINGW.

zibi added a comment.Feb 2 2021, 1:48 PM

On MINGW we need to build libunwind with -nodefaultlibs on b/c LIBUNWIND_SUPPORTS_FUNWIND_TABLES_FLAG is not set,
otherwise we issue this error:

if (LIBUNWIND_ENABLE_SHARED AND
    NOT (LIBUNWIND_SUPPORTS_FNO_EXCEPTIONS_FLAG AND
         LIBUNWIND_SUPPORTS_FUNWIND_TABLES_FLAG))
  message(FATAL_ERROR
          "Compiler doesn't support generation of unwind tables if exception "
          "support is disabled.  Building libunwind DSO with runtime dependency "
          "on C++ ABI library is not supported.")
endif()
mstorsjo added inline comments.Feb 2 2021, 1:52 PM
libunwind/cmake/config-ix.cmake
33

Doesn't this end up adding -nodefaultlibs twice to CMAKE_REQUIRED_FLAGS (the second time below on line 39)?

zibi marked an inline comment as done.Feb 2 2021, 3:10 PM
zibi added inline comments.
libunwind/cmake/config-ix.cmake
33

Doesn't this end up adding -nodefaultlibs twice to CMAKE_REQUIRED_FLAGS (the second time below on line 39)?

Thank you for caching this. Line 39 should be deleted but let me wait first for CI to see if the original regression is gone.

zibi updated this revision to Diff 320973.Feb 2 2021, 7:29 PM
zibi marked an inline comment as done.

reverse previous fix and try something new to test CI

zibi updated this revision to Diff 321171.Feb 3 2021, 11:18 AM

adding none c++ libraries back even for -nostdlib++ for libcxx/libcxxabi/libunwind to check if this will make CI clean

zibi updated this revision to Diff 321418.Feb 4 2021, 6:58 AM

Hopefully this time CI will be clean, the previous one never completed.

zibi added a comment.Feb 4 2021, 10:15 AM

@ldionne Can you check why the last 3 builds always hang in Apple tests?
Here is the link to the latest build.

@ldionne Can you check why the last 3 builds always hang in Apple tests?
Here is the link to the latest build.

The number of builders on mac is very limited. They're running on a few laptops sitting right beside me. You have to be a bit patient. Re-uploading patches to try and shake the CI is actually making things worse, as our BuildKite setup isn't smart enough to cancel jobs that have already been submitted when you re-upload a newer patch.

zibi added a comment.Feb 4 2021, 10:43 AM

@ldionne Can you check why the last 3 builds always hang in Apple tests?
Here is the link to the latest build.

The number of builders on mac is very limited. They're running on a few laptops sitting right beside me. You have to be a bit patient. Re-uploading patches to try and shake the CI is actually making things worse, as our BuildKite setup isn't smart enough to cancel jobs that have already been submitted when you re-upload a newer patch.

It's good to know, thank you. I was suspicious because quite a few builds completed which were submitted AFTER mine so I was warring that they might loop or hang. Now I actually see that the 1st of my builds just completed. It must be some kind of priority I guess.

@ldionne Can you check why the last 3 builds always hang in Apple tests?
Here is the link to the latest build.

The number of builders on mac is very limited. They're running on a few laptops sitting right beside me. You have to be a bit patient. Re-uploading patches to try and shake the CI is actually making things worse, as our BuildKite setup isn't smart enough to cancel jobs that have already been submitted when you re-upload a newer patch.

It's good to know, thank you. I was suspicious because quite a few builds completed which were submitted AFTER mine so I was warring that they might loop or hang. Now I actually see that the 1st of my builds just completed. It must be some kind of priority I guess.

Hmm, actually you're right, newer builds seem to be prioritized over older ones. That's exactly the opposite of what I would expect. That's weird. I'll work on making sure that remaining mac jobs don't run if any other job failed - I think that'll help reduce the load on the mac builders.

Hmm, actually you're right, newer builds seem to be prioritized over older ones. That's exactly the opposite of what I would expect. That's weird. I'll work on making sure that remaining mac jobs don't run if any other job failed - I think that'll help reduce the load on the mac builders.

Ah, I just figured out what was wrong. I changed the queue of the mac builders and your CI was targeting an old queue that doesn't exist. Please rebase onto main and resubmit, your build should go through normally. Sorry about that, I've been wresting with the infrastructure this week.

zibi updated this revision to Diff 321762.Feb 5 2021, 7:54 AM
  • resubmitting to get clean CI, thank you Louis for finding the issue with previous builds
zibi added a comment.Feb 8 2021, 11:41 AM

FYI, @ldionne I'm looking for you approval.

The only delta since the last approval is to run the logic of adding additional libraries in both -nostdlib++ and -nodefaultlibs cases for all 3 libraries. Originally I had the logic of adding additional libraries apply to -nodefaultlibs case only which caused regressions. The configuration for all libcxx, libcxxabi and libunwind libraries are now the same.

ldionne added inline comments.Feb 8 2021, 1:05 PM
libcxx/CMakeLists.txt
729

"In order to remove just libc++ from the link step"

libcxx/cmake/config-ix.cmake
27

Remove "The"

46

I would have expected that this conditional be if (NOT LIBCXX_SUPPORTS_NOSTDLIBXX_FLAG AND LIBCXX_SUPPORTS_NODEFAULTLIBS_FLAG) instead. Indeed, if we use -nostdlib++, we shouldn't need to add these system libraries manually, right?

libcxxabi/cmake/config-ix.cmake
17

Remove "The"

libunwind/cmake/config-ix.cmake
19

Remove "The"

zibi marked 5 inline comments as done.Feb 8 2021, 1:36 PM
zibi added inline comments.
libcxx/cmake/config-ix.cmake
46

That would cause cmake test for -nostdinc++ to fail and eventually it would lead to CI failure. See this failure. The root of the problem is that -nostdinc++ was missing from the compile command because cmake thought this option was not supported.

zibi updated this revision to Diff 322207.Feb 8 2021, 1:39 PM
zibi marked an inline comment as done.

NFC: typos

ldionne accepted this revision.Feb 16 2021, 7:17 AM

I think this is good for libunwind too - I feel confident enough to LGTM this CMake change even though I'm not an owner.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 16 2021, 10:43 AM
This revision was automatically updated to reflect the committed changes.
hvdijk added a subscriber: hvdijk.Oct 17 2021, 5:44 AM

I think this is good for libunwind too

It's not, depending on how clang is configured this can cause the libunwind build to fail because clang now tries to link libunwind against itself. -nodefaultlibs prevents that, -nostdlib++ does not.

/usr/bin/clang  -m64 -fPIC -O2 --target=x86_64-unknown-linux-gnu -DNDEBUG --target=x86_64-unknown-linux-gnu -rtlib=compiler-rt -nostdlib++ -Wl,-O1 -Wl,--as-needed -s -shared -Wl,-soname,libunwind.so.1 -o ../lib/x86_64-linux-gnu/libunwind.so.1.0 CMakeFiles/unwind_shared.dir/libunwind.cpp.o CMakeFiles/unwind_shared.dir/Unwind-EHABI.cpp.o CMakeFiles/unwind_shared.dir/Unwind-seh.cpp.o CMakeFiles/unwind_shared.dir/UnwindLevel1.c.o CMakeFiles/unwind_shared.dir/UnwindLevel1-gcc-ext.c.o CMakeFiles/unwind_shared.dir/Unwind-sjlj.c.o CMakeFiles/unwind_shared.dir/UnwindRegistersRestore.S.o CMakeFiles/unwind_shared.dir/UnwindRegistersSave.S.o  -lc /usr/lib/clang/13.0.0/lib/linux/libclang_rt.builtins-x86_64.a -ldl -lpthread 
ld.lld: error: unable to find library -l:libunwind.so
ld.lld: error: unable to find library -l:libunwind.so
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Would there be any objections to reverting this change for libunwind, or do you prefer to fix this some other way?

Would there be any objections to reverting this change for libunwind, or do you prefer to fix this some other way?

I would prefer to not revert it. The generally more specific way of disabling things with -nostdlibc++ is nicer as it doesn’t need to hardcode/guess what other platform specific libs are linked by default, that has to be specified manually.

However, should we try to use -unwindlib=none at the same time as testing for -nostdlibc++? I think that would take care of this issue, right?

However, should we try to use -unwindlib=none at the same time as testing for -nostdlibc++? I think that would take care of this issue, right?

That should work too, sure, and sounds good to me.