[PATCH] D75756: [llvm-objdump] Teach `llvm-objdump` dump dynamic symbols.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/elf-dynamic-symbol.test:54
+# RUN: llvm-objdump --dynamic-syms %t2 | FileCheck %s --check-prefix=NODYNSYM
+# RUN: llvm-objdump -T %t2 | FileCheck %s --check-prefix=NODYNSYM
+--- !ELF
----------------
Add an empty line before `--- !ELF` please.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1855
 void printSymbolTable(const ObjectFile *O, StringRef ArchiveName,
-                      StringRef ArchitectureName) {
-  outs() << "SYMBOL TABLE:\n";
+                      StringRef ArchitectureName, bool DumpDynamicSymbol) {
+  auto I = O->symbol_begin();
----------------
`DumpDynamicSymbol` -> `IsDynamic` (Used commonly ind shorter).


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1857
+  auto I = O->symbol_begin();
+  auto E = O->symbol_end();
+
----------------
It is a bit strange when the code assigns a value to a variable and then reassign it right below.

I.e. why not do something like below?


```
  if (const COFFObjectFile *Coff = dyn_cast<const COFFObjectFile>(O)) {
    outs() << "SYMBOL TABLE:\n";
    printCOFFSymbolTable(Coff);
    return;
  }
.......
llvm::basic_symbol_iterator I, E;
if (!DumpDynamicSymbol) {
   I = O->symbol_begin();
   E = O->symbol_end();
   outs() << "SYMBOL TABLE:\n";
...
} else {
   I = ELF->getDynamicSymbolIterators().begin();
   E = ELF->getDynamicSymbolIterators().end();
   outs() << "DYNAMIC SYMBOL TABLE:\n"
...
}
...
```


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1866
+          errs(), ToolName,
+          "This operation is only currently supported for ELF object files.\n");
+      return;
----------------
This error is untested. Also, perhaps you could use `reportWarning` instead.
(Does not seem we want to stop dump here).


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1870
+
+    const ELFObjectFileBase *ELF = dyn_cast<const ELFObjectFileBase>(O);
+    I = ELF->getDynamicSymbolIterators().begin();
----------------
`dyn_cast` will return `nullptr` when an object is not `ELF`. Since you know it is always an `ELF` object at this point, you should use `cast`.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1874
+    if (I != E)
+      ++I; // Skip first meaningless symbol.
+    else {
----------------
Non dynamic symbol table also has a null symbol. Why we do not need it for `!DumpDynamicSymbol`? (Or: why do we need it here?).


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1939
+            ? 'D'
+            : (Type == SymbolRef::ST_Debug || Type == SymbolRef::ST_File) ? 'd'
+                                                                          : ' ';
----------------
It seems that when we are consistent with the code around, it looks a bit simpler:

```
char Debug = ' ';
if (DumpDynamicSymbol)
 Debug = 'D';
else if (Type == SymbolRef::ST_Debug || Type == SymbolRef::ST_File)
 Debug = 'd';
```


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2256
+  if (DynamicSymbolTable)
+    printSymbolTable(O, ArchiveName, StringRef(), /*DumpDynamicSymbol=*/true);
   if (DwarfDumpType != DIDT_Null) {
----------------
`StringRef()` -> `/*ArchitectureName=*/=""`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75756