This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readelf/obj] - Add support of multiple SHT_SYMTAB_SHNDX sections.
ClosedPublic

Authored by grimar on Dec 9 2020, 2:20 AM.

Details

Summary

Currently we don't support multiple SHT_SYMTAB_SHNDX sections
and the DT_SYMTAB_SHNDX tag currently.

This patch implements it and fixes the
https://bugs.llvm.org/show_bug.cgi?id=43991.

I had to introduce the struct DataRegion to ELF.h,
it is used to represent a region that might have no known size.
It is needed, because we don't know the size of the extended
section indices table when it is located via DT_SYMTAB_SHNDX.
In this case we still want to validate that we don't read
past the end of the file.

Diff Detail

Event Timeline

grimar created this revision.Dec 9 2020, 2:20 AM
grimar requested review of this revision.Dec 9 2020, 2:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2020, 2:20 AM

Ping. Any comments about this approach?

Sorry, this somehow slipped off my radar, and I've run out of time to do any further significant reviewing today. I've skimmed it briefly, and the general approach looks fine. I'd like ideally to find a way to avoid needing to pass both a pointer and size around for the shndx table, even if it's just to wrap it in a new class, or change to Optional<ArrayRef<...>> or something. If you'd like me to take a more detailed look tomorrow, I can certainly do so.

grimar planned changes to this revision.Dec 15 2020, 1:34 AM

Sorry, this somehow slipped off my radar, and I've run out of time to do any further significant reviewing today. I've skimmed it briefly, and the general approach looks fine. I'd like ideally to find a way to avoid needing to pass both a pointer and size around for the shndx table, even if it's just to wrap it in a new class, or change to Optional<ArrayRef<...>> or something.

Thanks for looking! I am planning to add missing test cases and will update the patch. We can discuss the possible change of signatures after that.
Initially the code had ArrayRef<Elf_Word> ShndxTable. In theory we can use it, but then we will have an empty (size == 0) ArrayRef with a non-zero data.
I am just not sure that doing it looks nice.

Sorry, this somehow slipped off my radar, and I've run out of time to do any further significant reviewing today. I've skimmed it briefly, and the general approach looks fine. I'd like ideally to find a way to avoid needing to pass both a pointer and size around for the shndx table, even if it's just to wrap it in a new class, or change to Optional<ArrayRef<...>> or something. If you'd like me to take a more detailed look tomorrow, I can certainly do so.

Sorry, this somehow slipped off my radar, and I've run out of time to do any further significant reviewing today. I've skimmed it briefly, and the general approach looks fine. I'd like ideally to find a way to avoid needing to pass both a pointer and size around for the shndx table, even if it's just to wrap it in a new class, or change to Optional<ArrayRef<...>> or something.

Thanks for looking! I am planning to add missing test cases and will update the patch. We can discuss the possible change of signatures after that.
Initially the code had ArrayRef<Elf_Word> ShndxTable. In theory we can use it, but then we will have an empty (size == 0) ArrayRef with a non-zero data.
I am just not sure that doing it looks nice.

Hmmm... that's a fair point. Maybe we should write our own class that simply wraps the pointer and size, and it could do the bounds checking itself, and will therefore know that 0 is special for size somehow?

Sorry, this somehow slipped off my radar, and I've run out of time to do any further significant reviewing today. I've skimmed it briefly, and the general approach looks fine. I'd like ideally to find a way to avoid needing to pass both a pointer and size around for the shndx table, even if it's just to wrap it in a new class, or change to Optional<ArrayRef<...>> or something. If you'd like me to take a more detailed look tomorrow, I can certainly do so.

Sorry, this somehow slipped off my radar, and I've run out of time to do any further significant reviewing today. I've skimmed it briefly, and the general approach looks fine. I'd like ideally to find a way to avoid needing to pass both a pointer and size around for the shndx table, even if it's just to wrap it in a new class, or change to Optional<ArrayRef<...>> or something.

Thanks for looking! I am planning to add missing test cases and will update the patch. We can discuss the possible change of signatures after that.
Initially the code had ArrayRef<Elf_Word> ShndxTable. In theory we can use it, but then we will have an empty (size == 0) ArrayRef with a non-zero data.
I am just not sure that doing it looks nice.

Hmmm... that's a fair point. Maybe we should write our own class that simply wraps the pointer and size, and it could do the bounds checking itself, and will therefore know that 0 is special for size somehow?

This might look good. I'll try. Instead of size == 0 it probably can have Optional<> size.

grimar updated this revision to Diff 312767.Dec 18 2020, 5:28 AM
grimar retitled this revision from [llvm-readelf/obj][WIP] - Add support for multiple SHT_SYMTAB_SHNDX sections. to [llvm-readelf/obj] - Add support of multiple SHT_SYMTAB_SHNDX sections..
grimar edited the summary of this revision. (Show Details)
  • Added test cases.
  • Implemented helper DataRegion struct as discussed.

Note: if struct DataRegion is fine, then I'll add unit tests for it.
I believe It is impossible to trigger one of errors it might report
from the llvm-readelf/obj code.

jhenderson added inline comments.Jan 6 2021, 1:26 AM
llvm/include/llvm/Object/ELF.h
74

Maybe: "the index N is greater than..." (where N is the specified index)?

llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test
74

I found the original wording a little hard to follow initially. This phrasing would make it easier.

112
134–135
171–172
198
199

We probably also need a case for two linked to .symtab, where the dynamic tag isn't used, to show which is picked.

234
235
240

You only use the second docnum once. Can you get rid of the macro?

llvm/tools/llvm-readobj/ELFDumper.cpp
6588

etc, for consistency with the method name.

grimar updated this revision to Diff 316021.Jan 12 2021, 1:33 AM
grimar marked 11 inline comments as done.
  • Addressed review comments.
  • Added the unit test.
llvm/include/llvm/Object/ELF.h
74

For the message below I report the index value on the caller side (see getExtendedSymbolTableIndex).
Probably worth to be consistent here.

llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test
240

Yes, done.

jhenderson added inline comments.Jan 12 2021, 1:46 AM
llvm/include/llvm/Object/ELF.h
75
llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test
200

You didn't follow my fix properly here :)

llvm/unittests/Object/ELFTest.cpp
66–69 ↗(On Diff #316021)

Could this be:

ASSERT_THAT_EXPECTED(ValOrErr, Succeeded());
EXPECT_EQ(*ValOrErr, I);

Or something to that effect? It seems simpler, and if the good index-value cases don't work, there doesn't seem to be much point in worrying about the rest of the test case running, so asserting is fine.

grimar updated this revision to Diff 316066.Jan 12 2021, 5:03 AM
grimar marked 3 inline comments as done.
  • Rebased after landing the "[llvm-readef/obj] - Change the design structure of ELF dumper. NFCI." (D93900).
  • Addressed review comments.
llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test
200

Oops, sorry. Fixed.

llvm/unittests/Object/ELFTest.cpp
66–69 ↗(On Diff #316021)

Yes, it is possible to write in this way here. Done.

jhenderson accepted this revision.Jan 12 2021, 6:02 AM

LGTM.

llvm/unittests/Object/ELFTest.cpp
66–69 ↗(On Diff #316021)

Actually, does the ASSERT_THAT_EXPECTED work (i.e. stop the test) if the Expected is in a failure state here? Just slightly concerned given that it's in a lambda. If it doesn't, feel free to go back to the previous version.

This revision is now accepted and ready to land.Jan 12 2021, 6:02 AM
grimar marked an inline comment as done.Jan 12 2021, 6:09 AM
grimar added inline comments.
llvm/unittests/Object/ELFTest.cpp
66–69 ↗(On Diff #316021)

Yes, it works fine here, I've checked it. It doesn't stop the test, but it reports a failure and returns from the labda,
what is OK here.

If we wanted to return some value from the labda or if we needed to terminate the test then it wouldn't work.

This revision was automatically updated to reflect the committed changes.
grimar marked an inline comment as done.