[PATCH] D76352: [llvm-readobj] Derive dynamic symtab size from hash table

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 12 inline comments as done.
jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:572
+
+## b) Table size from DT_HASH does not match size from section header.
+# RUN: yaml2obj --docnum=14 %s -o %t10b-smaller -DCHAIN="[1, 2]"
----------------
grimar wrote:
> 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).
I've put the comment by the YAML, which is what describes the section header.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:575
+# RUN: llvm-readobj --dyn-symbols %t10b-smaller 2>&1 | \
+# RUN:   FileCheck %s --check-prefixes=LLVM-HASHB,HASHB-WARN \
+# RUN:              --implicit-check-not=warning -DNCHAIN=2
----------------
jhenderson wrote:
> grimar wrote:
> > `LLVM-HASHB` -> `LLVM-HASHB2`? (a bit more consistent with LLVM-HASHB4)
> > `HASHB-WARN` -> `WARN`? (shorter)
> 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.
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.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5924
   }
 }
 
----------------
grimar wrote:
> 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);
> ```
This won't work because getSymbolSectionName also handles the special section indexes (SHN_UNDEF, SHN_ABS etc).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76352/new/

https://reviews.llvm.org/D76352