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

[James Henderson via Phabricator via llvm-commits]

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


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:462
+# RUN: llvm-readelf --dyn-symbols %t10a2-64 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=GNU-HASHA --implicit-check-not=warning
+
----------------
grimar wrote:
> Why there is no `-DBITS=32` case like above?
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.


================
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
----------------
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.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:715
+## c) DT_HASH is missing (table size treated as 0).
+# RUN: yaml2obj --docnum=15 %s -o %t10c
+# RUN: llvm-strip --strip-sections %t10c
----------------
grimar wrote:
> > --docnum=15
> 
> Should we move all the test you've added to a new test case? e.g. `dyn-symbols-no-section-table.test`?
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?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:725
+
+--- !ELF
+FileHeader:
----------------
grimar wrote:
> 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``
Good idea, thanks.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2177
+      TempDynSymRegion->Addr != DynSymRegion->Addr)
+    reportWarning(
+        createError("SHT_DYNSYM section header and DT_SYMTAB disagree about "
----------------
grimar wrote:
> 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.
This warning has just been moved from above. I'm happy to change it as I move it though, if you still want?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3725
+    OS << "\nSymbol table for image";
+  OS << " contains " << Entries << " entries:\n";
 
----------------
grimar wrote:
> Looks like an independent improvement?
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76352