[PATCH] D72973: using symbol index+symbol name + storage mapping class as label for llvm-objdump -D

[Digger via Phabricator via llvm-commits]

DiggerLin marked 8 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test:5
+# RUN: llvm-objdump -D  %p/Inputs/xcoff-section-headers.o | \
+# RUN: FileCheck --check-prefix=CHECK-GNU %s
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Indent continuation lines by a couple of spaces for readability.
> > 
> > ```
> > # RUN: llvm-objdump -D  %p/Inputs/xcoff-section-headers.o | \
> > # RUN:   FileCheck --check-prefix=CHECK-GNU %s
> > ```
> > 
> > Also, I think `CHECK` should be the default without `--symbol-description` and `CHECK-DESC` or something with the option, or better yet, add it to a new test file. Also, why are you checking `--symbol-description` with `--disassemble-all/-D` instead of plain disassembly (i.e. `--disassemble`).
> > 
> > Finally, you probably need a test case + maybe a warning for using `--symbol-description` with something that isn't XCOFF.
> I do not saw different of "-D"  "--disassemble-all", and "--disassemble" in the llvm-objdump --help 
> 
> and 
> the original test case has 
> RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o 
> 
> I just added a new option --symbol-description in the this test case and test the new option.  I think keep the original option of test case llvm-objdump -D is a better choice.
> 
> I am prefer to put the  test  without --symbol-description in current test case.
> we can know what is the different output of llvm-objdump -D with and without  
> --symbol-description  at a glance.
> 
> 
I just read the source code, there are different of  "--disassemble-all", and "--disassemble".
  "--disassemble" only disassembly text section. "--disassemble-all" disassembly all sections

and in the test case , it also disassembly .data section, I think we need to use  "--disassemble-all" or '-D" for .data section.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973