[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:455
+
+## a2) Table size is dervied from hash table, with DT_HASH before DT_SYMTAB.
+# RUN: yaml2obj --docnum=13 %s -o %t10a2-64 -DBITS=64 \
----------------
dervied -> derived 


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


================
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]"
----------------
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).


================
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
----------------
`LLVM-HASHB` -> `LLVM-HASHB2`? (a bit more consistent with LLVM-HASHB4)
`HASHB-WARN` -> `WARN`? (shorter)


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dyn-symbols.test:581
+
+# RUN: yaml2obj --docnum=14 %s -o %t10b-larger -DCHAIN="[1, 2, 3, 4]"
+# RUN: llvm-readobj --dyn-symbols %t10b-larger 2>&1 | \
----------------
Should we test `-DCHAIN="[1, 2, 3]"` to verify there is no warning?


================
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
----------------
> --docnum=15

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


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


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2064
   uint64_t StringTableSize = 0;
+  Optional<DynRegionInfo> TempDynSymRegion;
   for (const Elf_Dyn &Dyn : dynamic_table()) {
----------------
I'd rename somehow to remove `Temp` prefix.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2177
+      TempDynSymRegion->Addr != DynSymRegion->Addr)
+    reportWarning(
+        createError("SHT_DYNSYM section header and DT_SYMTAB disagree about "
----------------
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.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2182
+
+  // Check to see if the hash table nchain value conflicts with the size of
+  // the dynamic symbol table according to the section header.
----------------
"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.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2208
+    DynSymRegion->Size = HashTable->nchain * DynSymRegion->EntSize;
+  }
 }
----------------
You do not need curly bracers around a single code line.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3725
+    OS << "\nSymbol table for image";
+  OS << " contains " << Entries << " entries:\n";
 
----------------
Looks like an independent improvement?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76352