Skip to content

[lld] Fix ILP32 ABI checks for bitcode files. #116537

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexrp
Copy link
Member

@alexrp alexrp commented Nov 17, 2024

Previously, using LTO with the MIPS N32 ABI and (e.g.) -m elf32btsmipn32 would fail because isN32Abi() made no effort to check whether a bitcode file is using the N32 ABI. Additionally, getBitcodeELFKind() would incorrectly pick 64-bit ELF for all ILP32 ABIs (not just N32).

There's probably still more work to be done for LTO to fully work for these ABIs, but this should hopefully be an uncontroversial step in the right direction.

Copy link

github-actions bot commented Nov 17, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

alexrp added a commit to alexrp/zig that referenced this pull request Nov 17, 2024
@alexrp alexrp marked this pull request as ready for review November 17, 2024 10:03
@llvmbot
Copy link
Member

llvmbot commented Nov 17, 2024

@llvm/pr-subscribers-lld

Author: Alex Rønne Petersen (alexrp)

Changes

Previously, using LTO with the MIPS N32 ABI and (e.g.) -m elf32btsmipn32 would fail because isN32Abi() made no effort to check whether a bitcode file is using the N32 ABI. Additionally, getBitcodeELFKind() would incorrectly pick 64-bit ELF for all ILP32 ABIs (not just N32).

There's probably still more work to be done for LTO to fully work for these ABIs, but this should hopefully be an uncontroversial step in the right direction.

Note: I don't have commit access.


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

3 Files Affected:

  • (modified) lld/ELF/Arch/MipsArchTree.cpp (+2)
  • (modified) lld/ELF/InputFiles.cpp (+9-7)
  • (modified) lld/ELF/InputFiles.h (+3)
diff --git a/lld/ELF/Arch/MipsArchTree.cpp b/lld/ELF/Arch/MipsArchTree.cpp
index 0c64a46fe85d08..e7d3d591e3b27b 100644
--- a/lld/ELF/Arch/MipsArchTree.cpp
+++ b/lld/ELF/Arch/MipsArchTree.cpp
@@ -365,6 +365,8 @@ uint8_t elf::getMipsFpAbiFlag(Ctx &ctx, uint8_t oldFlag, uint8_t newFlag,
 template <class ELFT> static bool isN32Abi(const InputFile &f) {
   if (auto *ef = dyn_cast<ELFFileBase>(&f))
     return ef->template getObj<ELFT>().getHeader().e_flags & EF_MIPS_ABI2;
+  if (auto *bc = dyn_cast<BitcodeFile>(&f))
+    return bc->triple.isABIN32();
   return false;
 }
 
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 769081fa71a449..d6f55603be6791 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1636,9 +1636,10 @@ template <class ELFT> void SharedFile::parse() {
 }
 
 static ELFKind getBitcodeELFKind(const Triple &t) {
-  if (t.isLittleEndian())
-    return t.isArch64Bit() ? ELF64LEKind : ELF32LEKind;
-  return t.isArch64Bit() ? ELF64BEKind : ELF32BEKind;
+  if (t.isArch64Bit() && !t.isABIN32() && !t.isX32() &&
+      t.getEnvironment() != Triple::GNUILP32)
+    return t.isLittleEndian() ? ELF64LEKind : ELF64BEKind;
+  return t.isLittleEndian() ? ELF32LEKind : ELF32BEKind;
 }
 
 static uint16_t getBitcodeMachineKind(Ctx &ctx, StringRef path,
@@ -1731,10 +1732,11 @@ BitcodeFile::BitcodeFile(Ctx &ctx, MemoryBufferRef mb, StringRef archiveName,
 
   obj = CHECK(lto::InputFile::create(mbref), this);
 
-  Triple t(obj->getTargetTriple());
-  ekind = getBitcodeELFKind(t);
-  emachine = getBitcodeMachineKind(ctx, mb.getBufferIdentifier(), t);
-  osabi = getOsAbi(t);
+  this->triple = Triple(obj->getTargetTriple());
+
+  ekind = getBitcodeELFKind(this->triple);
+  emachine = getBitcodeMachineKind(ctx, mb.getBufferIdentifier(), this->triple);
+  osabi = getOsAbi(this->triple);
 }
 
 static uint8_t mapVisibility(GlobalValue::VisibilityTypes gvVisibility) {
diff --git a/lld/ELF/InputFiles.h b/lld/ELF/InputFiles.h
index e00e2c83d017c2..33bc26cdf9e3a5 100644
--- a/lld/ELF/InputFiles.h
+++ b/lld/ELF/InputFiles.h
@@ -19,6 +19,7 @@
 #include "llvm/Object/ELF.h"
 #include "llvm/Support/MemoryBufferRef.h"
 #include "llvm/Support/Threading.h"
+#include "llvm/TargetParser/Triple.h"
 
 namespace llvm {
 struct DILineInfo;
@@ -335,6 +336,8 @@ class BitcodeFile : public InputFile {
   void postParse();
   std::unique_ptr<llvm::lto::InputFile> obj;
   std::vector<bool> keptComdats;
+
+  llvm::Triple triple;
 };
 
 // .so file.

@llvmbot
Copy link
Member

llvmbot commented Nov 17, 2024

@llvm/pr-subscribers-lld-elf

Author: Alex Rønne Petersen (alexrp)

Changes

Previously, using LTO with the MIPS N32 ABI and (e.g.) -m elf32btsmipn32 would fail because isN32Abi() made no effort to check whether a bitcode file is using the N32 ABI. Additionally, getBitcodeELFKind() would incorrectly pick 64-bit ELF for all ILP32 ABIs (not just N32).

There's probably still more work to be done for LTO to fully work for these ABIs, but this should hopefully be an uncontroversial step in the right direction.

Note: I don't have commit access.


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

3 Files Affected:

  • (modified) lld/ELF/Arch/MipsArchTree.cpp (+2)
  • (modified) lld/ELF/InputFiles.cpp (+9-7)
  • (modified) lld/ELF/InputFiles.h (+3)
diff --git a/lld/ELF/Arch/MipsArchTree.cpp b/lld/ELF/Arch/MipsArchTree.cpp
index 0c64a46fe85d08..e7d3d591e3b27b 100644
--- a/lld/ELF/Arch/MipsArchTree.cpp
+++ b/lld/ELF/Arch/MipsArchTree.cpp
@@ -365,6 +365,8 @@ uint8_t elf::getMipsFpAbiFlag(Ctx &ctx, uint8_t oldFlag, uint8_t newFlag,
 template <class ELFT> static bool isN32Abi(const InputFile &f) {
   if (auto *ef = dyn_cast<ELFFileBase>(&f))
     return ef->template getObj<ELFT>().getHeader().e_flags & EF_MIPS_ABI2;
+  if (auto *bc = dyn_cast<BitcodeFile>(&f))
+    return bc->triple.isABIN32();
   return false;
 }
 
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 769081fa71a449..d6f55603be6791 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1636,9 +1636,10 @@ template <class ELFT> void SharedFile::parse() {
 }
 
 static ELFKind getBitcodeELFKind(const Triple &t) {
-  if (t.isLittleEndian())
-    return t.isArch64Bit() ? ELF64LEKind : ELF32LEKind;
-  return t.isArch64Bit() ? ELF64BEKind : ELF32BEKind;
+  if (t.isArch64Bit() && !t.isABIN32() && !t.isX32() &&
+      t.getEnvironment() != Triple::GNUILP32)
+    return t.isLittleEndian() ? ELF64LEKind : ELF64BEKind;
+  return t.isLittleEndian() ? ELF32LEKind : ELF32BEKind;
 }
 
 static uint16_t getBitcodeMachineKind(Ctx &ctx, StringRef path,
@@ -1731,10 +1732,11 @@ BitcodeFile::BitcodeFile(Ctx &ctx, MemoryBufferRef mb, StringRef archiveName,
 
   obj = CHECK(lto::InputFile::create(mbref), this);
 
-  Triple t(obj->getTargetTriple());
-  ekind = getBitcodeELFKind(t);
-  emachine = getBitcodeMachineKind(ctx, mb.getBufferIdentifier(), t);
-  osabi = getOsAbi(t);
+  this->triple = Triple(obj->getTargetTriple());
+
+  ekind = getBitcodeELFKind(this->triple);
+  emachine = getBitcodeMachineKind(ctx, mb.getBufferIdentifier(), this->triple);
+  osabi = getOsAbi(this->triple);
 }
 
 static uint8_t mapVisibility(GlobalValue::VisibilityTypes gvVisibility) {
diff --git a/lld/ELF/InputFiles.h b/lld/ELF/InputFiles.h
index e00e2c83d017c2..33bc26cdf9e3a5 100644
--- a/lld/ELF/InputFiles.h
+++ b/lld/ELF/InputFiles.h
@@ -19,6 +19,7 @@
 #include "llvm/Object/ELF.h"
 #include "llvm/Support/MemoryBufferRef.h"
 #include "llvm/Support/Threading.h"
+#include "llvm/TargetParser/Triple.h"
 
 namespace llvm {
 struct DILineInfo;
@@ -335,6 +336,8 @@ class BitcodeFile : public InputFile {
   void postParse();
   std::unique_ptr<llvm::lto::InputFile> obj;
   std::vector<bool> keptComdats;
+
+  llvm::Triple triple;
 };
 
 // .so file.

alexrp added a commit to alexrp/zig that referenced this pull request Nov 17, 2024
alexrp added a commit to ziglang/zig that referenced this pull request Nov 17, 2024
@alexrp alexrp force-pushed the lld-ilp32-checks branch 3 times, most recently from 066b9c8 to 85cd568 Compare November 24, 2024 17:48
Previously, using LTO with the MIPS N32 ABI and (e.g.) -m elf32btsmipn32 would
fail because isN32Abi() made no effort to check whether a bitcode file is using
the N32 ABI. Additionally, getBitcodeELFKind() would incorrectly pick 64-bit ELF
for all ILP32 ABIs (not just N32).
@alexrp
Copy link
Member Author

alexrp commented Dec 5, 2024

ping

@brad0
Copy link
Contributor

brad0 commented Dec 15, 2024

@MaskRay

if (t.isLittleEndian())
return t.isArch64Bit() ? ELF64LEKind : ELF32LEKind;
return t.isArch64Bit() ? ELF64BEKind : ELF32BEKind;
if (t.isArch64Bit() && !t.isABIN32() && !t.isX32() &&
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test to cover every condition here? lld/ELF has very rigid test coverage requirement to ensure refactoring would not regress.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you mind pointing me in the right direction for how the test should be done? I'm not as familiar with LLD's testing infrastructure as LLVM's.

Copy link
Member

Choose a reason for hiding this comment

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

The tests are in lld/test/ELF. Many LTO tests are in lld/test/ELF/lto. Run them with ninja check-lld-elf.

lld/test/ELF/lto/aarch64.ll may be useful.

@@ -337,6 +338,8 @@ class BitcodeFile : public InputFile {
void postParse();
std::unique_ptr<llvm::lto::InputFile> obj;
std::vector<bool> keptComdats;

llvm::Triple triple;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding a Triple and an llvm/TargetParser include to this header file, you can compute and cache the getBitcodeELFKind value instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants