Skip to content
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.

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

[RelocationResolver][Xtensa] Implement R_XTENSA_32 #96311

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 27 additions & 2 deletions llvm/lib/Object/RelocationResolver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,28 @@ static uint64_t resolveX86(uint64_t Type, uint64_t Offset, uint64_t S,
}
}

static bool supportsXtensa(uint64_t Type) {
switch (Type) {
case ELF::R_XTENSA_NONE:
case ELF::R_XTENSA_32:
return true;
default:
return false;
}
}

static uint64_t resolveXtensa(uint64_t Type, uint64_t Offset, uint64_t S,
uint64_t LocData, int64_t Addend) {
switch (Type) {
case ELF::R_XTENSA_NONE:
Copy link
Member

Choose a reason for hiding this comment

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

We could drop R_*_NONE. It should not appear in relocation files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i feel it's better for this kind of tools to handle more cases as far as it's legal and easy.
anyway your comment isn't specific to this PR, right?

Copy link
Member

@MaskRay MaskRay Aug 22, 2024

Choose a reason for hiding this comment

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

Yes, the comment is specific to this PR.

RelocationResolver is to handle static relocations. While R_NONE might be defined by a psABI, in practice it's always a toolchain bug when RelocationResolver has to resolve R_NONE. We should not handle unnecessary relocations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the comment is specific to this PR.

RelocationResolver is to handle static relocations. While R___NONE might be defined by a psABI, in practice it's always a toolchain bug when RelocationResolver has to resolve R___NONE. We should not handle unnecessary relocations.

do you mean that for some reasons your rationale is specific to xtensa (this PR)
and doesn't apply to other non-xtensa relocations like R_X86_64_NONE?

return LocData;
case ELF::R_XTENSA_32:
return (S + Addend + LocData) & 0xFFFFFFFF;
default:
llvm_unreachable("Invalid relocation type");
}
}

static bool supportsPPC32(uint64_t Type) {
switch (Type) {
case ELF::R_PPC_ADDR32:
Expand Down Expand Up @@ -820,6 +842,8 @@ getRelocationResolver(const ObjectFile &Obj) {
switch (Obj.getArch()) {
case Triple::x86:
return {supportsX86, resolveX86};
case Triple::xtensa:
return {supportsXtensa, resolveXtensa};
case Triple::ppcle:
case Triple::ppc:
return {supportsPPC32, resolvePPC32};
Expand Down Expand Up @@ -885,11 +909,12 @@ uint64_t resolveRelocation(RelocationResolver Resolver, const RelocationRef &R,

if (GetRelSectionType() == ELF::SHT_RELA) {
Addend = getELFAddend(R);
// LoongArch and RISCV relocations use both LocData and Addend.
// LoongArch, RISCV, and Xtensa relocations use both LocData and Addend.
if (Obj->getArch() != Triple::loongarch32 &&
Obj->getArch() != Triple::loongarch64 &&
Obj->getArch() != Triple::riscv32 &&
Obj->getArch() != Triple::riscv64)
Obj->getArch() != Triple::riscv64 &&
Obj->getArch() != Triple::xtensa)
LocData = 0;
}
}
Expand Down
93 changes: 93 additions & 0 deletions llvm/test/tools/llvm-dwarfdump/Xtensa/xtensa-relocs.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
# Tests Xtensa relocations. We provide a .debug_info section with multiple
# DW_AT_high_pc entries (that's one of the attributes for which relocations are
# resolved by llvm-dwarfdump) and we add a relocation for each of them.
#
# RUN: yaml2obj %s | llvm-dwarfdump - | FileCheck %s

# To add more tests you need to modify the Content of the .debug_abbrev and
# .debug_info sections. To do that create a test.s which matches the assembly
# code below, run the command that follows and copy over the "Content" value of
# the respective sections:
Comment on lines +7 to +10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps the simplest thing to do would be to have only a .debug_addr section with some relocatable addresses in it?

Could the assembly actually include the symbol references that would generate the relocations (can llvm-mc generate these relocations?) so they don't have to be hand-crafted in the IR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps the simplest thing to do would be to have only a .debug_addr section with some relocatable addresses in it?

i'm not familiar enough with dwarf to say if it's simpler/reasonable.

Could the assembly actually include the symbol references that would generate the relocations (can llvm-mc generate these relocations?) so they don't have to be hand-crafted in the IR?

i'm sure the espressif fork of the llvm can produce R_XTENSA_32.
but i thought xtensa support in the stock llvm was somehow incomplete.

#
# ```
# $ cat test.s
# .section .debug_abbrev,"",@progbits
# .byte 1 # Abbreviation Code
# .byte 0x11 # DW_TAG_compile_unit
# .byte 0 # DW_CHILDREN_no
#
# # Add a DW_AT_high_pc for each relocation we test.
# .rept 2 # (UPDATE HERE)
# .byte 0x12 # DW_AT_high_pc
# .byte 0x01 # DW_FORM_addr
# .endr
#
# .byte 0 # EOM(1)
# .byte 0 # EOM(2)
# .byte 0 # EOM(3)
#
# .section .debug_info,"",@progbits
# .4byte 2+4+1+1+4*2 # Length of Unit (UPDATE HERE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

could use assembly labels to compute the length here to make it easier to update the test in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

# .2byte 4 # DWARF version number
# .4byte .debug_abbrev # Offset Into Abbrev. Section
# .byte 4 # Address Size (in bytes)
# .byte 1 # Abbrev 1
# .4byte 0x00000042 # Test 1
# .4byte 0x00000042 # Test 2
# $ llvm-mc test.s -filetype obj -triple xtensa -o - | obj2yaml
# ```
#
# Note that you may need xtensa-enabled llvm-mc.
# I used a version from https://github.com/espressif/llvm-project.

--- !ELF
FileHeader:
Class: ELFCLASS32
Data: ELFDATA2LSB
Type: ET_REL
Machine: EM_XTENSA
SectionHeaderStringTable: .strtab
Sections:
- Name: .text
Type: SHT_PROGBITS
Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
AddressAlign: 0x4
- Name: .debug_abbrev
Type: SHT_PROGBITS
AddressAlign: 0x1
Content: '01110012011201000000'
- Name: .debug_info
Type: SHT_PROGBITS
AddressAlign: 0x1
Content: '1000000004000000000004014200000042000000'
- Name: .rela.debug_info
Type: SHT_RELA
Flags: [ SHF_INFO_LINK ]
Link: .symtab
AddressAlign: 0x4
Info: .debug_info
Relocations:

# Test 1
# 0x42 with R_XTENSA_NONE(0x1) = 0x42
# CHECK: DW_AT_high_pc (0x00000042)
- Offset: 0x0000000C # 0xC + 4*0
Symbol: v1
Type: R_XTENSA_NONE

# Test 2
# 0x42 with R_XTENSA_32(0xFFFFFFFF) = 0x00000041
# CHECK-NEXT: DW_AT_high_pc (0x00000041)
- Offset: 0x00000010 # 0xC + 4*1
Symbol: vFFFFFFFF
Type: R_XTENSA_32

Symbols:
- Name: v1
Type: STT_SECTION
Section: .debug_info
Value: 0x00000001
- Name: vFFFFFFFF
Type: STT_SECTION
Section: .debug_info
Value: 0xFFFFFFFF
Loading