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

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:51
+private:
+  friend bool operator<(const SymbolInfoTy &P1, const SymbolInfoTy &P2) {
+    if (P1.IsXCOFF)
----------------
hubert.reinterpretcast wrote:
> Should we `assert` that `P1.IsXCOFF == P2.IsXCOFF`?
thanks


================
Comment at: llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test:4
+
+# RUN: llvm-objdump -D --symbol-description %p/Inputs/xcoff-section-headers.o | \
+# RUN:   FileCheck --check-prefixes=COMMON,DESC %s
----------------
hubert.reinterpretcast wrote:
> The entire object file has only one label definition and it is currently not tested. The symbol being used for the address associated with the label definition is currently a `C_STAT` symbol. One way of salvaging the test coverage is to implement the sorting change I am requesting.
As we discuss on the scrum meeting, This comment will be addressed in another patch.


================
Comment at: llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test:24
+COMMON: Disassembly of section .text:
+PLAIN:      00000000 .text:
+DESC:       00000000 (idx: 4) .text:
----------------
hubert.reinterpretcast wrote:
> This has helped me understand something about what is happening.
> 
> We should change the sorting so that we prefer symbols with CSECT Auxiliary Entries (i.e., it has a storage mapping class--even if it is a label and we won't print it). We should also prefer labels before csects. In other words, the name should only be considered after the criteria I just listed. The rationale is that the more specific symbol would be selected for the address.
> 
> Yes, it does mean that we should generate the XCOFF form of the symbol info always.
> @jasonliu @daltenty @sfertile, fyi.
> 
> This might be a separate patch; however, note that doing the above as part of this patch happens to fix a test coverage issue.
As we discuss on the scrum meeting, We will implement it  in another patch.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:62
+        SymbolInfo.XCOFFSymInfo.StorageMappingClass.getValue();
+    outs() << "[" << XCOFF::getMappingClassString(Smc).str() << "]";
+  }
----------------
hubert.reinterpretcast wrote:
> Please don't call `.str()` here.
thanks


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1210
     StringRef Name = unwrapOrError(Symbol.getName(), FileName);
     if (Name.empty())
       continue;
----------------
hubert.reinterpretcast wrote:
> With descriptive printing, we can print at least the index for a symbol with an empty name?
I am not sure there is a  symbol with an empty name in symbol table ?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.h:12
 #include "llvm/DebugInfo/DIContext.h"
+#include "llvm/Object/Archive.h"
 #include "llvm/Support/CommandLine.h"
----------------
hubert.reinterpretcast wrote:
> Moving the include directive is the only change in this file from the patch. We don't need to touch this file at all.
when git clang format, it will move the include directive automatically .


================
Comment at: llvm/tools/llvm-objdump/llvm-xcoffdump.h:26
+
+bool isLabel(const object::XCOFFObjectFile *Obj, const object::SymbolRef &Sym);
+
----------------
jhenderson wrote:
> hubert.reinterpretcast wrote:
> > I am having trouble accepting that `llvm-objdump` has any business injecting symbols directly into the `llvm` namespace. I understand that `llvm-objdump.h` does do so. @jhenderson, would you object if we introduced an `llvm::objdump_impl` namespace?
> Seems like a reasonable idea, although I'd probably just call it `llvm::objdump`.
do this in a new patch ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973