Page MenuHomePhabricator

Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal.
AcceptedPublic

Authored by georgemorgan on Sep 10 2018, 4:54 PM.

Details

Summary

When building for a baremetal triple an extraneous ".a" is appended to the clang_rt.builtins library. This causes the linker to fail to find the static library, as the flag passed to the linker is -lclang_rt.builtins-arm.a instead of -lclang_rt.builtins-arm.

ld.lld: error: unable to find library -lclang_rt.builtins-arm.a

Removing this extraneous '.a' produces the desired behavior and the link succeeds. Tests were updated to reflect the change.

Diff Detail

Event Timeline

georgemorgan created this revision.Sep 10 2018, 4:54 PM
CodaFi added a comment.EditedSep 10 2018, 5:11 PM

Two things:

  • Please re-upload this patch with context (git diff -U99999 or arc patch)
  • This is one answer, but the real problem is that this path is halfway between an absolute path and a link-relative path. We should be searching for these libraries relative to the resource directory and at absolute paths like the other drivers on the other platforms do. To that end, keep the ".a", but change the l to lib and see MachO::AddLinkRuntimeLib for how to add on the resource dir.

Update diff to provide context.

kristina added a comment.EditedSep 10 2018, 5:15 PM

This seems like it would break a lot, .a is just a generic extension for an object archive file, I don't think the extension is the issue unless you're using some sort of an exotic linker.

@kristina, the library isn't being passed as an object file to the linker but rather as a library name. Hence, the .a suffix is not correct.

@kristina both bfd and lld are adding the .a extension when you pass a lib through -l. Similarly to -llibfoo won't work, -lfoo.a will lead the linker to look for libfoo.a.a

The library shouldn't be an object file (it could be if you changed some stuff around but typically isn't), libbuiltins is a combination of multiple translation units, if you look at the source. I'm not sure how it's turning into an object file for you and even if it was you can pass an archive to the driver on the commandline and it would treat it like a group of objects. What is the generated output you're getting when you build it for bare metal?

CodaFi added a comment.EditedSep 10 2018, 5:32 PM

@kristina They're having linker troubles because this driver is hardcoding an incorrect linker flag for libbuiltin on the arm-baremetal triple. D33259 introduced this behavior by smashing together the -lclang_rt.builtins-$ARCH flag with the proper resource-directory-relative absolute path <RESOURCE_DIR>/lib/libclang_rt.builtins-$ARCH.a which resulted in the incorrect flag -libclang_rt.builtins-$ARCH.a. With that path, the linker is going to try to look for "libclang_rt.builtins-$ARCH.a.a" in the link libraries search paths. The driver should be forming an exact path to this library relative to the resource directory. If users wish to override this behavior, they need to pass -nostdlib or -nodefaultlibs

Added the main ARM code owner as a reviewer, I'm not entirely sure why you're getting this behavior since regression tests should have weeded it out.

The regression tests are only against the text of the driver's cc1 commands. It's not actually testing if we can compile.

kristina added a comment.EditedSep 10 2018, 6:01 PM

Sorry, didn't notice this was recently added, from looking at Darwin and Android embedded tests, this doesn't really match up with either of them. The hosted tests for Android using libbuiltins look like this:

// RUN: %clang -target arm-linux-androideabi -rtlib=compiler-rt -### %s 2>&1 | FileCheck %s -check-prefix ARM-ANDROID
// ARM-ANDROID: "{{.*[/\\]}}libclang_rt.builtins-arm-android.a"

Not sure what to make of this to be honest, I don't think this should be passed in as a conventional library since the driver usually picks the correct runtime for a given target triple. So I take what I said back, it shouldn't be passed using any argument directly, the driver should figure it out and construct the correct flags for the linker. This doesn't seem like a correct way of fixing it either.

Also, for Mach-O and Darwin in general, ld64 is the default linker, anything built below that builds the object or the executable and then post-processes it using tools outside of the scope of LLVM, so it has exception vectors in the correct place etc. since ld64 lacks linker script support. And having checked, at least for Darwin, there's no linking against any compiler runtime library whatsoever when targeting "bare metal", the few division related functions you'd ship with the application. It varies from toolchain to toolchain, sometimes with Linaro toolchains you may get a very minimal libgcc for your target.

I'm really rusty on ARM/ARM64 stuff right now, but I remember how many hoops you had to jump through back then to use Clang/LLVM and ld64 as the native compilers/"linkers" for those targets. And while I realize you're using lld, fundamentally, having bare metal toolchains supplied for every ARM target is difficult.

This idea seems broken end to end especially given the minimum amount of functions you would generally export from a runtime library in a freestanding environment and usually just division related ones. If you'd like to use compiler-rt, wouldn't it make more sense to just import whatever you need from the actual codebase and use it directly in your application?

compnerd accepted this revision.Oct 22 2019, 10:21 PM

This is not a linker specific thing - the library name that we are passing is misnamed - the parameter should be -l <library name> which will become lib <library name> [.so` | .a]. If we want to guarantee the static library, using -static, -nostatic around the library is more appropriate.

This revision is now accepted and ready to land.Oct 22 2019, 10:21 PM
MaskRay added inline comments.
lib/Driver/ToolChains/BareMetal.cpp
159

Ideally this should call ToolChain::getCompilerRTArgString to get an absolute path.

@MaskRay I have a refactoring in https://reviews.llvm.org/D59425 that covers that.