Page MenuHomePhabricator

Don't defer to the GCC driver for linking arm-baremetal
ClosedPublic

Authored by jroelofs on May 16 2017, 2:25 PM.

Details

Reviewers
compnerd
joerg

Diff Detail

Event Timeline

jroelofs created this revision.May 16 2017, 2:25 PM
jroelofs updated this revision to Diff 99210.May 16 2017, 2:54 PM

pass through linker flags, and remove my own paths from the test.

joerg edited edge metadata.May 16 2017, 3:57 PM

While I fully agree that the current fallback-to-GCC behavior is far from helpful, I'm not sure how much sense providing linker support for bare-metal makes. I would expect this to changes wildly depending on the specific environment.

I would expect this to changes wildly depending on the specific environment.

My assertion is that our default "specific environment" ought to cater to using llvm's own tools... at least until someone comes along and says they actually want -fuse-ld=gcc, or whatever.

compnerd requested changes to this revision.May 16 2017, 6:30 PM
compnerd added inline comments.
cmake/caches/BaremetalARM.cmake
2

Please rename this file to BareMetalARMv6.cmake. (I'm interested in the suffix primarily).

lib/Driver/ToolChains/BareMetal.cpp
69

Why not just the standard arm directory?

75

This seems wrong. The libc++ headers should *not* be in the resource dir. This should be based off of the sysroot IMO.

108–109

I think that this is a bit extreme. We already have -stdlib=libc++ and -stdlib=libstdc++. Why not just honor that?

lib/Driver/ToolChains/BareMetal.h
43

I think that this really should be ld still, as that is the canonical name for the linker.

58

Unnecessary?

68

Update this accordingly.

This revision now requires changes to proceed.May 16 2017, 6:30 PM
jroelofs planned changes to this revision.May 17 2017, 8:08 AM
jroelofs added inline comments.
cmake/caches/BaremetalARM.cmake
2

My plan is to eventually add multilibs to this configuration, so while that makes sense short term, I don't think it makes sense long term.

With that in mind, do you still want me to rename it?

lib/Driver/ToolChains/BareMetal.cpp
69

There are a few differences between the stuff in the existing ones, and what is needed on baremetal. For example __enable_execute_stack, emutls, as well as anything else that assumes existence of pthreads support shouldn't be there.

75

Oh, excellent point. I was just making that same argument a few weeks ago.

108–109

I wasn't going for "support every possible thing out of the tin", instead preferring incremental development :)

jroelofs added inline comments.May 17 2017, 2:32 PM
lib/Driver/ToolChains/BareMetal.h
43

You mean lld?

$ lld
lld is a generic driver.
Invoke ld.lld (Unix), ld (macOS) or lld-link (Windows) instead.

Or are you saying: "make binutils ld the default, not llvm's lld"?

jroelofs updated this revision to Diff 99350.May 17 2017, 2:35 PM
jroelofs edited edge metadata.
jroelofs marked 2 inline comments as done.May 17 2017, 2:41 PM
jroelofs added inline comments.
lib/Driver/ToolChains/BareMetal.cpp
108–109

Added support for -stdlib=.

I made the assumption that -stdlib=libstdc++ implies the user wants libsupc++ as their ABI library, though someone may want libstdc++ + libc++abi, or other combinations. If that's the case, we can add -abilib=, and likewise -unwinder=... but let's save that for another patch, on another day.

compnerd added inline comments.May 21 2017, 8:59 PM
cmake/caches/BaremetalARM.cmake
2

I think it makes sense longer term still. ARMv6 vs ARMv7. Even if you do multilib, that wouldnt take care of that right?

lib/Driver/ToolChains/BareMetal.cpp
69

Well, I think that "baremetal" here is a bad idea. How about using the android approach? Use clang_rt.builtins-arm-baremetal.a ?

108–109

Yeah, switching between libc++/libc++abi and libstdc++/libsupc++ sounds reasonable. Anything beyond that is something which doesnt work well within the driver anyways. I should polish off my -unwinder patch.

lib/Driver/ToolChains/BareMetal.h
43

Im saying use the name "ld". ld is a symlink on most linux distributions these days. ld -> ld.gold, ld.bfd, ld.lld

jroelofs added inline comments.May 22 2017, 1:32 PM
cmake/caches/BaremetalARM.cmake
2

It's not limited to just ARMv6m multilibs... imagine it had:

set(LLVM_BUILTIN_TARGETS "armv6m-none-eabi;armv7m-none-eabi" CACHE STRING "Builtin Targets")

Then naming the file BaremetalARMv6m.cmake will be inappropriately specific.

Granted, the way clangrt is built/used now isn't very conducive to multilibs that differ by more than just the arch, since the name currently has to be of the form: libclang_rt-builtins-${triple's subarch}.a, which for example doesn't allow for differentiating between:

v7m;@mthumb@march=armv7-m
v7m-PIC;@mthumb@march=armv7-m@fPIC

or

v7a-neon;@march=armv7-a@mfloat-abi=softfp@mfpu=neon
v7a-vfpv3-d16;@march=armv7-a@mfloat-abi=softfp@mfpu=vfpv3-d16
lib/Driver/ToolChains/BareMetal.cpp
69

Why? Given the way the cmake goop works in lib/runtimes + compiler-rt, the folder name there has to be the same as the CMAKE_SYSTEM_NAME. The alternative, I guess, is to call it 'generic', but I'm not convinced that's better than 'baremetal'.

lib/Driver/ToolChains/BareMetal.h
43

I don't think this makes sense. I don't care about "most linux distributions", but rather about putting together an LLVM baremetal distribution based as much as possible on LLVM components. If someone wants a different linker, they can use -fuse-ld= and/or -B.

Also, doesn't using a symlink named ld cause lld to behave like a mach-o linker? We really want the elf one here.

ruiu added a subscriber: ruiu.May 22 2017, 2:02 PM
ruiu added inline comments.
lib/Driver/ToolChains/BareMetal.h
43

If you invoke lld as ld, it behaves as a Mach-O linker on macOS. On other OSes, it behaves as an ELF linker.

compnerd added inline comments.May 22 2017, 9:12 PM
lib/Driver/ToolChains/BareMetal.cpp
69

Because I can have a baremetal environment that uses a different architecture. How do you differentiate between the MIPS and ARM bare metal runtimes? The way that the compiler actually looks up the builtins is that it uses clang_rt.[component]-[arch][-variant]

lib/Driver/ToolChains/BareMetal.h
43

ld really is just the unix name for the linker. I think that at some point we are going to have to consider cross-linking with lld. Adding a -flavor <flavor> with -fuse-ld=lld makes sense I think.

jroelofs added inline comments.May 23 2017, 8:03 AM
lib/Driver/ToolChains/BareMetal.cpp
69

Yes, and that's still how they're being looked up (and built/installed), even in this patch:

lib/clang/[version]/lib/[cmake_system_name]/libclangrt.[component]-[arch][-variant].a

Having arch+variant in the name means they won't intersect, just as they don't for any other system. The only difference here is that baremetal doesn't really have a "system" per se, and it's not appropriate to use the darwin/linux/whatever ones, hence the 'baremetal' folder.

jroelofs updated this revision to Diff 99950.May 23 2017, 10:43 AM
jroelofs planned changes to this revision.May 23 2017, 12:16 PM
jroelofs added inline comments.
cmake/caches/BaremetalARM.cmake
11

hmm. not sure why these two aren't getting built.

jroelofs updated this revision to Diff 100018.May 23 2017, 4:04 PM

fixed the v7m/v7em part of the build

Looks generally pretty good. This is going to be a pretty cool addition!

lib/Driver/ToolChains/BareMetal.cpp
69

Ah. I see, I was visualizing the tree incorrectly. Using baremetal this way is fine by me.

111

Is this layout consistent between libc++ and libstdc++?

131–134

Use StringSwitch?

lib/Driver/ToolChains/BareMetal.h
40

Is the profiler support in compiler-rt sufficiently standalone to build it for baremetal?

jroelofs updated this revision to Diff 100131.May 24 2017, 10:39 AM

implement feedback

jroelofs marked 2 inline comments as done.May 24 2017, 10:47 AM
jroelofs added inline comments.
lib/Driver/ToolChains/BareMetal.cpp
111

Damn, no it's not.

131–134

StringSwitch isn't great when you want to error out of the default case. That being said, this whole function is unnecessary: I can just defer to the base class' implementation, which does almost the same thing.

lib/Driver/ToolChains/BareMetal.h
40

IIRC, there was a test case that wanted this on for one of the arm-none-eabi triples. This is when my patches were against 4.0. After rebasing to trunk, I think that particular test is gone (split up into separate ones, I guess?).

I'll leave it off for now until someone confirms it is actually supported.

lib/Driver/ToolChains/Linux.cpp
379

Noticed that this was in the wrong place, and also, surprisingly, that it was incorrectly qualified.

jroelofs updated this revision to Diff 100177.May 24 2017, 3:44 PM
jroelofs marked an inline comment as done.

Fix a cmake warning:

Platform/baremetal to use this system, please send your config file to cmake@www.cmake.org so it can be added to cmake
Your CMakeCache.txt file was copied to CopyOfCMakeCache.txt. Please send that file to cmake@www.cmake.org.

Generic does what we want already, it just has a name that's not useful.

compnerd accepted this revision.May 24 2017, 7:16 PM
This revision is now accepted and ready to land.May 24 2017, 7:16 PM
jroelofs closed this revision.May 25 2017, 8:42 AM

r303873

This causes a test failure with non-standard CLANG_RESOURCE_DIR:

Command Output (stderr):
--
/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999/test/Driver/baremetal.cpp:8:22: error: expected string not found in input
// CHECK-V6M-C-SAME: "-resource-dir" "[[PREFIX_DIR]]{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}[[VERSION:[^"]*]]"
                     ^
<stdin>:5:128: note: scanning from here
 "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_32.x86/./bin/clang" "-cc1" "-triple" "thumbv6m-none--eabi" "-emit-obj" "-mrelax-all" "-disable-free" "-disable-llvm-verifier" "-discard-value-names" "-main-file-name" "baremetal.cpp" "-mrelocation-model" "static" "-mthread-model" "single" "-mdisable-fp-elim" "-fmath-errno" "-no-integrated-as" "-mconstructor-aliases" "-nostdsysteminc" "-target-cpu" "cortex-m0" "-target-feature" "+soft-float-abi" "-target-feature" "+strict-align" "-target-abi" "aapcs" "-mfloat-abi" "soft" "-dwarf-column-info" "-debugger-tuning=gdb" "-resource-dir" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_32.x86/./bin/../../../../lib/clang/5.0.0" "-isysroot" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999/test/Driver/Inputs/baremetal_arm" "-internal-isystem" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999/test/Driver/Inputs/baremetal_arm/include/c++/v1" "-internal-isystem" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_32.x86/./bin/../../../../lib/clang/5.0.0/include" "-internal-isystem" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999/test/Driver/Inputs/baremetal_arm/include" "-fdeprecated-macro" "-fdebug-compilation-dir" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_32.x86/test/Driver" "-ferror-limit" "19" "-fmessage-length" "0" "-fallow-half-arguments-and-returns" "-fno-signed-char" "-fobjc-runtime=gcc" "-fcxx-exceptions" "-fexceptions" "-fdiagnostics-show-option" "-o" "/var/tmp/portage/sys-devel/clang-9999/temp/baremetal-6b1d37.o" "-x" "c++" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999/test/Driver/baremetal.cpp"
                                                                                                                               ^
<stdin>:5:128: note: with variable "PREFIX_DIR" equal to "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_32.x86/."
 "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_32.x86/./bin/clang" "-cc1" "-triple" "thumbv6m-none--eabi" "-emit-obj" "-mrelax-all" "-disable-free" "-disable-llvm-verifier" "-discard-value-names" "-main-file-name" "baremetal.cpp" "-mrelocation-model" "static" "-mthread-model" "single" "-mdisable-fp-elim" "-fmath-errno" "-no-integrated-as" "-mconstructor-aliases" "-nostdsysteminc" "-target-cpu" "cortex-m0" "-target-feature" "+soft-float-abi" "-target-feature" "+strict-align" "-target-abi" "aapcs" "-mfloat-abi" "soft" "-dwarf-column-info" "-debugger-tuning=gdb" "-resource-dir" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_32.x86/./bin/../../../../lib/clang/5.0.0" "-isysroot" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999/test/Driver/Inputs/baremetal_arm" "-internal-isystem" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999/test/Driver/Inputs/baremetal_arm/include/c++/v1" "-internal-isystem" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_32.x86/./bin/../../../../lib/clang/5.0.0/include" "-internal-isystem" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999/test/Driver/Inputs/baremetal_arm/include" "-fdeprecated-macro" "-fdebug-compilation-dir" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_32.x86/test/Driver" "-ferror-limit" "19" "-fmessage-length" "0" "-fallow-half-arguments-and-returns" "-fno-signed-char" "-fobjc-runtime=gcc" "-fcxx-exceptions" "-fexceptions" "-fdiagnostics-show-option" "-o" "/var/tmp/portage/sys-devel/clang-9999/temp/baremetal-6b1d37.o" "-x" "c++" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999/test/Driver/baremetal.cpp"
                                                                                                                               ^
<stdin>:5:588: note: possible intended match here
 "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_32.x86/./bin/clang" "-cc1" "-triple" "thumbv6m-none--eabi" "-emit-obj" "-mrelax-all" "-disable-free" "-disable-llvm-verifier" "-discard-value-names" "-main-file-name" "baremetal.cpp" "-mrelocation-model" "static" "-mthread-model" "single" "-mdisable-fp-elim" "-fmath-errno" "-no-integrated-as" "-mconstructor-aliases" "-nostdsysteminc" "-target-cpu" "cortex-m0" "-target-feature" "+soft-float-abi" "-target-feature" "+strict-align" "-target-abi" "aapcs" "-mfloat-abi" "soft" "-dwarf-column-info" "-debugger-tuning=gdb" "-resource-dir" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_32.x86/./bin/../../../../lib/clang/5.0.0" "-isysroot" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999/test/Driver/Inputs/baremetal_arm" "-internal-isystem" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999/test/Driver/Inputs/baremetal_arm/include/c++/v1" "-internal-isystem" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_32.x86/./bin/../../../../lib/clang/5.0.0/include" "-internal-isystem" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999/test/Driver/Inputs/baremetal_arm/include" "-fdeprecated-macro" "-fdebug-compilation-dir" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999-abi_x86_32.x86/test/Driver" "-ferror-limit" "19" "-fmessage-length" "0" "-fallow-half-arguments-and-returns" "-fno-signed-char" "-fobjc-runtime=gcc" "-fcxx-exceptions" "-fexceptions" "-fdiagnostics-show-option" "-o" "/var/tmp/portage/sys-devel/clang-9999/temp/baremetal-6b1d37.o" "-x" "c++" "/var/tmp/portage/sys-devel/clang-9999/work/x/y/clang-9999/test/Driver/baremetal.cpp"

This causes a test failure with non-standard CLANG_RESOURCE_DIR:

https://reviews.llvm.org/D33877

(thanks for the patch)