This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Unconditionally use DW_EH_PE_indirect|DW_EH_PE_pcrel personality/lsda/ttype encodings
ClosedPublic

Authored by MaskRay on Jan 31 2023, 5:39 PM.

Details

Summary

For -fno-pic, without DW_EH_PE_indirect, the personality routine pointer in a
CIE needs an R_AARCH64_ABS64 relocation. In common configurations that
__gcc_personality_v0 is defined in a shared object, this will lead to a
discouraged canonical PLT entry, or, if ld.lld -z notext (betwen D122459 and
D143136), a dynamic R_AARCH64_ABS64 relocation with an incorrect offset:
https://github.com/llvm/llvm-project/issues/60392

Since GCC uses DW_EH_PE_indirect for -fno-pic code (the behavior hasn't changed
since the initial port in 2012), let's follow suit by simplifying the code.
(
For tiny and small code models, we use DW_EH_PE_sdata8 instead of GCC's
DW_EH_PE_sdata4. This is a deliberate choice to support personality-.eh_frame
offset > 2GiB. This is unneeded for small code model since "Max text segment
size < 2GiB" but making -fno-pic -mcmodel={tiny,small} different seems
unnecessary: the scenarios that uses both -fno-pic and C++ exceptions have been
increasingly rare now, so there is little advantage optimizing for the little
size saving with code complexity.
)


Two clang/test/Interpreter tests would fail without 6747fc07d1aa94e22622e278e5a02ba70675ac9b
([ORC] Use JITLink as the default linker for LLJIT on Linux/arm64.)

Diff Detail

Event Timeline

MaskRay created this revision.Jan 31 2023, 5:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 5:39 PM
MaskRay requested review of this revision.Jan 31 2023, 5:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 5:39 PM
MaskRay edited the summary of this revision. (Show Details)Jan 31 2023, 5:42 PM
MaskRay retitled this revision from [AArch64] Use DW_EH_PE_indirect personality/lsda/ttype encodings for -fno-pic to [AArch64] Unconditionally use DW_EH_PE_indirect|DW_EH_PE_pcrel personality/lsda/ttype encodings.Jan 31 2023, 5:46 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay edited the summary of this revision. (Show Details)Jan 31 2023, 5:54 PM
MatzeB accepted this revision.Feb 1 2023, 7:27 AM

LGTM, thanks

This revision is now accepted and ready to land.Feb 1 2023, 7:27 AM

For tiny and small code models, we use DW_EH_PE_sdata8 instead of GCC's
DW_EH_PE_sdata4. This is a deliberate choice to support personality-.eh_frame
offset > 2GiB.

Does this actually make sense? In small/medium, these are all required to be within 2GB anyways, no?

For tiny and small code models, we use DW_EH_PE_sdata8 instead of GCC's
DW_EH_PE_sdata4. This is a deliberate choice to support personality-.eh_frame
offset > 2GiB.

Does this actually make sense? In small/medium, these are all required to be within 2GB anyways, no?

This is what the existing comment intends to support: if .eh_frame resides at 0x30000000 while personality resides at 0x1000, this is valid small code model (4GiB instead of 2GiB!)

aarch64 small code model supports 4GiB max combined code and data.

The Code Models https://github.com/ARM-software/abi-aa/blob/main/sysvabi64/sysvabi64.rst#7code-models do clarify that the text segment includes ro-data explicitly for the benefit of signed 32-bit offsets, however I don't think that this should change the decision here to use sdata8 for AArch64 targets. So no objections from me.

The Code Models https://github.com/ARM-software/abi-aa/blob/main/sysvabi64/sysvabi64.rst#7code-models do clarify that the text segment includes ro-data explicitly for the benefit of signed 32-bit offsets, however I don't think that this should change the decision here to use sdata8 for AArch64 targets. So no objections from me.

Thanks. Perhaps a better interpretation is that it is unnecessary to make -fno-pic -mcmodel={tiny,small} different. The scenarios that uses both -fno-pic and C++ exceptions have been increasingly rare now, so there is not many advantage optimizing for the little size saving here.

MaskRay edited the summary of this revision. (Show Details)Feb 3 2023, 10:33 AM
MaskRay edited the summary of this revision. (Show Details)Feb 3 2023, 10:38 AM
chapuni reopened this revision.Feb 4 2023, 4:09 AM
chapuni added a subscriber: chapuni.

Excuse me, I have reverted this.
See also, https://lab.llvm.org/buildbot/#/builders/183/builds/10611

This revision is now accepted and ready to land.Feb 4 2023, 4:09 AM
lhames added a subscriber: lhames.Feb 4 2023, 9:35 AM
JIT session error: Symbols not found: [ DW.ref.__gxx_personality_v0 ]

If I hand-compile source for the failing simple-execution.cpp testcase on Darwin with clang --target=arm64-pc-linux-gnu -fno-pic -S I see new references to DW.ref.__gxx_personality_v0, but I also see a definition:

        .hidden DW.ref.__gxx_personality_v0
        .weak   DW.ref.__gxx_personality_v0
        .section        .data.DW.ref.__gxx_personality_v0,"aGw",@progbits,DW.ref.__gxx_personality_v0,comdat
        .p2align        3, 0x0
        .type   DW.ref.__gxx_personality_v0,@object
        .size   DW.ref.__gxx_personality_v0, 8
DW.ref.__gxx_personality_v0:
        .xword  __gxx_personality_v0

My best guess is that the JIT linker is dropping this. I'm just doing a Linux build now so that I can figure out what's going wrong.

JIT session error: Symbols not found: [ DW.ref.__gxx_personality_v0 ]

If I hand-compile source for the failing simple-execution.cpp testcase on Darwin with clang --target=arm64-pc-linux-gnu -fno-pic -S I see new references to DW.ref.__gxx_personality_v0, but I also see a definition:

        .hidden DW.ref.__gxx_personality_v0
        .weak   DW.ref.__gxx_personality_v0
        .section        .data.DW.ref.__gxx_personality_v0,"aGw",@progbits,DW.ref.__gxx_personality_v0,comdat
        .p2align        3, 0x0
        .type   DW.ref.__gxx_personality_v0,@object
        .size   DW.ref.__gxx_personality_v0, 8
DW.ref.__gxx_personality_v0:
        .xword  __gxx_personality_v0

My best guess is that the JIT linker is dropping this. I'm just doing a Linux build now so that I can figure out what's going wrong.

I suspect that 43acef48d38ec0dd391f212144d4a25095e4fc5f has incomplete support.

rg DW_EH_PE_indirect llvm/lib/ExecutionEngine has no occurrence while I think it should. libunwind/src/AddressSpace.hpp:352 describes how DW_EH_PE_indirect works.

For llvm/test/ExecutionEngine/JITLink/X86/ELF_ehframe_large_static_personality_encodings.s, I think .cfi_personality 0, __gxx_personality_v0 and .cfi_personality 156, DW.ref.__gxx_personality_v0 should be in separate tests to show that .cfi_personality 156, DW.ref.__gxx_personality_v0 is correctly handled.

lhames added a comment.Feb 4 2023, 6:18 PM

My best guess is that the JIT linker is dropping this. I'm just doing a Linux build now so that I can figure out what's going wrong.

I suspect that 43acef48d38ec0dd391f212144d4a25095e4fc5f has incomplete support.

I'm not sure that the linker needs to do anything with DW_EH_PE_indirect, does it? libunwind to know to do one more level of pointer chasing, but from the linker's point it's irrelevant.

Looks like the root cause is that we're not using JITLink at all -- on Linux/aarch64 we still default to using RuntimeDyld, and that's fumbling the weak symbol somehow. I'm going to try switching LLJIT's default for Linux/aarch64 to JITLink -- it's about time we did that anyway.

lhames added a comment.Feb 4 2023, 9:11 PM

LLJIT has been update to default to JITLink on arm64 in 6747fc07d1aa, and on my local machine the interpreter tests now pass with this patch applied.

We should probably give 6747fc07d1aa a minute to settle on the bots, but then I think you're clear to re-land this.

For RuntimeDyld, we would have maybe needed the equivalent of 72ea1a721e005f29c6fea4a684807a68abd93c39 also for AArch64?

MaskRay edited the summary of this revision. (Show Details)Feb 5 2023, 10:44 AM

LLJIT has been update to default to JITLink on arm64 in 6747fc07d1aa, and on my local machine the interpreter tests now pass with this patch applied.

We should probably give 6747fc07d1aa a minute to settle on the bots, but then I think you're clear to re-land this.

Thanks! I tested this patch on an Ubuntu Linux aarch64 22.04 machine and check-clang-interpreter tests now pass.
(And I see how riscv64 host (which also uses DW_EH_PE_indirect) works as well: it also uses JITLink.)