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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test:313
+
+# LLVM3:      DynamicSymbols [
+# LLVM3: ]
----------------
Not aligned?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:444
+# RUN: llvm-readobj --dyn-symbols %t10a1-64 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=LLVM-HASHA --implicit-check-not=warning
+# RUN: llvm-readelf --dyn-symbols %t10a1-64 2>&1 | \
----------------
`--implicit-check-not=warning:`

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


================
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:
> 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?
> Why there is no -DBITS=32 case like above?

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


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2175
+  // and the location we find here should match.
+  if (DynSymRegion && TempDynSymRegion &&
+      TempDynSymRegion->Addr != DynSymRegion->Addr)
----------------
The two `if` can be guarded by an outer `if (DynSymRegion) { ... }`

We may add a DT_GNU_HASH check in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76352