Page MenuHomePhabricator

Add --unwindlib=[libgcc|compiler-rt] to parallel --rtlib= [take 2]
ClosedPublic

Authored by saugustine on Mar 7 2019, 2:06 PM.

Details

Summary

"clang++ hello.cc --rtlib=compiler-rt"

now can works without specifying additional unwind or exception
handling libraries.

This reworked version of the feature no longer modifies today's default
unwind library for compiler-rt: which is nothing. Rather, a user
can specify -DCLANG_DEFAULT_UNWINDLIB=compiler-rt when configuring
the compiler.

This should address the issues from the previous version.

Update tests for new --unwindlib semantics.

Diff Detail

Repository
rL LLVM

Event Timeline

saugustine created this revision.Mar 7 2019, 2:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2019, 2:06 PM
srhines added a subscriber: kongyi.Mar 7 2019, 3:10 PM
phosek added inline comments.Mar 7 2019, 7:41 PM
clang/include/clang/Driver/ToolChain.h
104 ↗(On Diff #189782)

I think it's confusing to call LLVM's libunwind library compiler-rt since they're two separate and distinct LLVM subprojects. Is there a plan to move LLVM's libunwind into compiler-rt?

mgorny added inline comments.Mar 7 2019, 8:13 PM
clang/include/clang/Driver/ToolChain.h
104 ↗(On Diff #189782)

Also, note that -lunwind may actually be 'non-GNU libunwind'. Both libraries normally use the same name.

MaskRay added inline comments.
clang/include/clang/Driver/ToolChain.h
104 ↗(On Diff #189782)

Yeah there are lots of projects called "libunwind":

llvm libunwind
nongnu libunwind https://www.nongnu.org/libunwind/
PathScale libunwind https://github.com/pathscale/libunwind (this is used by default On FreeBSD)
(libgcc_s.so.1 doesn't call itself "libunwind")

phosek added inline comments.Mar 8 2019, 11:21 AM
clang/include/clang/Driver/ToolChain.h
104 ↗(On Diff #189782)

There have been suggestions to rename LLVM's libunwind to avoid this confusion in the past, maybe it's time to do it?

saugustine marked an inline comment as done.Mar 8 2019, 11:31 AM
saugustine added inline comments.
clang/include/clang/Driver/ToolChain.h
104 ↗(On Diff #189782)

The issues with the name "libunwind" come up pretty much constantly, and various people have proposed renaming llvm's libunwind (as someone did when commenting on v1 of this patch), but it never really goes anywhere. Others have suggested rolling it into compiler-rt, which makes quite a bit of sense to me, but is not really something I can do.

libgcc_s.so.1 doesn't call itself libunwind, but it provides the unwind symbols. As does libgcc_eh.

If you bootstrap clang with -DLLVM_ENABLE_PROJECTS="...,libunwind", then clang tries to use llvm's libunwind in its testing if -lunwind appears on the link line. It isn't built in enough variants to even run the test suite. For example, it would be missing a normal x86 abi build. Compare that to compiler-rt, which builds a copy for every variant.

So solving libunwind's name problem is out of scope for this patch.

Perhaps the best thing to do here is to make the option an actual driver argument string, rather than name an. --unwindlib="-lunwind" or "-lgcc_s".

saugustine marked an inline comment as done.Mar 11 2019, 3:35 PM
saugustine added inline comments.
clang/include/clang/Driver/ToolChain.h
104 ↗(On Diff #189782)

There are various plans, see, for example, https://reviews.llvm.org/D57128.

But the current plan is to wait until after things solidify on github, and I would prefer not to wait on this patch.

And I can't handle this as a string, because the libgcc unwind library changes depending on whether the link is static and so on.

So I think this patch as written is the best option, unless just adding "libunwind" instead of compiler-rt is people's choice. But which libunwind gets found is highly system dependent.

rsmith: Do you have any comments on the naming here?

"--unwindlib=compiler-rt" has the disadvantage that the llvm-unwind lib isn't inside compiler-rt (although there are hints of moving it there).

Using --unwindlib="some-string-passed-exactly-as-is-to-the-linker" cannot work because we should use "-lgcc_s" for some links and "-lgcc_eh" for others.

-lunwind has the disadvantage of picking up whatever libunwind happens to be first in the search path. Fixing that would require moving llvm-libunwind into compiler-rt and naming it differently.

-lunwind has the disadvantage of picking up whatever libunwind happens to be first in the search path. Fixing that would require moving llvm-libunwind into compiler-rt and naming it differently.

Is this really a problem? Don't forget the dynamic loader is going to use the first library in the search path as well.

E5ten added a subscriber: E5ten.Mar 16 2019, 5:20 PM
This comment was removed by saugustine.
  • Undo cpu model change.
  • Add --unwindlib=[libgcc|compiler-rt] to parallel --rtlib= [take 2]
  • Change option argument --unwindlib= from "compiler-rt" to "libunwind".
  • Fix tests syntax and driver checks for --unwindlib=

I have now switched it to use --unwindlib=[libgcc|libunwind]

If there are no objections to this going in, I'm going to submit tomorrow morning.

phosek:
kristina:
mstorsjo:
uabelho:

I'm hoping for your review because the last version of this change broke your builds. Any comments?

MaskRay added inline comments.Mar 18 2019, 10:20 PM
clang/lib/Driver/ToolChain.cpp
695 ↗(On Diff #191174)

Is it return ToolChain::UNW_CompilerRT;?

MaskRay accepted this revision.Mar 18 2019, 10:32 PM
MaskRay added inline comments.
clang/lib/Driver/ToolChain.cpp
695 ↗(On Diff #191174)

Sorry I didn't read the previous discussion :)

702 ↗(On Diff #191174)

} else if

This revision is now accepted and ready to land.Mar 18 2019, 10:32 PM
saugustine marked 4 inline comments as done.Mar 19 2019, 12:57 PM
saugustine added inline comments.
clang/lib/Driver/ToolChain.cpp
695 ↗(On Diff #191174)

Just so that no one else needs to go back and read the previous discussion:

Current users of --rtlib=compiler-rt use a wide variety of platform specific methods of getting their libunwind symbols, and anything else breaks their build.

So the default when --rtlib=compiler-rt is no unwind library at all.

This revision was automatically updated to reflect the committed changes.
saugustine marked an inline comment as done.

This broke our builders, the error is the following:

FAIL: Clang :: Driver/compiler-rt-unwind.c (4674 of 14362)
******************** TEST 'Clang :: Driver/compiler-rt-unwind.c' FAILED ********************
Script:
--
: 'RUN: at line 4';   /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/clang -no-canonical-prefixes /b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c -### -o /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/tools/clang/test/Driver/Output/compiler-rt-unwind.c.tmp.o 2>&1      --target=x86_64-unknown-linux      --gcc-toolchain=""    | /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/FileCheck --check-prefix=RTLIB-EMPTY /b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c
: 'RUN: at line 11';   /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/clang -no-canonical-prefixes /b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c -### -o /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/tools/clang/test/Driver/Output/compiler-rt-unwind.c.tmp.o 2>&1      --target=x86_64-unknown-linux -rtlib=libgcc      --gcc-toolchain=""    | /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/FileCheck --check-prefix=RTLIB-GCC /b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c
: 'RUN: at line 18';   /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/clang -no-canonical-prefixes /b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c -### -o /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/tools/clang/test/Driver/Output/compiler-rt-unwind.c.tmp.o 2>&1      --target=x86_64-unknown-linux -rtlib=libgcc --unwindlib=libunwind      --gcc-toolchain=""    | /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/FileCheck --check-prefix=RTLIB-GCC-UNWINDLIB-COMPILER-RT /b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c
: 'RUN: at line 25';   /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/clang -no-canonical-prefixes /b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c -### -o /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/tools/clang/test/Driver/Output/compiler-rt-unwind.c.tmp.o 2>&1        --target=x86_64-unknown-linux -rtlib=compiler-rt      --gcc-toolchain=""    | /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/FileCheck --check-prefix=RTLIB-COMPILER-RT /b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c
: 'RUN: at line 31';   /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/clang -no-canonical-prefixes /b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c -### -o /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/tools/clang/test/Driver/Output/compiler-rt-unwind.c.tmp.o 2>&1        --target=x86_64-unknown-linux -rtlib=compiler-rt --unwindlib=libgcc      --gcc-toolchain=""    | /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/FileCheck --check-prefix=RTLIB-COMPILER-RT-UNWINDLIB-GCC /b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c
: 'RUN: at line 38';   /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/clang -no-canonical-prefixes /b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c -### -o /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/tools/clang/test/Driver/Output/compiler-rt-unwind.c.tmp.o 2>&1                   --target=x86_64-unknown-linux -rtlib=compiler-rt --unwindlib=libgcc      -static --gcc-toolchain=""    | /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/FileCheck --check-prefix=RTLIB-COMPILER-RT-UNWINDLIB-GCC-STATIC /b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c
: 'RUN: at line 45';   not /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/clang -no-canonical-prefixes /b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c -o /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/tools/clang/test/Driver/Output/compiler-rt-unwind.c.tmp.o 2> /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/tools/clang/test/Driver/Output/compiler-rt-unwind.c.tmp.err                   --target=x86_64-unknown-linux -rtlib=libgcc --unwindlib=libunwind      --gcc-toolchain=""  /b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/FileCheck --input-file=/b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/tools/clang/test/Driver/Output/compiler-rt-unwind.c.tmp.err --check-prefix=RTLIB-GCC-UNWINDLIB-COMPILER_RT /b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c
--
Exit Code: 1

Command Output (stderr):
--
/b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c:8:17: error: RTLIB-EMPTY: expected string not found in input
// RTLIB-EMPTY: "{{.*}}lgcc"
                ^
<stdin>:1:1: note: scanning from here
Fuchsia clang version 9.0.0 (https://fuchsia.googlesource.com/a/third_party/llvm-project ba92e9bb1187a2c43ab727fb9685b65898a87a9d) (based on LLVM 9.0.0svn)
^
<stdin>:5:979: note: possible intended match here
 "/b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/bin/clang" "-cc1" "-triple" "x86_64-unknown-linux" "-emit-obj" "-mrelax-all" "--mrelax-relocations" "-disable-free" "-main-file-name" "compiler-rt-unwind.c" "-mrelocation-model" "static" "-mthread-model" "posix" "-mdisable-fp-elim" "-fmath-errno" "-masm-verbose" "-mconstructor-aliases" "-munwind-tables" "-fuse-init-array" "-target-cpu" "x86-64" "-dwarf-column-info" "-debugger-tuning=gdb" "-resource-dir" "/b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/lib/clang/9.0.0" "-internal-isystem" "/usr/local/include" "-internal-isystem" "/b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/lib/clang/9.0.0/include" "-internal-externc-isystem" "/usr/include/x86_64-linux-gnu" "-internal-externc-isystem" "/include" "-internal-externc-isystem" "/usr/include" "-fdebug-compilation-dir" "/b/s/w/ir/k/recipe_cleanup/clangcmtTrW/llvm_build_dir/tools/clang/test/Driver" "-ferror-limit" "19" "-fmessage-length" "0" "-fobjc-runtime=gcc" "-fdiagnostics-show-option" "-o" "/b/s/w/ir/tmp/t/lit_tmp_A2OKVE/compiler-rt-unwind-347fd6.o" "-x" "c" "/b/s/w/ir/k/llvm-project/clang/test/Driver/compiler-rt-unwind.c" "-faddrsig"
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  ^

--

The full log is here: https://luci-milo.appspot.com/p/fuchsia/builders/ci/clang-x64-linux/b8918524447769519952

We set compiler-rt as the default runtime library in our toolchain (through CLANG_DEFAULT_RTLIB), so this check is invalid:

// RTLIB-EMPTY: "{{.*}}lgcc"
// RTLIB-EMPTY: "{{.*}}-lgcc_s"

Is it possible to either fix the test or revert this change? Our builders have been failing for second day. I would have sent a patch but I'm out-of-office until next week.