Page MenuHomePhabricator

[llvm-objdump] Fix dumping dynamic relative relocations for SHT_REL
ClosedPublic

Authored by arichardson on Apr 14 2021, 8:48 AM.

Details

Summary

Previously printing R_386_RELATIVE relocations would trigger
error: can't read an entry at 0x40: it goes past the end of the section (0x40)
I found this while writing a test case for LLD (D100490)

Diff Detail

Event Timeline

arichardson created this revision.Apr 14 2021, 8:48 AM
arichardson published this revision for review.Apr 14 2021, 9:08 AM
arichardson edited the summary of this revision. (Show Details)
arichardson added a reviewer: grimar.
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2021, 9:09 AM
arichardson retitled this revision from [UPSTREAM][llvm-readobj] Fix dumping dynamic relative relocations for SHT_REL to [llvm-readobj] Fix dumping dynamic relative relocations for SHT_REL.
MaskRay added inline comments.Apr 14 2021, 12:08 PM
llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test
2

This looks verbose.

If you just keep things as in test/tools/llvm-readelf/ELF/reloc-types-elf-i386.test, does it work?
llvm-readobj generally has better support for ELF dumping and you may take inspiration from its tests.

136

foo is not needed.

arichardson planned changes to this revision.Apr 14 2021, 12:29 PM
arichardson added inline comments.
llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test
2

Yes, I should have reduced this test input further, right now it's just and obj2yaml of an lld test with a few obvious lines removed. Will remove everything that's not needed for the relative relocation tomorrow.

  • simplify test case
arichardson marked an inline comment as done.

Improve test

Hopefully the new test is better?

llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test
2

I could add a RELATIVE reloc to a .o file as in test/tools/llvm-readelf/ELF/reloc-types-elf-i386.test, but I thought using a dynamic relocation like llvm/test/tools/llvm-readobj/ELF/dynamic-reloc.test makes more sense.
The latest version also checks both relocations with and without a symbol (the same as llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs.test)

  • remove debug RUN: line
arichardson added inline comments.Apr 19 2021, 2:25 AM
llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test
136

I've kept foo to make this test closer to llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs.test and test both relocations with and without a symbol.

The patch title tags llvm-readobj, but this is an llvm-objdump change.

llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test
2–4

I don't think you need the "Previously ..." sentence at all. What you probably do want to say is that this test shows that Elf_Rel relocations can be dumped for both cases with and without a corresponding symbol, something like the inline edit.

10

I don't think this line is useful?

12–13

Nit suggestion to make things line up nicely.

62–64

I think you can omit the static symbol?

arichardson retitled this revision from [llvm-readobj] Fix dumping dynamic relative relocations for SHT_REL to [llvm-objdump] Fix dumping dynamic relative relocations for SHT_REL.Apr 19 2021, 6:21 AM
arichardson added inline comments.Apr 19 2021, 6:27 AM
llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test
10

I agree. I simply copied it from llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs.test. Should I drop it there too?

62–64

Unfortunately that gives me a yaml2obj error when using Symbol: foo inside Relocations:.
I haven't looked at the yaml2obj code, but it seems like we would need to either hardcode dynamic relocation section names or add another attribute to Relocations to indicate that it should look in the dynamic symbol list instead. Alternatively, we could make that decision based on the file type (ET_REL uses Symbols, all other use DynamicSymbols?

jhenderson added inline comments.Apr 19 2021, 6:45 AM
llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test
10

It probably wouldn't hurt (just make sure that somewhere there is a lit test that shows that the file format line is printed, so that we don't lose coverage!)

32

You probably should have Link: .dynsym here, since these are dynamic relocations (by default, it'll use the static symbol table).

62–64

Ah okay, thanks. Try the Link: suggestion above. If that doesn't work, I think adding a FIXME/TODO comment of some kind here would be a good idea.

@grimar, any thoughts?

arichardson marked 8 inline comments as done.

Address review feedback from @jhenderson

llvm/test/tools/llvm-objdump/X86/elf-dynamic-relocs-rel.test
10

Looks like it's tested by llvm/test/tools/llvm-objdump/ELF/file-headers.test, so I have dropped it in this version.

62–64

Thanks, adding Link: allows me to drop Symbols:.

jhenderson accepted this revision.Apr 20 2021, 1:55 AM

Thanks. LGTM, but please give @MaskRay a chance to confirm he's happy. Also, probably worth updating the description to mention the tidying/fixing up of elf-dynamic-relcos.test too. Alternatively, make that change in a different commit.

This revision is now accepted and ready to land.Apr 20 2021, 1:55 AM
MaskRay accepted this revision.Apr 27 2021, 10:33 AM