Skip to content

[JITLink][AArch32] Fix Unaligned Data Symbol Address Resolution #97030

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jun 29, 2024
Merged

[JITLink][AArch32] Fix Unaligned Data Symbol Address Resolution #97030

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5244f5f
[JITLink][AArch32] Fix Unaligned Data Symbol Address Resolution
eymay Jun 28, 2024
608326f
Enhance the test with LinkGraph and additional checks
eymay Jun 28, 2024
9fffa30
Make FileCheck checks stricter
eymay Jun 28, 2024
aa0b3f2
Turn the condition check from OBJECT to FUNC
eymay Jun 28, 2024
0859338
Make the checker expressions prettier
eymay Jun 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,9 @@ class ELFLinkGraphBuilder_aarch32

protected:
TargetFlagsType makeTargetFlags(const typename ELFT::Sym &Sym) override {
// Only emit target flag for callable symbols
if (Sym.getType() != ELF::STT_FUNC)
return TargetFlagsType{};
Copy link
Member

Choose a reason for hiding this comment

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

With your fix in getRawOffset() this check shouldn't be necessary anymore, isn't it? Instead we might want to assert that all symbols with the ThumbSymbol target flag are ELF::STT_FUNC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree but I had to keep it because of a failed assert in

llvm-project/llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp

Line 52 in f1faba2

assert(Sym.isCallable() && "Only callable symbols can have thumb flag");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With your fix in getRawOffset() this check shouldn't be necessary anymore, isn't it? Instead we might want to assert that all symbols with the ThumbSymbol target flag are ELF::STT_FUNC.

Updated the condition to check for STT_FUNC. I found a similar assert to what you mentioned in the previous comment, should I still add one?

if (Sym.getValue() & 0x01)
return aarch32::ThumbSymbol;
return TargetFlagsType{};
Expand All @@ -209,7 +212,9 @@ class ELFLinkGraphBuilder_aarch32
TargetFlagsType Flags) override {
assert((makeTargetFlags(Sym) & Flags) == Flags);
static constexpr uint64_t ThumbBit = 0x01;
return Sym.getValue() & ~ThumbBit;
if (Sym.getType() == ELF::STT_FUNC)
return Sym.getValue() & ~ThumbBit;
return Sym.getValue();
}

public:
Expand Down
72 changes: 72 additions & 0 deletions llvm/test/ExecutionEngine/JITLink/AArch32/ELF_data_alignment.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# RUN: llvm-mc -triple=armv7-linux-gnueabi -arm-add-build-attributes -filetype=obj -o %t_armv7.o %s
# RUN: llvm-objdump -s --section=.rodata %t_armv7.o | FileCheck --check-prefix=CHECK-OBJ %s
# RUN: llvm-jitlink -noexec -slab-address 0x76ff0000 -slab-allocate 10Kb \
# RUN: -slab-page-size 4096 %t_armv7.o -debug-only=jitlink 2>&1 \
# RUN: | FileCheck --check-prefix=CHECK-LG %s
# RUN: llvm-jitlink -noexec -slab-address 0x76ff0000 -slab-allocate 10Kb \
# RUN: -slab-page-size 4096 %t_armv7.o -check %s

# RUN: llvm-mc -triple=thumbv7-linux-gnueabi -arm-add-build-attributes -filetype=obj -o %t_thumbv7.o %s
# RUN: llvm-objdump -s --section=.rodata %t_thumbv7.o | FileCheck --check-prefix=CHECK-OBJ %s
# RUN: llvm-jitlink -noexec -slab-address 0x76ff0000 -slab-allocate 10Kb \
# RUN: -slab-page-size 4096 %t_thumbv7.o -debug-only=jitlink 2>&1 \
# RUN: | FileCheck --check-prefix=CHECK-LG %s
# RUN: llvm-jitlink -noexec -slab-address 0x76ff0000 -slab-allocate 10Kb \
# RUN: -slab-page-size 4096 %t_thumbv7.o -check %s

# The strings of "H1\00", "H2\00" and "H3\00" are encoded as
# 0x483100, 0x483200 and 0x483300 .rodata section.
# CHECK-OBJ: Contents of section .rodata:
# CHECK-OBJ: 0000 48310048 32004833 00 H1.H2.H3.

# CHECK-LG: Starting link phase 1 for graph
# CHECK-LG: section .rodata:

# CHECK-LG: block 0x0 size = 0x00000009, align = 1, alignment-offset = 0
# CHECK-LG-NEXT: symbols:
# CHECK-LG-NEXT: 0x0 (block + 0x00000000): size: 0x00000003, linkage: strong, scope: default, live - Lstr.H1
# CHECK-LG-NEXT: 0x3 (block + 0x00000003): size: 0x00000003, linkage: strong, scope: default, live - Lstr.H2
# CHECK-LG-NOT: 0x2 (block + 0x00000002): size: 0x00000003, linkage: strong, scope: default, live - Lstr.H2
# CHECK-LG-NEXT: 0x6 (block + 0x00000006): size: 0x00000003, linkage: strong, scope: default, live - Lstr.H3

# FIXME: The expression we want is either *{3}(Lstr.H1) = ...
# or *{4}(Lstr.H1) & 0x00ffffff = ...
# The first is not supported and the latter segfaults.
# Also, whitespaces are not recognized and not consumed by the checker.
Copy link
Member

Choose a reason for hiding this comment

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

I think you could write either (*{4}(Lstr.H1))[23:0] or (*{4}(Lstr.H1)) & 0x00ffffff. Does that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does! Thanks for the tip.


# jitlink-check: Lstr.H1 = 0x76ff0000
# jitlink-check: 0x00ffffff&*{4}(Lstr.H1) = 0x003148
.globl Lstr.H1
.type Lstr.H1,%object
.section .rodata,"a",%progbits
Lstr.H1:
.asciz "H1"
.size Lstr.H1, 3

# H2 is unaligned as its beginning address is base address + 0x3
# Make sure the string we get is 0x003248 and not 0x324800
# jitlink-check: Lstr.H2 = 0x76ff0003
# jitlink-check: 0x00ffffff&*{4}(Lstr.H2) = 0x003248
.globl Lstr.H2
.type Lstr.H2,%object
Lstr.H2:
.asciz "H2"
.size Lstr.H2, 3

# jitlink-check: Lstr.H3 = 0x76ff0006
# jitlink-check: 0x00ffffff&*{4}(Lstr.H3) = 0x003348
.globl Lstr.H3
.type Lstr.H3,%object
Lstr.H3:
.asciz "H3"
.size Lstr.H3, 3

.text
.syntax unified
# Empty main function for jitlink to be happy
.globl main
.type main,%function
.p2align 2
main:
bx lr
.size main,.-main
4 changes: 0 additions & 4 deletions llvm/test/ExecutionEngine/Orc/global-ctor-order.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
; Test that global constructors are correctly ordered
;
; Uncovers a pre-existing issue on Arm 32 bit, see
; https://github.com/llvm/llvm-project/issues/95911.
; UNSUPPORTED: target=arm{{.*}}
;
; RUN: lli -jit-kind=orc %s | FileCheck %s
;
; CHECK: H1
Expand Down
Loading