From 5244f5f64e60dc596313577d5584df64ef210581 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eymen=20=C3=9Cnay?= Date: Fri, 28 Jun 2024 12:34:26 +0300 Subject: [PATCH 1/5] [JITLink][AArch32] Fix Unaligned Data Symbol Address Resolution The ARM architecture uses the LSB bit for ARM/Thumb mode switch flagging. This is true for alignments of 2 and 4 but in data relocations the alignment is 1 allowing the LSB bit to be set. ELF::STT_OBJECT typed symbols are made to bypass the TargetFlag mechanism assuming they are the only symbols affected by data relocations. The test is a minimal example of the issue mentioned below. Fixes the issue "Orc global constructor order test fails on 32 bit ARM #95911". --- .../ExecutionEngine/JITLink/ELF_aarch32.cpp | 8 +++ .../JITLink/AArch32/data-alignment.s | 50 +++++++++++++++++++ .../ExecutionEngine/Orc/global-ctor-order.ll | 4 -- 3 files changed, 58 insertions(+), 4 deletions(-) create mode 100644 llvm/test/ExecutionEngine/JITLink/AArch32/data-alignment.s diff --git a/llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp b/llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp index 908b88fef1b31..3d8af86afa76b 100644 --- a/llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp +++ b/llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp @@ -200,6 +200,9 @@ class ELFLinkGraphBuilder_aarch32 protected: TargetFlagsType makeTargetFlags(const typename ELFT::Sym &Sym) override { + // Data symbols do not have Arm or Thumb flags. + if (Sym.getType() == ELF::STT_OBJECT) + return TargetFlagsType{}; if (Sym.getValue() & 0x01) return aarch32::ThumbSymbol; return TargetFlagsType{}; @@ -208,6 +211,11 @@ class ELFLinkGraphBuilder_aarch32 orc::ExecutorAddrDiff getRawOffset(const typename ELFT::Sym &Sym, TargetFlagsType Flags) override { assert((makeTargetFlags(Sym) & Flags) == Flags); + // Data relocations can be aligned to 1 making some symbol addresses have + // their LSB set. To access the real addresses of these symbols their LSB + // should be protected. + if (Sym.getType() == ELF::STT_OBJECT) + return Sym.getValue(); static constexpr uint64_t ThumbBit = 0x01; return Sym.getValue() & ~ThumbBit; } diff --git a/llvm/test/ExecutionEngine/JITLink/AArch32/data-alignment.s b/llvm/test/ExecutionEngine/JITLink/AArch32/data-alignment.s new file mode 100644 index 0000000000000..bda6872a7be0c --- /dev/null +++ b/llvm/test/ExecutionEngine/JITLink/AArch32/data-alignment.s @@ -0,0 +1,50 @@ +# 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-DATA %s +# RUN: llvm-jitlink -noexec -slab-address 0x76ff0000 -slab-allocate 10Kb \ +# RUN: -slab-page-size 4096 -check %s %t_armv7.o + +# 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-DATA %s +# RUN: llvm-jitlink -noexec -slab-address 0x76ff0000 -slab-allocate 10Kb \ +# RUN: -slab-page-size 4096 -check %s %t_armv7.o + +# The strings of "H1\00", "H2\00" and "H3\00" are encoded as +# 0x483100, 0x483200 and 0x483300 .rodata section. +# CHECK-DATA: Contents of section .rodata: +# CHECK-DATA: 0000 48310048 32004833 00 + +# 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. + +# 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 + +# Not 0x324800 +# jitlink-check: 0x00ffffff&*{4}(Lstr.H2) = 0x003248 + .globl Lstr.H2 + .type Lstr.H2,%object +Lstr.H2: + .asciz "H2" + .size Lstr.H2, 3 + +# jitlink-check: 0x00ffffff&*{4}(Lstr.H3) = 0x003348 + .globl Lstr.H3 + .type Lstr.H3,%object +Lstr.H3: + .asciz "H3" + .size Lstr.H3, 3 + +# Empty main function for jitlink to be happy + .globl main + .type main,%function + .p2align 2 +main: + bx lr + .size main,.-main diff --git a/llvm/test/ExecutionEngine/Orc/global-ctor-order.ll b/llvm/test/ExecutionEngine/Orc/global-ctor-order.ll index de9a35465f8cb..98cf67af8276d 100644 --- a/llvm/test/ExecutionEngine/Orc/global-ctor-order.ll +++ b/llvm/test/ExecutionEngine/Orc/global-ctor-order.ll @@ -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 From 608326f98ed0717262d706ad269de901e33cc609 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eymen=20=C3=9Cnay?= Date: Fri, 28 Jun 2024 16:44:40 +0300 Subject: [PATCH 2/5] Enhance the test with LinkGraph and additional checks --- .../JITLink/AArch32/ELF_data_alignment.s | 72 +++++++++++++++++++ .../JITLink/AArch32/data-alignment.s | 50 ------------- 2 files changed, 72 insertions(+), 50 deletions(-) create mode 100644 llvm/test/ExecutionEngine/JITLink/AArch32/ELF_data_alignment.s delete mode 100644 llvm/test/ExecutionEngine/JITLink/AArch32/data-alignment.s diff --git a/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_data_alignment.s b/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_data_alignment.s new file mode 100644 index 0000000000000..935ba28a75974 --- /dev/null +++ b/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_data_alignment.s @@ -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: symbols: +# CHECK-LG: 0x0 (block + 0x00000000): size: 0x00000003, linkage: strong, scope: default, live - Lstr.H1 +# CHECK-LG: 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: 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. + +# 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 diff --git a/llvm/test/ExecutionEngine/JITLink/AArch32/data-alignment.s b/llvm/test/ExecutionEngine/JITLink/AArch32/data-alignment.s deleted file mode 100644 index bda6872a7be0c..0000000000000 --- a/llvm/test/ExecutionEngine/JITLink/AArch32/data-alignment.s +++ /dev/null @@ -1,50 +0,0 @@ -# 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-DATA %s -# RUN: llvm-jitlink -noexec -slab-address 0x76ff0000 -slab-allocate 10Kb \ -# RUN: -slab-page-size 4096 -check %s %t_armv7.o - -# 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-DATA %s -# RUN: llvm-jitlink -noexec -slab-address 0x76ff0000 -slab-allocate 10Kb \ -# RUN: -slab-page-size 4096 -check %s %t_armv7.o - -# The strings of "H1\00", "H2\00" and "H3\00" are encoded as -# 0x483100, 0x483200 and 0x483300 .rodata section. -# CHECK-DATA: Contents of section .rodata: -# CHECK-DATA: 0000 48310048 32004833 00 - -# 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. - -# 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 - -# Not 0x324800 -# jitlink-check: 0x00ffffff&*{4}(Lstr.H2) = 0x003248 - .globl Lstr.H2 - .type Lstr.H2,%object -Lstr.H2: - .asciz "H2" - .size Lstr.H2, 3 - -# jitlink-check: 0x00ffffff&*{4}(Lstr.H3) = 0x003348 - .globl Lstr.H3 - .type Lstr.H3,%object -Lstr.H3: - .asciz "H3" - .size Lstr.H3, 3 - -# Empty main function for jitlink to be happy - .globl main - .type main,%function - .p2align 2 -main: - bx lr - .size main,.-main From 9fffa30fe1999ee8e23e9320f9c0bab1b0645b07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eymen=20=C3=9Cnay?= Date: Fri, 28 Jun 2024 17:19:06 +0300 Subject: [PATCH 3/5] Make FileCheck checks stricter --- .../JITLink/AArch32/ELF_data_alignment.s | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_data_alignment.s b/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_data_alignment.s index 935ba28a75974..916e0112c70e5 100644 --- a/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_data_alignment.s +++ b/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_data_alignment.s @@ -22,12 +22,12 @@ # 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: symbols: -# CHECK-LG: 0x0 (block + 0x00000000): size: 0x00000003, linkage: strong, scope: default, live - Lstr.H1 -# CHECK-LG: 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: 0x6 (block + 0x00000006): size: 0x00000003, linkage: strong, scope: default, live - Lstr.H3 +# 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 = ... From aa0b3f26cd236bb27e9af3e7ddd7d9fe0cee241c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eymen=20=C3=9Cnay?= Date: Fri, 28 Jun 2024 17:39:15 +0300 Subject: [PATCH 4/5] Turn the condition check from OBJECT to FUNC --- llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp b/llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp index 3d8af86afa76b..866de2cb227c3 100644 --- a/llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp +++ b/llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp @@ -200,8 +200,8 @@ class ELFLinkGraphBuilder_aarch32 protected: TargetFlagsType makeTargetFlags(const typename ELFT::Sym &Sym) override { - // Data symbols do not have Arm or Thumb flags. - if (Sym.getType() == ELF::STT_OBJECT) + // Only emit target flag for callable symbols + if (Sym.getType() != ELF::STT_FUNC) return TargetFlagsType{}; if (Sym.getValue() & 0x01) return aarch32::ThumbSymbol; @@ -211,13 +211,10 @@ class ELFLinkGraphBuilder_aarch32 orc::ExecutorAddrDiff getRawOffset(const typename ELFT::Sym &Sym, TargetFlagsType Flags) override { assert((makeTargetFlags(Sym) & Flags) == Flags); - // Data relocations can be aligned to 1 making some symbol addresses have - // their LSB set. To access the real addresses of these symbols their LSB - // should be protected. - if (Sym.getType() == ELF::STT_OBJECT) - return Sym.getValue(); static constexpr uint64_t ThumbBit = 0x01; - return Sym.getValue() & ~ThumbBit; + if (Sym.getType() == ELF::STT_FUNC) + return Sym.getValue() & ~ThumbBit; + return Sym.getValue(); } public: From 0859338cfee9e4163128675e10b214be3d1615bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eymen=20=C3=9Cnay?= Date: Fri, 28 Jun 2024 22:07:02 +0300 Subject: [PATCH 5/5] Make the checker expressions prettier --- .../JITLink/AArch32/ELF_data_alignment.s | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_data_alignment.s b/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_data_alignment.s index 916e0112c70e5..882e938e0fadb 100644 --- a/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_data_alignment.s +++ b/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_data_alignment.s @@ -15,7 +15,7 @@ # 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. +# 0x483100, 0x483200 and 0x483300 in the .rodata section. # CHECK-OBJ: Contents of section .rodata: # CHECK-OBJ: 0000 48310048 32004833 00 H1.H2.H3. @@ -29,13 +29,8 @@ # 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. - # jitlink-check: Lstr.H1 = 0x76ff0000 -# jitlink-check: 0x00ffffff&*{4}(Lstr.H1) = 0x003148 +# jitlink-check: (*{4}(Lstr.H1))[23:0] = 0x003148 .globl Lstr.H1 .type Lstr.H1,%object .section .rodata,"a",%progbits @@ -46,7 +41,7 @@ Lstr.H1: # 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 +# jitlink-check: (*{4}(Lstr.H2))[23:0] = 0x003248 .globl Lstr.H2 .type Lstr.H2,%object Lstr.H2: @@ -54,7 +49,7 @@ Lstr.H2: .size Lstr.H2, 3 # jitlink-check: Lstr.H3 = 0x76ff0006 -# jitlink-check: 0x00ffffff&*{4}(Lstr.H3) = 0x003348 +# jitlink-check: (*{4}(Lstr.H3))[23:0] = 0x003348 .globl Lstr.H3 .type Lstr.H3,%object Lstr.H3: