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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D75756#1918796 <https://reviews.llvm.org/D75756#1918796>, @Higuoxing wrote:

> - I didn't apply `--strict-whitespace` to `FileCheck`, because there are tabs preventing me doing so (Shall we look into this later?).


Does GNU objdump use hard tabs too? How practical would it be to write hard tabs in the test check? I believe they should be respected with --strict-whitespace (although you'll need to remove the extra space between the CHECKs' ':' and the start of the string to check).



================
Comment at: llvm/test/tools/llvm-objdump/ELF/dynamic-symbol.test:12
+# DYNSYM-NEXT: 0000000000000000 g    Df .data  0000000000000000 filesym
+# DYNSYM-NEXT: 0000000000000000 g    DF .data  0000000000000000 funcsym
+
----------------
You need to add a CHECK-EMPTY or similar the line below to show that there are no more symbols printed.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/dynamic-symbol.test:49
+    Binding: STB_GLOBAL
+
+
----------------
You don't need the double blank line. It's clear enough that the next bit is a new case with a single one.

Same goes below.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/dynamic-symbol.test:56
+
+# NODYNSYM: DYNAMIC SYMBOL TABLE:
+
----------------
You need to add a CHECK-EMPTY or similar the line below to show that there are no symbols printed.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/dynamic-symbol.test:64-67
+Sections:
+  - Name:  .data
+    Type:  SHT_PROGBITS
+    Flags: [ SHF_ALLOC, SHF_WRITE ]
----------------
You don't need the .data section here or in any of the test cases below..


================
Comment at: llvm/test/tools/llvm-objdump/ELF/dynamic-symbol.test:70
+
+## Test dumping ELF files with logically empty .dynsym section. (only have a 0-index NULL symbol)
+# RUN: llvm-objdump --dynamic-syms %t1 | FileCheck %s  --match-full-lines --check-prefix=LOGICAL_EMPTY
----------------
Put the brackets before the full stop. Also "have" -> "has".


================
Comment at: llvm/test/tools/llvm-objdump/ELF/dynamic-symbol.test:74
+
+# LOGICAL_EMPTY: DYNAMIC SYMBOL TABLE:
+
----------------
I'd change the check prefix to "ONLY-NULL"

You need to add a CHECK-EMPTY or similar the line below to show that there are no symbols printed.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/dynamic-symbol.test:88
+
+## Test dumping ELF files with truely empty .dynsym section. (size of .dynsym section is 0)
+# RUN: llvm-objdump --dynamic-syms %t1 | FileCheck %s  --match-full-lines --check-prefix=TRUELY_EMPTY
----------------
Same comment as above re. full stop position. Also truely -> truly.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/dynamic-symbol.test:92
+
+# TRUELY_EMPTY: DYNAMIC SYMBOL TABLE:
+
----------------
TRUELY_EMPTY to simply EMPTY is fine.

You need to add a CHECK-EMPTY or similar the line below to show that there are no symbols printed.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1875
+      reportWarning(
+          "This operation is only currently supported for ELF object files.\n",
+          FileName);
----------------
Still need a test for this. Also, use lower-case first letter for diagnostics, and no trailing full stop. Finally, do you need the '\n'? I'd expect `reportWarning` to do that itself.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1971
       }
-      StringRef SectionName =
-          unwrapOrError(Section->getName(), O->getFileName());
+      StringRef SectionName = unwrapOrError(Section->getName(), FileName);
       outs() << SectionName;
----------------
Unrelated formatting change should not be included.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75756