[PATCH] D102603: [llvm-objdump] Print the DEBUG type under `--section-headers`.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/PowerPC/section-headers-debug.test:1
+## Check the `--section-headers` option can print the DEBUG type.
+# RUN: yaml2obj %s -o %t
----------------
Rather than having a test dedicated to a specific type field, it would make more sense to have a test that tests all type fields, probably as part of a generic `--section-headers` test. As such, before you add the DEBUG type, I'd recommend writing a new test/expanding an existing test for --section-headers, to test that option thoroughly (a quick skim of test/tools/llvm-objdump suggests there is no such test there, but there is `objdump-sectionheaders.test` in llvm/test/Object, which should probably be moved). This would be a separate patch, prior to this one.

Another point: the behaviour is not specific to PowerPC, and the test doesn't rely on the PowerPC target being available, so move the test into the ELF directory.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/xcoff-section-headers-debug.test:1
+## Check the `--section-headers` option can print the DEBUG type.
+## TODO: Replace the binary input with a YAML input and then use yaml2obj.
----------------
Similar comments as above. Don't have a dedicated test for the DEBUG type. Just expand the existing test.

Also, it would be preferable to use yaml2obj for this testing in the first place, rather than a canned binary, so rebase this patch on top of the related patches and wait for those to land.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1717
+    if (Section.isDebugSection())
+      Type += Type.empty() ? "DEBUG" : "DEBUG";
 
----------------
Higuoxing wrote:
> I think we need some tests to check the output when the section has multiple types, e.g., 'DATA' and 'DEBUG'.
+1 - also I think this code is actually slightly incompatible with GNU output, which uses comma separators. It should be fixed to match.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102603