This is an archive of the discontinued LLVM Phabricator instance.

Enable multilib.yaml in the BareMetal ToolChain
ClosedPublic

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
198–199

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
779

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

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

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

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

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

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.Mar 13 2023, 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.Mar 13 2023, 9:25 AM
abidh accepted this revision.Mar 13 2023, 3:13 PM

The BareMetal.cpp part looks ok to me.

phosek added inline comments.Mar 13 2023, 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.

phosek added inline comments.May 23 2023, 1:34 AM
clang/lib/Driver/ToolChains/BareMetal.cpp
180

Defer to llvm::sys::path::append for joining the path components to ensure that the right separator is used.

183

I see that this is a pre-existing behavior, but I find it a unfortunate that the BareMetal driver is using its own convention. I'd prefer using ../sys-root which the existing convention used by GCC. I'm wondering if this is something we can change as part of the transition to the new multilib implementation?

184

Relying on --sysroot is fine if you're assembling the sysroot yourself and control the content, but when dealing with an existing sysroot it may not be possible to add additional files in which case having the ability to specify the .yaml file may be essential. Having an option also gives more flexibility, for example I could imagine having more than one .yaml file for the same sysroot with different subsets of multilibs.

michaelplatings marked an inline comment as done.

Rebase

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

If I remember correctly @abidh did it this way specifically to avoid conflicting with the contents of GCC's sys-root directory.

184

Sounds good, let's do it in a later patch.

phosek added inline comments.Jun 6 2023, 10:52 PM
clang/lib/Driver/ToolChains/BareMetal.cpp
174

This is not idiomatic, I couldn't find any instance of #define for string literals anywhere in LLVM. I believe the idiomatic LLVM alternative is:

static constexpr llvm::StringLiteral MultilibFilename = "multilib.yaml";
michaelplatings marked an inline comment as done.

Change string literal as suggested by @phosek

phosek accepted this revision.Jun 8 2023, 12:39 AM

LGTM

This revision was landed with ongoing or failed builds.Jun 13 2023, 10:47 PM
This revision was automatically updated to reflect the committed changes.