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

[George Rimar via Phabricator via llvm-commits]

grimar accepted this revision.
grimar added a comment.
This revision is now accepted and ready to land.

I have only 2 minor nits and a question to others, otherwise this looks good, thanks!
So I've accepted, but let's wait for one more opinion.



================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-dynamic-symbols.test:86
+  Machine: EM_X86_64
+DynamicSymbols:
+
----------------
`DynamicSymbols: []` is a bit more commonly used in tests for cases when you want to have a `.dynsym` with a zero symbol only.


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-dynamic-symbols.test:107
+    Type:    SHT_DYNSYM
+    Content: ""
+
----------------
`Size: 0` probably better reflects what you want to achieve.


================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-dynamic-symbols.test:118
+# RUN: FileCheck %s --match-full-lines --strict-whitespace
+#      CHECK:DYNAMIC SYMBOL TABLE:
+# CHECK-NEXT:{{.*}}/llvm-objdump: warning:{{.*}}: this operation is only currently supported for ELF object files
----------------
I am not sure about this part.
I am not sure it should be in this test, because it called "elf-*".
And I am concerned that precomiled objects are used a bit, though we could use
a trivial YAMLs instead (anyways the general direction is to remove all committed precompiled objects,
so at least it worth to stop adding new places I think).

At the same time I wonder if we might want to add a test case like `all-targets-unimlemented-features.test` or alike and put all
temporarily checks like this there.

Or perhaps we can just go with this piece for now and remove it in a follow-up (since you do not add new binaries, I think it is OK)

**2All**: What do others think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75756