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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:88-91
+.. option:: -T, --dynamic-syms
+
+  Display the contents of the dynamic symbol table.
+
----------------
Lexicographically, 'T' is before 't', so this should probably be before the -t option.


================
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
----------------
grimar wrote:
> grimar wrote:
> > 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?
> If we decide to go with this piece for now, please add empty line after "RUN:"
I'd move these into `unimplemented-features.test` probably. I'd also only have one version for each file format (i.e. just --dynamic-syms, not -T as well). I suspect there should be more versions of this though for other formats (I'm thinking XCOFF off the top of my head, but there may be others). As for using pre-compiled binaries, my preference is to not use them if we can avoid it. yaml2obj support exists for COFF and Mach-O at least, so those can be added even if other formats require existing prebuilt binaries.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1874
+    reportWarning(
+        "this operation is only currently supported for ELF object files",
+        FileName);
----------------
This should probably be changed to say "this operation is not currently supported for this file format", removing the "ELF" reference, since the message will need to be updated every time support is added for a new file format.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1885-1887
+void printSymbol(const ObjectFile *O, const SymbolRef &Symbol,
+                 StringRef FileName, StringRef ArchiveName,
+                 StringRef ArchitectureName, bool DumpDynamic) {
----------------
As this function is now unindented, you need to run clang-format on the whole function. Sorry, if I incorrectly commented incorrectly on something being an unrelated formatting before! Phabricator doesn't show the indentation change as an actual change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75756