Page MenuHomePhabricator

Enable multilib.yaml in the BareMetal ToolChain
AcceptedPublic

Authored by michaelplatings on Jan 31 2023, 7:59 AM.

Details

Summary

The default location for multilib.yaml is lib/clang-runtimes, without
any target-specific suffix. This will allow multilibs for different
architectures to share a common include directory.

To avoid breaking the arm-execute-only.c CHECK-NO-EXECUTE-ONLY-ASM
test, add a GetExecuteOnly argument to getARMTargetFeatures.

Since the presence of multilib.yaml can change the exact location of a
library, relax the baremetal.cpp test.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 7:59 AM
michaelplatings requested review of this revision.Jan 31 2023, 7:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 7:59 AM
bsmith added a subscriber: bsmith.Feb 1 2023, 2:16 AM

Looks good to me. Some small suggestions around execute-only as I expect this to be a useful multilib variant.

clang/lib/Driver/ToolChain.cpp
194–196

Now that there are two boolean parameters, could you put a prefix comment to show which one is which for example:
/* ForAs */ false, /* GenExecuteOnly */

clang/lib/Driver/ToolChains/Arch/ARM.cpp
790–791

I can see execute-only being used in a multilib configuration as it can impact code-generation significantly yet is a requirement for a system aiming to have all code execute-only.

Rather than a specific GenExecuteOnly flag, would it be better for something like ForMultilib like ForAS and then we could move it to not give the error message when doing ForMultilib the name would also generalise to other parts.

Add clangMinimumVersion

MaskRay added inline comments.Feb 10 2023, 10:47 PM
clang/lib/Driver/ToolChains/BareMetal.cpp
167

delete blank line

169
193
clang/test/Driver/baremetal-multilib.cpp
1 ↗(On Diff #496537)

This C++ source file is empty excluding comments. Just name it baremetal-multilib.yaml.

Note: if you need extra files, check out split-file

14 ↗(On Diff #496537)

With -cc1 CHECK lines, remove clang{{.*}} before -cc1, then remove -no-canonical-prefixes. See my prior commits in this area.

MaskRay added inline comments.Feb 10 2023, 11:06 PM
clang/test/Driver/baremetal-multilib.cpp
19 ↗(On Diff #496537)

Use a style similar to linux-cross.cpp.

Test a few variants (https://github.com/MaskRay/Config/wiki/LLVM#clanglibdriver)

michaelplatings marked 6 inline comments as done.

Apply changes requested by @peter.smith and @MaskRay

clang/test/Driver/baremetal-multilib.cpp
14 ↗(On Diff #496537)

I found your commit 980679981fbc311bc07f8cd23e3739fd56c22d2a which removes many uses of -no-canonical-prefixes. However in this case the test relies on -no-canonical-prefixes to ensure that clang finds the correct lib/clang-runtimes directory so it's necessary to keep it.

I have removed clang{{.*}} which was a copy-paste from baremetal-sysroot.cpp and applied similar tweaks like your tidy-up to ld.

19 ↗(On Diff #496537)

Use a style similar to linux-cross.cpp.

I think you're referring to the use of -L[[SYSROOT]] instead of -L{{.*}} and I've made that change.

Test a few variants (https://github.com/MaskRay/Config/wiki/LLVM#clanglibdriver)

The test follows the same pattern as baremetal-sysroot.cpp so in theory if that works on all the variants then so should this.

PrintArgs -> PrintOptions

flags -> tags

peter.smith accepted this revision.Mon, Mar 13, 9:25 AM

I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions.

This revision is now accepted and ready to land.Mon, Mar 13, 9:25 AM
abidh accepted this revision.Mon, Mar 13, 3:13 PM

The BareMetal.cpp part looks ok to me.

phosek added inline comments.Mon, Mar 13, 9:43 PM
clang/lib/Driver/ToolChains/BareMetal.cpp
184

Rather than hardcoding the filename and the location, which is inflexible, could we instead provide a command line option to specify the file to use?

Rebase

clang/lib/Driver/ToolChains/BareMetal.cpp
184

I'm not opposed to that in principle but I'd rather leave that as an option for a future change. At this point, for LLVM Embedded Toolchain for Arm our intent is that users will specify --sysroot if they want to use a separate set of multilibs.