[PATCH] D87613: [llvm-readelf/obj] - Print section symbol names properly when dumping relocations.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

There seems to be more going on in this patch than the description talks about. In particular, it looks like this patch causes relocations to be printed when they weren't before. I don't think this is necessarily a problem, but it needs explaining in the description at least, and if sensibly practical, should actually be a separate patch, to minimise behaviour changes per patch.



================
Comment at: llvm/test/tools/llvm-readobj/ELF/relocation-errors.test:71
         Type:   R_X86_64_NONE
-## Case 4: Test a relocation against a section symbol that has an invalid
-##         sh_name offset that goes past the end of the section name string table.
+## Case 4: Test a relocation against a unnamed section symbol that has an invalid
+##         section index (larger than the number of sections).
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/relocation-errors.test:76-77
+        Type:   R_X86_64_NONE
+## Case 4: Test a relocation against a named section symbol that has an invalid
+##         sh_name offset that goes past the end of the section name string table.
+      - Offset: 0x5
----------------
I think you need to rephrase this and the similar comment below - section symbols (like any other symbol) don't have "sh_name" fields, and therefore this comment is confusing.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/relocation-errors.test:81-82
         Type:   R_X86_64_NONE
-## Case 5: Test a relocation in a section that is linked to a symbol table that
+## Case 5: Test a relocation against a unnamed section symbol that has an invalid
+##         sh_name offset that goes past the end of the section name string table.
+      - Offset: 0x6
----------------



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1175-1178
+    const object::ELFFile<ELFT> &Obj = *ObjF->getELFFile();
+    const Elf_Shdr *SymTab = IsDynamic ? DotDynsymSec : DotSymtabSec;
+    Elf_Sym_Range Syms =
+        unwrapOrError(ObjF->getFileName(), Obj.symbols(SymTab));
----------------
If I'm following this correctly, there's an unrelated behaviour change here to do with which symbol table to use when `IsDynamic` is `true`? That sounds like a different patch (also possibly unnecessary, since I'm not sure a STT_SECTION symbol in a dynsym makes sense, but that's possibly tangential). 


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1187
+
+    // A section symbol describes the section at index 0.
+    if (*SectionIndex == 0)
----------------
I think you can delete this comment. It just describes what the code says.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1200
     }
-    Expected<StringRef> NameOrErr = getSymbolSectionName(Symbol, *SectionIndex);
-    if (!NameOrErr) {
----------------
I'm concerned that not using this function is a behaviour change here too? Note that `getSymbolSectionName` does more than just look up the section's name - there's special behaviour for e.g. common and absolute symbols, which the new code doesn't seem to be accounting for?


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

https://reviews.llvm.org/D87613