Skip to content

[clang] add llvm abi support #121123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

RossComputerGuy
Copy link
Member

This PR makes it possible to use the LLVM ABI which uses LLVM libc.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Dec 26, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 26, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Tristan Ross (RossComputerGuy)

Changes

This PR makes it possible to use the LLVM ABI which uses LLVM libc.


Full diff: https://github.com/llvm/llvm-project/pull/121123.diff

2 Files Affected:

  • (modified) clang/lib/Driver/Driver.cpp (+1-1)
  • (modified) clang/lib/Driver/ToolChains/Gnu.cpp (+1-1)
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index dc84c1b9d1cc4e..98b8e6e97419c6 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1531,7 +1531,7 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
 
   // Check if the environment version is valid except wasm case.
   llvm::Triple Triple = TC.getTriple();
-  if (!Triple.isWasm()) {
+  if (!Triple.isWasm() && Triple.getEnvironment() != llvm::Triple::LLVM) {
     StringRef TripleVersionName = Triple.getEnvironmentVersionString();
     StringRef TripleObjectFormat =
         Triple.getObjectFormatTypeName(Triple.getObjectFormat());
diff --git a/clang/lib/Driver/ToolChains/Gnu.cpp b/clang/lib/Driver/ToolChains/Gnu.cpp
index 8397f1121ec2ce..2c9539ef56683c 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -464,7 +464,7 @@ void tools::gnutools::Linker::ConstructJob(Compilation &C, const JobAction &JA,
       if (!Args.hasArg(options::OPT_shared)) {
         if (Args.hasArg(options::OPT_pg))
           crt1 = "gcrt1.o";
-        else if (IsPIE)
+        else if (IsPIE && Triple.getEnvironment() != llvm::Triple::LLVM)
           crt1 = "Scrt1.o";
         else if (IsStaticPIE)
           crt1 = "rcrt1.o";

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests.

@@ -1531,7 +1531,7 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {

// Check if the environment version is valid except wasm case.
llvm::Triple Triple = TC.getTriple();
if (!Triple.isWasm()) {
if (!Triple.isWasm() && Triple.getEnvironment() != llvm::Triple::LLVM) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong to me. Opting out of here makes me think there is something wrong with how the LLVM environment was implemented in the triple.

The Wasm case is special because (apparently) it is supported to pass arbitrary non-environment values in the triple (see discussion: #78655 (comment)).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I was running into was I'd get invalid version error. My assumption was that the LLVM ABI would not have a version because that would be the version of the entire LLVM stack. I didn't see any other ways when grepping the source that could disable the error.

@MaskRay
Copy link
Member

MaskRay commented Jan 7, 2025

NAK. While llvm libc can be used an overlay, it is not complete for standalone Linux server/desktop usage. Introducing a target triple has large ecosystem implication and should not be taken lightly.

@MaskRay MaskRay closed this Jan 7, 2025
@RossComputerGuy
Copy link
Member Author

Why close? LLVM libc is maturing and one of the goals I had was to try using it in Nixpkgs to build whatever I can. LLVM libc doesn't work well in an overlay mode so we have to use full builds. It's true that a lot of functions are still missing or not implemented but by having the ability to properly specify it can make it easier to find what to do next. Also, from what I can tell Clang and LLVM itself have already gotten some support for the ABI. This PR simply unblocks 1 error which makes the ABI usable.

@jhuber6
Copy link
Contributor

jhuber6 commented Jan 7, 2025

You can compile and run static C applications, I'm not sure what the threshold is before we can start adding support to make it usable in clang.

@carlocab
Copy link
Member

carlocab commented Jan 7, 2025

Introducing a target triple has large ecosystem implication and should not be taken lightly.

To be fair this patch doesn't introduce a target triple, it only makes an existing one usable in Clang.

That said, I tend to agree with the review at #121123 (comment) that this doesn't seem like the right approach.

@RossComputerGuy
Copy link
Member Author

That said, I tend to agree with the review at #121123 (comment) that this doesn't seem like the right approach.

Yeah I agree too, I'm not sure what the right approach is but that's one of the reasons why PR's get reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants