[lld][ARM] Fix assertion when mixing ARM and Thumb objects #101985
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, we selected the Thumb2 PLT sequences if any input object is marked as not supporting the ARM ISA, which then causes assertion failures when calls from ARM code are seen. I think the intention here was to only use Thumb PLTs when the target does not have the ARM ISA available, signalled by no objects being marked as having it avaiable. To do that we need to track which ISAs we have seen as we parse the build attributes, and defer the decision about PLTs until all input objects have been parsed. This bug was triggered by real code in picolibc, which have some versions of string.h functions built with Thumb2-only build attributes, so that they are compatible with v7-A, v7-R and v7-M. Fixes llvm#99008
|
@llvm/pr-subscribers-lld-elf Author: Oliver Stannard (ostannard) ChangesPreviously, we selected the Thumb2 PLT sequences if any input object is marked as not supporting the ARM ISA, which then causes assertion failures when calls from ARM code in other objects are seen. I think the intention here was to only use Thumb PLTs when the target does not have the ARM ISA available, signalled by no objects being marked as having it available. To do that we need to track which ISAs we have seen as we parse the build attributes, and defer the decision about PLTs until all input objects have been parsed. This bug was triggered by real code in picolibc, which have some versions of string.h functions built with Thumb2-only build attributes, so that they are compatible with v7-A, v7-R and v7-M. Fixes #99008. Full diff: https://github.com/llvm/llvm-project/pull/101985.diff 4 Files Affected:
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index 183dc88a93e2f..d3ea095940cd8 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -218,6 +218,8 @@ struct Config {
bool allowMultipleDefinition;
bool fatLTOObjects;
bool androidPackDynRelocs = false;
+ bool armHasArmISA = false;
+ bool armHasThumb2ISA = false;
bool armThumbPLTs = false;
bool armHasBlx = false;
bool armHasMovtMovw = false;
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index a8c52e8c2b8e1..1928648a7c115 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -82,6 +82,7 @@ ConfigWrapper elf::config;
Ctx elf::ctx;
static void setConfigs(opt::InputArgList &args);
+static void setConfigsPostParse();
static void readConfigs(opt::InputArgList &args);
void elf::errorOrWarn(const Twine &msg) {
@@ -1886,6 +1887,14 @@ static void setConfigs(opt::InputArgList &args) {
}
}
+// Initialise config options which depend on information previously parsed from
+// input files, such as Arm build attributes.
+static void setConfigsPostParse() {
+ // Currently, Thumb PLTs require Thumb2, and are only used for targets which
+ // do not have the ARM ISA.
+ config->armThumbPLTs = config->armHasThumb2ISA && !config->armHasArmISA;
+}
+
static bool isFormatBinary(StringRef s) {
if (s == "binary")
return true;
@@ -2897,6 +2906,8 @@ template <class ELFT> void LinkerDriver::link(opt::InputArgList &args) {
// No more lazy bitcode can be extracted at this point. Do post parse work
// like checking duplicate symbols.
+ setConfigsPostParse();
+
parallelForEach(ctx.objectFiles, [](ELFFileBase *file) {
initSectionsAndLocalSyms(file, /*ignoreComdats=*/false);
});
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 3bc39848d6035..b87a3ab429219 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -200,10 +200,8 @@ static void updateSupportedARMFeatures(const ARMAttributeParser &attributes) {
attributes.getAttributeValue(ARMBuildAttrs::ARM_ISA_use);
std::optional<unsigned> thumb =
attributes.getAttributeValue(ARMBuildAttrs::THUMB_ISA_use);
- bool noArmISA = !armISA || *armISA == ARMBuildAttrs::Not_Allowed;
- bool hasThumb2 = thumb && *thumb >= ARMBuildAttrs::AllowThumb32;
- if (noArmISA && hasThumb2)
- config->armThumbPLTs = true;
+ config->armHasArmISA |= armISA && *armISA >= ARMBuildAttrs::Allowed;
+ config->armHasThumb2ISA |= thumb && *thumb >= ARMBuildAttrs::AllowThumb32;
}
InputFile::InputFile(Kind k, MemoryBufferRef m)
diff --git a/lld/test/ELF/arm-mixed-plts.s b/lld/test/ELF/arm-mixed-plts.s
new file mode 100644
index 0000000000000..5699371449532
--- /dev/null
+++ b/lld/test/ELF/arm-mixed-plts.s
@@ -0,0 +1,44 @@
+# REQUIRES: arm
+
+# RUN: rm -rf %t && split-file %s %t
+# RUN: llvm-mc -filetype=obj -arm-add-build-attributes -triple=armv7a-none-linux-gnueabi %t/a.s -o %t1.o
+# RUN: llvm-mc -filetype=obj -arm-add-build-attributes -triple=armv7a-none-linux-gnueabi %t/b.s -o %t2.o
+# RUN: ld.lld -shared %t1.o %t2.o -o %t.so
+# RUN: llvm-objdump -d %t.so | FileCheck %s
+
+# Check that, when the input is a mixture of objects which can and cannot use
+# the ARM ISA, we use the default ARM PLT sequences.
+
+# CHECK: 000101d0 <.plt>:
+# CHECK-NEXT: 101d0: e52de004 str lr, [sp, #-0x4]!
+# CHECK-NEXT: 101d4: e28fe600 add lr, pc, #0, #12
+# CHECK-NEXT: 101d8: e28eea20 add lr, lr, #32, #20
+# CHECK-NEXT: 101dc: e5bef084 ldr pc, [lr, #0x84]!
+# CHECK-NEXT: 101e0: d4 d4 d4 d4 .word 0xd4d4d4d4
+# CHECK-NEXT: 101e4: d4 d4 d4 d4 .word 0xd4d4d4d4
+# CHECK-NEXT: 101e8: d4 d4 d4 d4 .word 0xd4d4d4d4
+# CHECK-NEXT: 101ec: d4 d4 d4 d4 .word 0xd4d4d4d4
+# CHECK-NEXT: 101f0: e28fc600 add r12, pc, #0, #12
+# CHECK-NEXT: 101f4: e28cca20 add r12, r12, #32, #20
+# CHECK-NEXT: 101f8: e5bcf06c ldr pc, [r12, #0x6c]!
+# CHECK-NEXT: 101fc: d4 d4 d4 d4 .word 0xd4d4d4d4
+
+#--- a.s
+ .globl foo
+ .type foo, %function
+ .globl bar
+ .type bar, %function
+
+ .thumb
+foo:
+ bl bar
+ bx lr
+
+#--- b.s
+ .eabi_attribute Tag_ARM_ISA_use, 0
+
+ .arm
+ .globl bar
+ .type bar, %function
+bar:
+ bx lr
|
|
@llvm/pr-subscribers-lld Author: Oliver Stannard (ostannard) ChangesPreviously, we selected the Thumb2 PLT sequences if any input object is marked as not supporting the ARM ISA, which then causes assertion failures when calls from ARM code in other objects are seen. I think the intention here was to only use Thumb PLTs when the target does not have the ARM ISA available, signalled by no objects being marked as having it available. To do that we need to track which ISAs we have seen as we parse the build attributes, and defer the decision about PLTs until all input objects have been parsed. This bug was triggered by real code in picolibc, which have some versions of string.h functions built with Thumb2-only build attributes, so that they are compatible with v7-A, v7-R and v7-M. Fixes #99008. Full diff: https://github.com/llvm/llvm-project/pull/101985.diff 4 Files Affected:
diff --git a/lld/ELF/Config.h b/lld/ELF/Config.h
index 183dc88a93e2f..d3ea095940cd8 100644
--- a/lld/ELF/Config.h
+++ b/lld/ELF/Config.h
@@ -218,6 +218,8 @@ struct Config {
bool allowMultipleDefinition;
bool fatLTOObjects;
bool androidPackDynRelocs = false;
+ bool armHasArmISA = false;
+ bool armHasThumb2ISA = false;
bool armThumbPLTs = false;
bool armHasBlx = false;
bool armHasMovtMovw = false;
diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
index a8c52e8c2b8e1..1928648a7c115 100644
--- a/lld/ELF/Driver.cpp
+++ b/lld/ELF/Driver.cpp
@@ -82,6 +82,7 @@ ConfigWrapper elf::config;
Ctx elf::ctx;
static void setConfigs(opt::InputArgList &args);
+static void setConfigsPostParse();
static void readConfigs(opt::InputArgList &args);
void elf::errorOrWarn(const Twine &msg) {
@@ -1886,6 +1887,14 @@ static void setConfigs(opt::InputArgList &args) {
}
}
+// Initialise config options which depend on information previously parsed from
+// input files, such as Arm build attributes.
+static void setConfigsPostParse() {
+ // Currently, Thumb PLTs require Thumb2, and are only used for targets which
+ // do not have the ARM ISA.
+ config->armThumbPLTs = config->armHasThumb2ISA && !config->armHasArmISA;
+}
+
static bool isFormatBinary(StringRef s) {
if (s == "binary")
return true;
@@ -2897,6 +2906,8 @@ template <class ELFT> void LinkerDriver::link(opt::InputArgList &args) {
// No more lazy bitcode can be extracted at this point. Do post parse work
// like checking duplicate symbols.
+ setConfigsPostParse();
+
parallelForEach(ctx.objectFiles, [](ELFFileBase *file) {
initSectionsAndLocalSyms(file, /*ignoreComdats=*/false);
});
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 3bc39848d6035..b87a3ab429219 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -200,10 +200,8 @@ static void updateSupportedARMFeatures(const ARMAttributeParser &attributes) {
attributes.getAttributeValue(ARMBuildAttrs::ARM_ISA_use);
std::optional<unsigned> thumb =
attributes.getAttributeValue(ARMBuildAttrs::THUMB_ISA_use);
- bool noArmISA = !armISA || *armISA == ARMBuildAttrs::Not_Allowed;
- bool hasThumb2 = thumb && *thumb >= ARMBuildAttrs::AllowThumb32;
- if (noArmISA && hasThumb2)
- config->armThumbPLTs = true;
+ config->armHasArmISA |= armISA && *armISA >= ARMBuildAttrs::Allowed;
+ config->armHasThumb2ISA |= thumb && *thumb >= ARMBuildAttrs::AllowThumb32;
}
InputFile::InputFile(Kind k, MemoryBufferRef m)
diff --git a/lld/test/ELF/arm-mixed-plts.s b/lld/test/ELF/arm-mixed-plts.s
new file mode 100644
index 0000000000000..5699371449532
--- /dev/null
+++ b/lld/test/ELF/arm-mixed-plts.s
@@ -0,0 +1,44 @@
+# REQUIRES: arm
+
+# RUN: rm -rf %t && split-file %s %t
+# RUN: llvm-mc -filetype=obj -arm-add-build-attributes -triple=armv7a-none-linux-gnueabi %t/a.s -o %t1.o
+# RUN: llvm-mc -filetype=obj -arm-add-build-attributes -triple=armv7a-none-linux-gnueabi %t/b.s -o %t2.o
+# RUN: ld.lld -shared %t1.o %t2.o -o %t.so
+# RUN: llvm-objdump -d %t.so | FileCheck %s
+
+# Check that, when the input is a mixture of objects which can and cannot use
+# the ARM ISA, we use the default ARM PLT sequences.
+
+# CHECK: 000101d0 <.plt>:
+# CHECK-NEXT: 101d0: e52de004 str lr, [sp, #-0x4]!
+# CHECK-NEXT: 101d4: e28fe600 add lr, pc, #0, #12
+# CHECK-NEXT: 101d8: e28eea20 add lr, lr, #32, #20
+# CHECK-NEXT: 101dc: e5bef084 ldr pc, [lr, #0x84]!
+# CHECK-NEXT: 101e0: d4 d4 d4 d4 .word 0xd4d4d4d4
+# CHECK-NEXT: 101e4: d4 d4 d4 d4 .word 0xd4d4d4d4
+# CHECK-NEXT: 101e8: d4 d4 d4 d4 .word 0xd4d4d4d4
+# CHECK-NEXT: 101ec: d4 d4 d4 d4 .word 0xd4d4d4d4
+# CHECK-NEXT: 101f0: e28fc600 add r12, pc, #0, #12
+# CHECK-NEXT: 101f4: e28cca20 add r12, r12, #32, #20
+# CHECK-NEXT: 101f8: e5bcf06c ldr pc, [r12, #0x6c]!
+# CHECK-NEXT: 101fc: d4 d4 d4 d4 .word 0xd4d4d4d4
+
+#--- a.s
+ .globl foo
+ .type foo, %function
+ .globl bar
+ .type bar, %function
+
+ .thumb
+foo:
+ bl bar
+ bx lr
+
+#--- b.s
+ .eabi_attribute Tag_ARM_ISA_use, 0
+
+ .arm
+ .globl bar
+ .type bar, %function
+bar:
+ bx lr
|
smithp35
reviewed
Aug 5, 2024
lld/ELF/Driver.cpp
Outdated
| @@ -1886,6 +1887,14 @@ static void setConfigs(opt::InputArgList &args) { | |||
| } | |||
| } | |||
|
|
|||
| // Initialise config options which depend on information previously parsed from | |||
| // input files, such as Arm build attributes. | |||
| static void setConfigsPostParse() { | |||
There was a problem hiding this comment.
Given that config->armThumbPLTs is only used within Arch/ARM.cpp and it is made up of only two booleans we could remove config->armThumbPLTs , this function and in Arch/ARM.cpp replace config->armThumbPLTs` with a function call to
static bool hasThumbPLT() {
return config->armHasThumb2ISA && !config->armHasArmISA;
}
The other bit of prior art we have in this area is the decision to write PAC/BTI PLT sequences. For that we had to delay until all the input objects had been written to. That ended up being done in a target specific getAArch64TargetInfo() but there is also getARMTargetInfo() that could be used.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
MaskRay
reviewed
Aug 5, 2024
lld/test/ELF/arm-mixed-plts.s
Outdated
| # RUN: ld.lld -shared %t1.o %t2.o -o %t.so | ||
| # RUN: llvm-objdump -d %t.so | FileCheck %s | ||
|
|
||
| # Check that, when the input is a mixture of objects which can and cannot use |
There was a problem hiding this comment.
In new tests, we use ## for non-CHECK-non-RUN comments.
MaskRay
reviewed
Aug 5, 2024
lld/test/ELF/arm-mixed-plts.s
Outdated
| # the ARM ISA, we use the default ARM PLT sequences. | ||
|
|
||
| # CHECK: 000101d0 <.plt>: | ||
| # CHECK-NEXT: 101d0: e52de004 str lr, [sp, #-0x4]! |
There was a problem hiding this comment.
The leading addresses are not significant and are omitted for new tests to ease test update.
smithp35
approved these changes
Aug 6, 2024
MaskRay
approved these changes
Aug 6, 2024
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Aug 7, 2024
Previously, we selected the Thumb2 PLT sequences if any input object is marked as not supporting the ARM ISA, which then causes assertion failures when calls from ARM code in other objects are seen. I think the intention here was to only use Thumb PLTs when the target does not have the ARM ISA available, signalled by no objects being marked as having it available. To do that we need to track which ISAs we have seen as we parse the build attributes, and defer the decision about PLTs until all input objects have been parsed. This bug was triggered by real code in picolibc, which have some versions of string.h functions built with Thumb2-only build attributes, so that they are compatible with v7-A, v7-R and v7-M. Fixes llvm#99008. (cherry picked from commit a1c6467)
tru
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Aug 10, 2024
Previously, we selected the Thumb2 PLT sequences if any input object is marked as not supporting the ARM ISA, which then causes assertion failures when calls from ARM code in other objects are seen. I think the intention here was to only use Thumb PLTs when the target does not have the ARM ISA available, signalled by no objects being marked as having it available. To do that we need to track which ISAs we have seen as we parse the build attributes, and defer the decision about PLTs until all input objects have been parsed. This bug was triggered by real code in picolibc, which have some versions of string.h functions built with Thumb2-only build attributes, so that they are compatible with v7-A, v7-R and v7-M. Fixes llvm#99008. (cherry picked from commit a1c6467)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, we selected the Thumb2 PLT sequences if any input object is marked as not supporting the ARM ISA, which then causes assertion failures when calls from ARM code in other objects are seen. I think the intention here was to only use Thumb PLTs when the target does not have the ARM ISA available, signalled by no objects being marked as having it available. To do that we need to track which ISAs we have seen as we parse the build attributes, and defer the decision about PLTs until all input objects have been parsed.
This bug was triggered by real code in picolibc, which have some versions of string.h functions built with Thumb2-only build attributes, so that they are compatible with v7-A, v7-R and v7-M.
Fixes #99008.