[Driver] Recognize powerpc-unknown-eabi as a bare-metal toolchain

[cwalther]

Summary

At Indel, we have been building bare-metal embedded applications that run on custom PowerPC and ARM systems with Clang and LLD for a couple of years now, using target triples powerpc-indel-eabi, powerpc-indel-eabi750, arm-indel-eabi, aarch64-indel-eabi (which I just learned from D153430 is wrong and should be aarch64-indel-elf instead, but that’s a different matter). This has worked fine for ARM, but for PowerPC we have been unable to call the linker (LLD) through the Clang driver, because it would insist on calling GCC as the linker, even when told -fuse-ld=lld. That does not work for us, there is no GCC around. Instead we had to call ld.lld directly, introducing some special cases in our build system to translate between linker-via-driver and linker-called-directly command line arguments. I have now dug into why that is, and found that the difference between ARM and PowerPC is that arm-indel-eabi hits a special case that causes the Clang driver to instantiate a BareMetal toolchain that is able to call LLD and works the way we need, whereas powerpc-indel-eabi lands in the default case of a Generic_ELF (subclass of Generic_GCC) toolchain which expects GCC.

It seems to me that maybe anything with OS none (although that doesn’t seem to be distinguished from unknown) or with environment eabi should be treated as bare-metal, but I don’t have a lot of experience in this regard. Since this seems to have been handled on a case-by-case basis in the past (arm, riscv, aarch64), what I am proposing here is to add another case to the list to also handle powerpc[64][le]-unknown-unknown-eabi using the BareMetal toolchain, following the example of the existing cases. (We don’t care about powerpc64 and powerpc[64]le, but it seemed appropriate to lump them in.)

[jroelofs]

It seems to me that maybe anything with OS none (although that doesn’t seem to be distinguished from unknown)

I considered splitting none from unknown in Triple, but never had the time to do it. I think this would be good for baremetal toolchains in light of the gcc installation detection machinery taking over whenever there isn't otherwise a match.

or with environment eabi should be treated as bare-metal, but I don’t have a lot of experience in this regard.

IIRC there are arm-linux-eabi toolchains in the wild, which are not baremetal.

clang/lib/Driver/ToolChains/BareMetal.cpp

169

does this break your use cases re: having indel as the vendor? or will you have a downstream patch to exempt indel from this check?

[cwalther]

Should I be worried about the build and test failures? At a glance, they look unrelated to my changes, so unless told otherwise I’m going to ignore them.

clang/lib/Driver/ToolChains/BareMetal.cpp

169

No, indel is recognized as Triple::UnknownVendor, so this works for us. It would indeed stop working if Indel were added to enum VendorType (and the triple parsing functions), but I don’t expect that to happen.

[jroelofs]

I don't think those failures look related.

[MaskRay]

At Indel, we have been building bare-metal embedded applications that run on custom PowerPC and ARM systems with Clang and LLD for a couple of years now, using target triples powerpc-indel-eabi, powerpc-indel-eabi750, arm-indel-eabi, aarch64-indel-eabi (which I just learned from D153430 is wrong and should be aarch64-indel-elf instead, but that’s a different matter).

If eabi is not valid for powerpc, Clang Driver should not endorse this use case and we should not add positive tests for powerpc-unknown-eabi or powerpc64le-unknown-eabi.

[jroelofs]

gcc docs seem to indicate that these are valid triples:

https://gcc.gnu.org/install/specific.html#powerpc-x-eabi

[michaelplatings]

Hi @MaskRay, thanks for the add. Yes, we've been deleting a lot of eabi recently, but that's specific to AArch64. I have no particular insight into PowerPC but from @jroelofs' link, I agree eabi seems correct here.

I've been touching BareMetal.cpp a lot recently, and from that point of view it seems reasonable to have another architecture join the party. LGTM but I agree with MaskRay it would be good to give folks from the PowerPC group a chance to comment.

[MaskRay]

In D154357#4485881, @jroelofs wrote:

gcc docs seem to indicate that these are valid triples:

https://gcc.gnu.org/install/specific.html#powerpc-x-eabi

Thanks, good to know. powerpc-*-eabi indicates that the eabi environment desginates an embedded system, regardless of the vendor.
It seems that we should remove if (Triple.getVendor() != llvm::Triple::UnknownVendor).

[cwalther]

Thanks for the comments. Yes, powerpc-*-eabi should be valid (sorry if I was unclear about that – my parenthetical was only about AArch64). There is a PowerPC EABI: https://www.nxp.com/docs/en/application-note/PPCEABI.pdf . I wouldn’t know if Clang/LLD obey it, but it works for us…

I am fine with removing the vendor check if that is the consensus.

[MaskRay]

In D154357#4487896, @cwalther wrote:

Thanks for the comments. Yes, powerpc-*-eabi should be valid (sorry if I was unclear about that – my parenthetical was only about AArch64). There is a PowerPC EABI: https://www.nxp.com/docs/en/application-note/PPCEABI.pdf . I wouldn’t know if Clang/LLD obey it, but it works for us…

I am fine with removing the vendor check if that is the consensus.

Sounds good. I think && is cleaner: return Triple.getOS() == llvm::Triple::UnknownOS && ...;

[MaskRay]

In D154357#4487896, @cwalther wrote:

[...] There is a PowerPC EABI: https://www.nxp.com/docs/en/application-note/PPCEABI.pdf . I wouldn’t know if Clang/LLD obey it, but it works for us…

Good to know! I read a newer ABI named Power Architecture® 32-bit Application Binary Interface Supplement 1.0 - Linux® & Embedded which contains EABI as well but I forgot it after I implemented ld.lld PPC32 back in 2019 (D62464 and a few others) :)

[cwalther]

I think && is cleaner: return Triple.getOS() == llvm::Triple::UnknownOS && ...;

Totally agree. I was just following precedent there.

[cwalther]

Updated patch according to current discussion:

• remove check for vendor, any vendor is accepted
• replace cascaded if by &&
• rebase on main a3f9ce6

[nemanjai]

No concerns from the perspective of PowerPC here. Of course, my focus is primarily on the server side of things but I am not aware of any other group that could be adversely affected.

[cwalther]

Oops, forgot to run git-clang-format.

[MaskRay]

clang/test/Driver/baremetal.cpp

348

Without a sysroot, we may pick up powerpc-unknown-eabi-gcc (if installed) and the paths below may be incorrect.

[MaskRay]

The code change looks good but the driver test will cause an issue.
I think we need a fake sysroot tree under Inputs/.
Otherwise if we have a system cross gcc at /usr/lib/gcc{,-cross}/powerpc64le-unknown-eabi/12, Clang Driver will pick up these files.
This is really difficult to debug for someone not so familiar with Driver (I think I have done quite a lot of work, but still quite puzzled).
You may check many other tests using --sysroot, remove --sysroot, and check the behavior change.

[cwalther]

I am unable to provoke any problem with my tests by putting various things in /usr/lib/gcc/, and I also can’t find any code that would go looking there on the code path that is exercised by these tests. Can you give me a hint where that code is, so I can try harder to provoke it? Is it outside of Generic_GCC::GCCInstallationDetector? GCCInstallationDetector is not used on the code path taken for --target=powerpc[64][le]-unknown-eabi (after my patch), as far as I can tell. It’s only used by Generic_GCC and subclasses, getting away from which is the whole point of my patch.

Or are you suggesting the addition of --sysroot just to prevent potential future problems?

[MaskRay]

In D154357#4492625, @cwalther wrote:

I am unable to provoke any problem with my tests by putting various things in /usr/lib/gcc/, and I also can’t find any code that would go looking there on the code path that is exercised by these tests. Can you give me a hint where that code is, so I can try harder to provoke it? Is it outside of Generic_GCC::GCCInstallationDetector? GCCInstallationDetector is not used on the code path taken for --target=powerpc[64][le]-unknown-eabi (after my patch), as far as I can tell. It’s only used by Generic_GCC and subclasses, getting away from which is the whole point of my patch.

Or are you suggesting the addition of --sysroot just to prevent potential future problems?

Sorry, I misremembered. baremetal doesn't use GCC detection.
If you use a Linux target triple, you may notice a lot of failed GCC directory openat syscalls: strace -e file --status=failed /tmp/Rel/bin/clang --target=XXX -c -fdriver-only -xc /dev/null

I have run the check-clang tests and the patch didn’t break any of them, which makes me hopeful. Or do you think that this may break anyone’s usage? Should we additionally check for the presence of a --gcc-toolchain argument like in the RISC-V case? I am new here, so any guidance is appreciated.

This part of the summary/commit message should be removed before committing.

Specifying --gcc-toolchain= is for Generic_GCC::GCCInstallationDetector::init. RISCVToolChain::RISCVToolChain calls this function, other baremetal targets don't.
If we don't call Generic_GCC::GCCInstallationDetector::init, --gcc-toolchain= is not checked and will lead to an unused warning.

[cwalther]

That summary wasn’t intended to be a commit message at all, but a description of the problem and start of the discussion. My commit message was part of the uploaded patch (since I generated it using git show) and said

[Driver] Recognize powerpc-*-unknown-eabi as a bare-metal toolchain

Calling GCC as the linker as the Generic_ELF toolchain does is
inappropriate for an all-LLVM embedded toolchain.

but it looks like Phabricator doesn’t show that anywhere.

I haven’t user Phabricator before, so to make sure I am understanding you correctly: You want me to edit the “summary” on this page, and then when you commit the patch, it will automatically use that as the commit message? That still seems much too verbose for a commit message to me even after removing that last paragraph, but if you prefer that to the terse message above, that’s fine with me.

[cwalther]

I have gone ahead and removed the last paragraph of the summary, assuming that was what was requested. Happy to make other edits.

[MaskRay]

In D154357#4496247, @cwalther wrote:

That summary wasn’t intended to be a commit message at all, but a description of the problem and start of the discussion. My commit message was part of the uploaded patch (since I generated it using git show) and said

[Driver] Recognize powerpc-*-unknown-eabi as a bare-metal toolchain

Calling GCC as the linker as the Generic_ELF toolchain does is
inappropriate for an all-LLVM embedded toolchain.

but it looks like Phabricator doesn’t show that anywhere.

I haven’t user Phabricator before, so to make sure I am understanding you correctly: You want me to edit the “summary” on this page, and then when you commit the patch, it will automatically use that as the commit message? That still seems much too verbose for a commit message to me even after removing that last paragraph, but if you prefer that to the terse message above, that’s fine with me.

Yes, edit the summary. You have done that. Thanks!

If you have write access to the llvm-project repository, you may adjust commit message before committing.
If you don't and ask someone else to land the patch for you, they may use arc diff D154357 to fetch the patch, whose commit message will contain the Differential summary.

[cwalther]

Okay. No, I don’t have write access (as far as I know) and would appreciate if you (or any other maintainer) could land the patch, with any commit message you deem appropriate.

[MaskRay]

In D154357#4497973, @cwalther wrote:

Okay. No, I don’t have write access (as far as I know) and would appreciate if you (or any other maintainer) could land the patch, with any commit message you deem appropriate.

Happy to do this! What git commit --amend --author=... shall I use to set the author name/email for the commit?

[cwalther]

Christian Walther <[e-mail]>

[cwalther]

Thank you!

So I guess next time I submit a patch here (nothing planned at the moment), I should put the proposed commit message into the summary field and the backstory and start of discussion / open questions into a first comment. But maybe the point is moot as I understand you are about to move to GitHub pull requests, which I am more familiar with.

[MaskRay]

In D154357#4500241, @cwalther wrote:

Thank you!

So I guess next time I submit a patch here (nothing planned at the moment), I should put the proposed commit message into the summary field and the backstory and start of discussion / open questions into a first comment. But maybe the point is moot as I understand you are about to move to GitHub pull requests, which I am more familiar with.

Yeah, you can place the commit message in the summary field and add supplementary information as a separate comment.

[MaskRay]

clang/test/Driver/baremetal.cpp

348

I managed to notice an issue. See the commit message of the follow-up fix: eddc4850d81f4c9f79d0b17869d67eac8ca88070 (really obscure, I forgot it...)

[cwalther]

clang/test/Driver/baremetal.cpp

348

I see. Thanks for the fix!

clang/lib/Driver/ToolChains/BareMetal.cpp

if (Triple.getVendor() != llvm::Triple::UnknownVendor)
     return false;
 
   if (Triple.getOS() != llvm::Triple::UnknownOS)
     return false;
 
   return Triple.getEnvironmentName() == "elf";
 }
 
+/// Is the triple powerpc[64][le]-*-none-eabi?
+static bool isPPCBareMetal(const llvm::Triple &Triple) {
+  return Triple.isPPC() && Triple.getOS() == llvm::Triple::UnknownOS &&
+         Triple.getEnvironment() == llvm::Triple::EABI;
+}
+
 static void findMultilibsFromYAML(const ToolChain &TC, const Driver &D,
                                   StringRef MultilibPath, const ArgList &Args,
                                   DetectedMultilibs &Result) {
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> MB =
       D.getVFS().getBufferForFile(MultilibPath);
   if (!MB)
     return;
   Multilib::flags_list Flags = TC.getMultilibFlags(Args);
if (isRISCVBareMetal(Triple)) {
     findMultilibsFromYAML(*this, D, MultilibPath, Args, Result);
     SelectedMultilibs = Result.SelectedMultilibs;
     Multilibs = Result.Multilibs;
   }
 }
 
 bool BareMetal::handlesTarget(const llvm::Triple &Triple) {
   return isARMBareMetal(Triple) || isAArch64BareMetal(Triple) ||
-         isRISCVBareMetal(Triple);
+         isRISCVBareMetal(Triple) || isPPCBareMetal(Triple);
 }
 
 Tool *BareMetal::buildLinker() const {
   return new tools::baremetal::Linker(*this);
 }
 
 Tool *BareMetal::buildStaticLibTool() const {
   return new tools::baremetal::StaticLibTool(*this);

clang/test/Driver/baremetal.cpp

 // CHECK-RV32IMAFC-SAME: "-isysroot" "[[SYSROOT:[^"]*]]"
 // CHECK-RV32IMAFC-SAME: "-internal-isystem" "[[SYSROOT]]{{[/\\]+}}rv32imafc{{[/\\]+}}ilp32f{{[/\\]+}}include{{[/\\]+}}c++{{[/\\]+}}v1"
 // CHECK-RV32IMAFC-SAME: "-internal-isystem" "[[SYSROOT]]{{[/\\]+}}rv32imafc{{[/\\]+}}ilp32f{{[/\\]+}}include"
 // CHECK-RV32IMAFC-SAME: "-x" "c++" "{{.*}}baremetal.cpp"
 // CHECK-RV32IMAFC-NEXT: ld{{(.exe)?}}" "{{.*}}.o" "-Bstatic"
 // CHECK-RV32IMAFC-SAME: "-L[[SYSROOT:[^"]+]]{{[/\\]+}}rv32imafc{{[/\\]+}}ilp32f{{[/\\]+}}lib"
 // CHECK-RV32IMAFC-SAME: "-L[[RESOURCE_DIR:[^"]+]]{{[/\\]+}}lib{{[/\\]+}}baremetal{{[/\\]+}}rv32imafc{{[/\\]+}}ilp32f"
 
+// RUN: %clang %s -### --target=powerpc-unknown-eabi 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-PPCEABI %s
+// CHECK-PPCEABI: InstalledDir: [[INSTALLEDDIR:.+]]
+// CHECK-PPCEABI: "-nostdsysteminc"
+// CHECK-PPCEABI-SAME: "-resource-dir" "[[RESOURCE:[^"]+]]"
+// CHECK-PPCEABI-SAME: "-internal-isystem" "[[INSTALLEDDIR]]{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+[^"]*}}include{{[/\\]+}}c++{{[/\\]+}}v1"
+// CHECK-PPCEABI-SAME: "-internal-isystem" "[[RESOURCE]]{{[/\\]+}}include"
+// CHECK-PPCEABI-SAME: "-internal-isystem" "[[INSTALLEDDIR]]{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+[^"]*}}include"
+// CHECK-PPCEABI-NEXT: ld{{(.exe)?}}" "{{.*}}.o" "-Bstatic"
+// CHECK-PPCEABI-SAME: "-L[[INSTALLEDDIR]]{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+[^"]*}}lib"
+// CHECK-PPCEABI-SAME: "-L[[RESOURCE]]{{[/\\]+}}lib{{[/\\]+}}baremetal"
+// CHECK-PPCEABI-SAME: "-lc" "-lm" "-lclang_rt.builtins-powerpc" "-o" "a.out"
+
+// RUN: %clang %s -### --target=powerpc64-unknown-eabi 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-PPC64EABI %s
+// CHECK-PPC64EABI: InstalledDir: [[INSTALLEDDIR:.+]]
+// CHECK-PPC64EABI: "-nostdsysteminc"
+// CHECK-PPC64EABI-SAME: "-resource-dir" "[[RESOURCE:[^"]+]]"
+// CHECK-PPC64EABI-SAME: "-internal-isystem" "[[INSTALLEDDIR]]{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+[^"]*}}include{{[/\\]+}}c++{{[/\\]+}}v1"
+// CHECK-PPC64EABI-SAME: "-internal-isystem" "[[RESOURCE]]{{[/\\]+}}include"
+// CHECK-PPC64EABI-SAME: "-internal-isystem" "[[INSTALLEDDIR]]{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+[^"]*}}include"
+// CHECK-PPC64EABI-NEXT: ld{{(.exe)?}}" "{{.*}}.o" "-Bstatic"
+// CHECK-PPC64EABI-SAME: "-L[[INSTALLEDDIR]]{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+[^"]*}}lib"
+// CHECK-PPC64EABI-SAME: "-L[[RESOURCE]]{{[/\\]+}}lib{{[/\\]+}}baremetal"
+// CHECK-PPC64EABI-SAME: "-lc" "-lm" "-lclang_rt.builtins-powerpc64" "-o" "a.out"
+
+// RUN: %clang %s -### --target=powerpcle-unknown-eabi 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-PPCLEEABI %s
+// CHECK-PPCLEEABI: InstalledDir: [[INSTALLEDDIR:.+]]
+// CHECK-PPCLEEABI: "-nostdsysteminc"
+// CHECK-PPCLEEABI-SAME: "-resource-dir" "[[RESOURCE:[^"]+]]"
+// CHECK-PPCLEEABI-SAME: "-internal-isystem" "[[INSTALLEDDIR]]{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+[^"]*}}include{{[/\\]+}}c++{{[/\\]+}}v1"
+// CHECK-PPCLEEABI-SAME: "-internal-isystem" "[[RESOURCE]]{{[/\\]+}}include"
+// CHECK-PPCLEEABI-SAME: "-internal-isystem" "[[INSTALLEDDIR]]{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+[^"]*}}include"
+// CHECK-PPCLEEABI-NEXT: ld{{(.exe)?}}" "{{.*}}.o" "-Bstatic"
+// CHECK-PPCLEEABI-SAME: "-L[[INSTALLEDDIR]]{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+[^"]*}}lib"
+// CHECK-PPCLEEABI-SAME: "-L[[RESOURCE]]{{[/\\]+}}lib{{[/\\]+}}baremetal"
+// CHECK-PPCLEEABI-SAME: "-lc" "-lm" "-lclang_rt.builtins-powerpcle" "-o" "a.out"
+
+// RUN: %clang %s -### --target=powerpc64le-unknown-eabi 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-PPC64LEEABI %s
+// CHECK-PPC64LEEABI: InstalledDir: [[INSTALLEDDIR:.+]]
+// CHECK-PPC64LEEABI: "-nostdsysteminc"
+// CHECK-PPC64LEEABI-SAME: "-resource-dir" "[[RESOURCE:[^"]+]]"
+// CHECK-PPC64LEEABI-SAME: "-internal-isystem" "[[INSTALLEDDIR]]{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+[^"]*}}include{{[/\\]+}}c++{{[/\\]+}}v1"
+// CHECK-PPC64LEEABI-SAME: "-internal-isystem" "[[RESOURCE]]{{[/\\]+}}include"
+// CHECK-PPC64LEEABI-SAME: "-internal-isystem" "[[INSTALLEDDIR]]{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+[^"]*}}include"
+// CHECK-PPC64LEEABI-NEXT: ld{{(.exe)?}}" "{{.*}}.o" "-Bstatic"
+// CHECK-PPC64LEEABI-SAME: "-L[[INSTALLEDDIR]]{{[/\\]+}}..{{[/\\]+}}lib{{[/\\]+}}clang-runtimes{{[/\\]+[^"]*}}lib"
+// CHECK-PPC64LEEABI-SAME: "-L[[RESOURCE]]{{[/\\]+}}lib{{[/\\]+}}baremetal"
+// CHECK-PPC64LEEABI-SAME: "-lc" "-lm" "-lclang_rt.builtins-powerpc64le" "-o" "a.out"
+
 // Check that compiler-rt library without the arch filename suffix will
 // be used if present.
 // RUN: rm -rf %T/baremetal_clang_rt_noarch
 // RUN: mkdir -p %T/baremetal_clang_rt_noarch/lib
 // RUN: touch %T/baremetal_clang_rt_noarch/lib/libclang_rt.builtins.a
 // RUN: %clang %s -### 2>&1 \
 // RUN:     --target=armv6m-none-eabi \
 // RUN:     --sysroot=%T/baremetal_clang_rt_noarch \