This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] Derive dynamic symtab size from hash table
ClosedPublic

Authored by jhenderson on Mar 18 2020, 3:47 AM.

Details

Summary

If the section headers have been removed by a tool such as llvm-objcopy or llvm-strip, previously llvm-readobj/llvm-readelf would not dump the dynamic symbols when --dyn-symbols was specified. However, the nchain value of the DT_HASH data specifies the number of dynamic symbols, so if it is present, we can use that. This patch implements this behaviour.

Fixes https://bugs.llvm.org/show_bug.cgi?id=45089.

Diff Detail

Event Timeline

jhenderson created this revision.Mar 18 2020, 3:47 AM

Fix unchecked Expected.

grimar added inline comments.Mar 18 2020, 5:49 AM
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
455

dervied -> derived

462

Why there is no -DBITS=32 case like above?

572

Could you expand the comment to mention we have 4 dynamic symbols (with null) in the object,
but we set the size of .dynsym so that it says we have 3 (it is not really obvious I think).

575

LLVM-HASHB -> LLVM-HASHB2? (a bit more consistent with LLVM-HASHB4)
HASHB-WARN -> WARN? (shorter)

581

Should we test -DCHAIN="[1, 2, 3]" to verify there is no warning?

715

--docnum=15

Should we move all the test you've added to a new test case? e.g. dyn-symbols-no-section-table.test?

725

Perhaps you can reuse the --docnum-13? e.g. if you reorder tags:

Entries:
  - Tag:   DT_STRTAB
    Value: 0x800
  - Tag:   DT_STRSZ
    Value: 9
  - Tag:   [[TAG1]]
    Value: [[ADDR1]]
  - Tag:   [[TAG2]]
    Value: [[ADDR2]]
  - Tag:   DT_NULL
    Value: 0

You should probably be able to -DTAG1=DT_SYMTAB -DTAG2=DT_NULL instead of -DTAG1=DT_SYMTAB -DTAG2=DT_HASH`

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

I'd rename somehow to remove Temp prefix.

2177

Should we just use reportUniqueWarning everywhere for the new code, even if it is not really needed?
That might help to avoid copy-paste issues in future probably.

2182

"conflicts with the number of symbols ... " I think, not with the size of the dynamic symbol table.

I'd also add something like "In according to the ELF specification, the number of symbol table entries should equal nchain"
because I think it was never said in comments explicitly.

2208

You do not need curly bracers around a single code line.

3725

Looks like an independent improvement?

grimar added inline comments.Mar 18 2020, 5:57 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
5924

Perhaps, the following piece looks slightly better.

StringRef Name= "<?>";
if (!this->dumper()->getElfObject()->sections().empty()) {
  if (Expected<StringRef> SectionName =
      this->dumper()->getSymbolSectionName(Symbol, *SectionIndex))
  Name = *SectionName;
else
  this->reportUniqueWarning(SectionName.takeError());
}

W.printHex("Section", Name, *SectionIndex);
jhenderson marked 7 inline comments as done.Mar 19 2020, 4:04 AM
jhenderson added inline comments.
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
462

I didn't think it was necessary. We've confirmed with the first 32/64 bit case that we can read the nchain value. After that, there's no difference in behaviour beyond what is already tested in the generic test cases.

575

I'm reluctant to do either of these.

LLVM-HASHB is used by both the 2 and 4 case. Having the 4 case use one called "HASHB2" would be potentially confusing.

HASHB-WARN gives context to this warning check, and prevents confusion if somebody wanted to add a check for a different warning in this test.

715

I originally considered that, but the b) cases have a section table, so it didn't make sense to call it that. I guess dyn-symbols-size-from-hash.test might work?

725

Good idea, thanks.

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

This warning has just been moved from above. I'm happy to change it as I move it though, if you still want?

3725

The "Symbol table for image" part is almost dead code prior to my patch. This function is only called in one place, and there the only time this can be hit is if either the SHT_SYMTAB/SHT_DYNSYM section name is empty. If you attempted to dump the dynamic symbols for a stripped object prior to my change, the early out in the calling function stops this code being executed.

If you still want me to, I can go ahead and move it into a separate patch, but I'm not sure there's much benefit to it (I'd either have to write a test for a symbol table with no name, or modify the new test cases I'm adding in the next patch).

jhenderson marked 12 inline comments as done.Mar 19 2020, 4:58 AM
jhenderson added inline comments.
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
572

I've put the comment by the YAML, which is what describes the section header.

575

The WARN rename makes sense after splitting out the new tests.

I've redone the LLVM-HASHB case, since I've added the third test case for this situation now.

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

This won't work because getSymbolSectionName also handles the special section indexes (SHN_UNDEF, SHN_ABS etc).

grimar added inline comments.Mar 19 2020, 4:58 AM
llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
462

OK. Perhaps mention this in comment for other readers?

575

OK

715

Sound good.

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

I think it is OK to change while you are here anyways.

3725

Lets keep it.
I think I slightly misread this code, I've remembered this piece as the no-op cleanup, but today I see it is not (sorry).

jhenderson marked 8 inline comments as done.

Address review comments.

MaskRay added inline comments.Mar 19 2020, 12:39 PM
llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test
313 ↗(On Diff #251380)

Not aligned?

llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test
444

--implicit-check-not=warning:

(It is very unlikely that %t10a1-64 may contain warning:)

462

Why there is no -DBITS=32 case like above?

Already covered. No need to add another -DBITS=32 case...

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

The two if can be guarded by an outer if (DynSymRegion) { ... }

We may add a DT_GNU_HASH check in the future.

jhenderson marked 3 inline comments as done.

Address review comments from @MaskRay.

Add colons to --implicit-check-not=warning (missed because they didn't appear in the diff, as they were in the old test).

jhenderson added inline comments.Mar 20 2020, 3:42 AM
llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test
313 ↗(On Diff #251380)

Thanks: copy and paste error.

Harbormaster completed remote builds in B49859: Diff 251592.
MaskRay accepted this revision.Mar 20 2020, 10:38 AM
This revision is now accepted and ready to land.Mar 20 2020, 10:38 AM

from hash table

Using DT_HASH may resolve ambiguity here, because there at least 2 flavors of hash tables (I used "at least" because GNU OSABI has DT_MIPS_XHASH. I started a thread about dropping it. dalias thinks this is doable https://sourceware.org/pipermail/binutils/2020-January/109362.html I hope we won't add it.)

This revision was automatically updated to reflect the committed changes.