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

[George Rimar via Phabricator via llvm-commits]

grimar 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
+
----------------
jhenderson wrote:
> 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.
OK. Perhaps mention this in comment for other readers?


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


================
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
----------------
jhenderson wrote:
> 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?
Sound good.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2177
+      TempDynSymRegion->Addr != DynSymRegion->Addr)
+    reportWarning(
+        createError("SHT_DYNSYM section header and DT_SYMTAB disagree about "
----------------
jhenderson wrote:
> 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?
I think it is OK to change while you are here anyways.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76352