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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Some thoughts on this, which don't need to be addressed in this patch, but should be in follow-on ones if necessary:

1. Does GNU objdump use the section name or section type to identify the section? What happens if there are multiple SHT_DYNSYM sections? What about if ".dynsym" is not SHT_DYNSYM?
2. What should we do if the section header table is missing (e.g. due to llvm-objcopy --strip-sections)? See llvm-readobj bugs https://bugs.llvm.org/show_bug.cgi?id=45089 and https://bugs.llvm.org/show_bug.cgi?id=45090 for suggestions.



================
Comment at: llvm/test/tools/llvm-objdump/ELF/elf-dynamic-symbol.test:1
+## Test that llvm-objdump can dump dynamic symbols.
+
----------------
No need for "elf-" in the test file name.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/elf-dynamic-symbol.test:4-5
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-objdump --dynamic-syms %t1 | FileCheck %s --check-prefix=DYNSYM
+# RUN: llvm-objdump -T %t1 | FileCheck %s --check-prefix=DYNSYM
+
----------------
It would be a good idea to use --strict-whitespace and --match-full-lines in the FileCheck calls.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/elf-dynamic-symbol.test:9-12
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_DYN
+  Machine:         EM_X86_64
----------------
Here and below, remove the excessive spacing, so that the values line up, but with only a single space between `Machine:` and its value.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/elf-dynamic-symbol.test:14-16
+  - Name: .data
+    Type: SHT_PROGBITS
+    Flags: [ SHF_ALLOC, SHF_WRITE ]
----------------
Could you line up blocks like this so that the values all line up:

```
  - Name: .data
    Type:  SHT_PROGBITS
    Flags:  [ SHF_ALLOC, SHF_WRITE ]
```



================
Comment at: llvm/test/tools/llvm-objdump/ELF/elf-dynamic-symbol.test:18-21
+  - Name: localsym
+    Type: STT_OBJECT
+    Section: .data
+    Binding: STB_LOCAL
----------------
```
  - Name:    localsym
    Type:    STT_OBJECT
    Section: .data
    Binding: STB_LOCAL
```

Same below


================
Comment at: llvm/test/tools/llvm-objdump/ELF/elf-dynamic-symbol.test:51
+
+## Test dumping ELF files with no dynamic symbol.
+# RUN: yaml2obj --docnum=2 %s -o %t2
----------------
I think there are a few different cases here, and I think you need to test them all:

1) No dynamic symbol table at all (i.e. no SHT_DYNSYM section).
2) A truly empty .dynsym section (size == 0)
3) A logically empty .dynsym section (just has the 0-index symbol).

I think this is testing the first one, but I'm not sure.


================
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` -> `DumpDynamic`

I think that's sufficient, and also it's the symbol table, not a single symbol.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75756