[PATCH] D67078: [llvm-readelf] - Allow dumping the dynamic symbols when there is no program headers.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: test/tools/llvm-readobj/dyn-symbols.test:220
+
+## Check that when we can't map the value of the DT_SYMTAB tag to address, we report a warning and
+## use the information in the section header table to locate the dynamic symbol table.
----------------
to address -> to an address


================
Comment at: test/tools/llvm-readobj/dyn-symbols.test:251
+
+## Check that if we can get the location of dynamic symbol table using both the DT_SYMTAB value
+## and the section headers table then we prefer the former and report a warning.
----------------
of dynamic -> of the dynamic


================
Comment at: test/tools/llvm-readobj/dyn-symbols.test:274-275
+        Value: 0
+  - Name: .dynsym
+    Type: SHT_DYNSYM
+  - Name: .mydynsym
----------------
Won't this be implicitly generated?


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1647
+    case ELF::DT_SYMTAB: {
+      // Often we find the information about the dynamic symbols section
+      // location in the SHT_DYNSYM section header. However, the value in
----------------
dynamic symbols section -> dynamic symbol table

(technically, if there are no section headers, there's no dynamic symbol section)


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1649
+      // location in the SHT_DYNSYM section header. However, the value in
+      // DT_SYMTAB has a priority, because it is used by dynamic loaders to
+      // locate .dynsym in runtime. Normally the location we find in the
----------------
has a priority -> has priority


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1650
+      // DT_SYMTAB has a priority, because it is used by dynamic loaders to
+      // locate .dynsym in runtime. Normally the location we find in the
+      // section header and the location we find here should match.
----------------
in runtime -> at runtime

Normally the -> The
(should implies normally)


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1652
+      // section header and the location we find here should match.
+      // If we can't map the DT_SYMTAB value to address (e.g. when there are no
+      // program headers), we ignore its value.
----------------
to address -> to an address


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1655
+      if (const uint8_t *VA = toMappedAddr(Dyn.getTag(), Dyn.getPtr())) {
+        if (DynSymRegion.EntSize && VA != DynSymRegion.Addr)
+          reportWarning(
----------------
Using `DynSymRegion.EntSize` to determine if the dynamic symbol table has been found via a section header feels a bit weird to me. Please at least add a comment.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:1659
+                  "SHT_DYNSYM section header and DT_SYMTAB disagree about "
+                  "the location of dynamic symbol table"),
+              ObjF->getFileName());
----------------
of dynamic -> of the dynamic


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

https://reviews.llvm.org/D67078