[ARM] [Windows] Use IMAGE_SYM_CLASS_STATIC for private functions #101828
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
|
@llvm/pr-subscribers-backend-arm @llvm/pr-subscribers-mc Author: Martin Storsjö (mstorsjo) ChangesFor functions with private linkage, pick This matches what the AArch64 target does. This activates a preexisting fix for the AArch64 target from 1e7f592, for the ARM target as well. When a relocation points at a symbol, one usually can convey an offset to the symbol by encoding it as an immediate in the instruction. However, for the ARM and AArch64 branch instructions, the immediate stored in the instruction is ignored by MS link.exe (and lld-link matches this aspect). (It would be simple to extend lld-link to support it - but such object files would be incompatible with MS link.exe.) This was worked around by 1e7f592 by emitting symbols into the object file symbol table, for temporary symbols that otherwise would have been omitted, if they have the class IMAGE_SYM_CLASS_STATIC, in order to avoid needing an offset in the relocated instruction. This change gives the symbols generated from functions with the IR level "private" linkage the right class, to activate that workaround. This fixes #100101, fixing code generation for coroutines for Windows on ARM. After the change in f786881, coroutines generate a function with private linkage, and calls to this function were previously broken for this target. Full diff: https://github.com/llvm/llvm-project/pull/101828.diff 3 Files Affected:
diff --git a/llvm/lib/Target/ARM/ARMAsmPrinter.cpp b/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
index 642739a29d6b0..6cf63055a518b 100644
--- a/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
+++ b/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
@@ -153,9 +153,9 @@ bool ARMAsmPrinter::runOnMachineFunction(MachineFunction &MF) {
OptimizationGoals = 0;
if (Subtarget->isTargetCOFF()) {
- bool Internal = F.hasInternalLinkage();
- COFF::SymbolStorageClass Scl = Internal ? COFF::IMAGE_SYM_CLASS_STATIC
- : COFF::IMAGE_SYM_CLASS_EXTERNAL;
+ bool Local = F.hasLocalLinkage();
+ COFF::SymbolStorageClass Scl = Local ? COFF::IMAGE_SYM_CLASS_STATIC
+ : COFF::IMAGE_SYM_CLASS_EXTERNAL;
int Type = COFF::IMAGE_SYM_DTYPE_FUNCTION << COFF::SCT_COMPLEX_TYPE_SHIFT;
OutStreamer->beginCOFFSymbolDef(CurrentFnSym);
diff --git a/llvm/test/CodeGen/ARM/Windows/private-func.ll b/llvm/test/CodeGen/ARM/Windows/private-func.ll
new file mode 100644
index 0000000000000..2d030ae3fabbb
--- /dev/null
+++ b/llvm/test/CodeGen/ARM/Windows/private-func.ll
@@ -0,0 +1,17 @@
+; RUN: llc -mtriple thumbv7-windows -filetype asm -o - %s | FileCheck %s
+
+define dso_local void @func1() {
+entry:
+ call void @func2()
+ ret void
+}
+
+define private void @func2() {
+entry:
+ ret void
+}
+
+; CHECK: .def .Lfunc2;
+; CHECK-NEXT: .scl 3;
+; CHECK-NEXT: .type 32;
+; CHECK-NEXT: .endef
diff --git a/llvm/test/MC/ARM/Windows/branch-reloc-offset.s b/llvm/test/MC/ARM/Windows/branch-reloc-offset.s
new file mode 100644
index 0000000000000..2e70a723ccf78
--- /dev/null
+++ b/llvm/test/MC/ARM/Windows/branch-reloc-offset.s
@@ -0,0 +1,57 @@
+// RUN: llvm-mc -triple thumbv7-windows-gnu -filetype obj %s -o - | llvm-objdump -D -r - | FileCheck %s
+
+ .text
+main:
+ nop
+ b .Ltarget
+ b .Lother_target
+
+// A private label target in the same section
+ .def .Ltarget
+ .scl 3
+ .type 32
+ .endef
+ .p2align 2
+.Ltarget:
+ bx lr
+
+// A private label target in another section
+ .section "other", "xr"
+ nop
+ nop
+ nop
+ nop
+ nop
+ nop
+ nop
+ nop
+ .def .Lother_target
+ .scl 3
+ .type 32
+ .endef
+ .p2align 2
+.Lother_target:
+ bx lr
+
+// Check that both branches have a relocation with a zero offset.
+//
+// CHECK: 00000000 <main>:
+// CHECK: 0: bf00 nop
+// CHECK: 2: f000 b800 b.w 0x6 <main+0x6> @ imm = #0x0
+// CHECK: 00000002: IMAGE_REL_ARM_BRANCH24T .Ltarget
+// CHECK: 6: f000 b800 b.w 0xa <main+0xa> @ imm = #0x0
+// CHECK: 00000006: IMAGE_REL_ARM_BRANCH24T .Lother_target
+// CHECK: a: bf00 nop
+// CHECK: 0000000c <.Ltarget>:
+// CHECK: c: 4770 bx lr
+// CHECK: 00000000 <other>:
+// CHECK: 0: bf00 nop
+// CHECK: 2: bf00 nop
+// CHECK: 4: bf00 nop
+// CHECK: 6: bf00 nop
+// CHECK: 8: bf00 nop
+// CHECK: a: bf00 nop
+// CHECK: c: bf00 nop
+// CHECK: e: bf00 nop
+// CHECK: 00000010 <.Lother_target>:
+// CHECK: 10: 4770 bx lr
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
We could also add similar checking as was done in 1e7f592, to error out instead of creating relocations with a nonzero offset - I have such a change as well, which can be submitted as a separate step afterwards. I tried to keep this one as small as possible to reduce the risk wrt backporting it. |
For functions with private linkage, pick IMAGE_SYM_CLASS_STATIC rather than IMAGE_SYM_CLASS_EXTERNAL; GlobalValue::isInternalLinkage() only checks for InternalLinkage, while GlobalValue::isLocalLinkage() checks for both InternalLinkage and PrivateLinkage. This matches what the AArch64 target does. This activates a preexisting fix for the AArch64 target from 1e7f592, for the ARM target as well. When a relocation points at a symbol, one usually can convey an offset to the symbol by encoding it as an immediate in the instruction. However, for the ARM and AArch64 branch instructions, the immediate stored in the instruction is ignored by MS link.exe (and lld-link matches this aspect). (It would be simple to extend lld-link to support it - but such object files would be incompatible with MS link.exe.) This was worked around by 1e7f592 by emitting symbols into the object file symbol table, for temporary symbols that otherwise would have been omitted, if they have the class IMAGE_SYM_CLASS_STATIC, in order to avoid needing an offset in the relocated instruction. This change gives the symbols generated from functions with the IR level "private" linkage the right class, to activate that workaround. This fixes llvm#100101, fixing code generation for coroutines for Windows on ARM. After the change in f786881, coroutines generate a function with private linkage, and calls to this function were previously broken for this target.
mstorsjo
force-pushed
the
arm-private-func-static
branch
from
009491d to
cc1ee7e
Compare
|
FWIW, this matches AArch64 commit 3406934. |
cjacek
approved these changes
Aug 4, 2024
|
/cherry-pick 8dd065d |
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Aug 4, 2024
…m#101828) For functions with private linkage, pick IMAGE_SYM_CLASS_STATIC rather than IMAGE_SYM_CLASS_EXTERNAL; GlobalValue::isInternalLinkage() only checks for InternalLinkage, while GlobalValue::isLocalLinkage() checks for both InternalLinkage and PrivateLinkage. This matches what the AArch64 target does, since commit 3406934. This activates a preexisting fix for the AArch64 target from 1e7f592, for the ARM target as well. When a relocation points at a symbol, one usually can convey an offset to the symbol by encoding it as an immediate in the instruction. However, for the ARM and AArch64 branch instructions, the immediate stored in the instruction is ignored by MS link.exe (and lld-link matches this aspect). (It would be simple to extend lld-link to support it - but such object files would be incompatible with MS link.exe.) This was worked around by 1e7f592 by emitting symbols into the object file symbol table, for temporary symbols that otherwise would have been omitted, if they have the class IMAGE_SYM_CLASS_STATIC, in order to avoid needing an offset in the relocated instruction. This change gives the symbols generated from functions with the IR level "private" linkage the right class, to activate that workaround. This fixes llvm#100101, fixing code generation for coroutines for Windows on ARM. After the change in f786881, coroutines generate a function with private linkage, and calls to this function were previously broken for this target. (cherry picked from commit 8dd065d)
|
/pull-request #101904 |
tru
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Aug 5, 2024
…m#101828) For functions with private linkage, pick IMAGE_SYM_CLASS_STATIC rather than IMAGE_SYM_CLASS_EXTERNAL; GlobalValue::isInternalLinkage() only checks for InternalLinkage, while GlobalValue::isLocalLinkage() checks for both InternalLinkage and PrivateLinkage. This matches what the AArch64 target does, since commit 3406934. This activates a preexisting fix for the AArch64 target from 1e7f592, for the ARM target as well. When a relocation points at a symbol, one usually can convey an offset to the symbol by encoding it as an immediate in the instruction. However, for the ARM and AArch64 branch instructions, the immediate stored in the instruction is ignored by MS link.exe (and lld-link matches this aspect). (It would be simple to extend lld-link to support it - but such object files would be incompatible with MS link.exe.) This was worked around by 1e7f592 by emitting symbols into the object file symbol table, for temporary symbols that otherwise would have been omitted, if they have the class IMAGE_SYM_CLASS_STATIC, in order to avoid needing an offset in the relocated instruction. This change gives the symbols generated from functions with the IR level "private" linkage the right class, to activate that workaround. This fixes llvm#100101, fixing code generation for coroutines for Windows on ARM. After the change in f786881, coroutines generate a function with private linkage, and calls to this function were previously broken for this target. (cherry picked from commit 8dd065d)
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.
For functions with private linkage, pick
IMAGE_SYM_CLASS_STATIC rather than IMAGE_SYM_CLASS_EXTERNAL; GlobalValue::isInternalLinkage() only checks for
InternalLinkage, while GlobalValue::isLocalLinkage() checks for both InternalLinkage and PrivateLinkage.
This matches what the AArch64 target does.
This activates a preexisting fix for the AArch64 target from 1e7f592, for the ARM target as well.
When a relocation points at a symbol, one usually can convey an offset to the symbol by encoding it as an immediate in the instruction. However, for the ARM and AArch64 branch instructions, the immediate stored in the instruction is ignored by MS link.exe (and lld-link matches this aspect). (It would be simple to extend lld-link to support it - but such object files would be incompatible with MS link.exe.)
This was worked around by 1e7f592 by emitting symbols into the object file symbol table, for temporary symbols that otherwise would have been omitted, if they have the class IMAGE_SYM_CLASS_STATIC, in order to avoid needing an offset in the relocated instruction.
This change gives the symbols generated from functions with the IR level "private" linkage the right class, to activate that workaround.
This fixes #100101, fixing code generation for coroutines for Windows on ARM. After the change in f786881, coroutines generate a function with private linkage, and calls to this function were previously broken for this target.