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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast marked an inline comment as done.
hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:769
 
+// TODO: The function need to return an error if there is no Csect auxiliary
+// entry.
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > need -> needs (same below)
> > > 
> > > Is there any reason you can't do this in a separate and prerequisite commit?
> > yes, we can do it separate and prerequisite commit. but We prefer to  have a NFC plan for it after this patch . @hubert.reinterpretcast  
> The change indicated by the TODO might be easier to understand and tune with the context added by the additional uses from this patch. Adding it to this patch would go past the intended scope of this patch to include adjustments needed for some existing calls. I think a follow-on patch makes sense.
> 
> I do understand that there have been multiple things identified as improvements that we want through the review of this patch. In terms of improvements to existing code quality, we are working on D77285, and the subject of the TODO here is the other code quality (as opposed to, e.g., functional behaviour) follow-on.
I expect the code comment to go away shortly, but note that our intended style for "csect" is to use all-lowercase when the word is used in a context where it would not be capitalized in English (and "CSECT" when it would be). "Csect" is only used for camelCasing purposes.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:796-797
 
+// TODO: The function name need to be changed to express the purpose of the
+// function.
 bool XCOFFSymbolRef::hasCsectAuxEnt() const {
----------------
jhenderson wrote:
> Is there any reason you can't do this in a separate and prerequisite commit?
I would like to clarify that I believe this TODO could be done at the same time as change to `getXCOFFCsectAuxEnt32`.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:63
+void llvm::objdump::printXCOFFSymbolDescription(const SymbolInfoTy &SymbolInfo,
+                                                StringRef SymbolName) {
+  // Dummy symbols have no symbol index.
----------------
hubert.reinterpretcast wrote:
> Add a way to assert `SymbolInfo.XCOFFSymInfo.isXCOFF`. Maybe a `getXCOFFSymInfo` function that asserts `isXCOFF`.
Please address: https://reviews.llvm.org/D72973?id=254179#inline-705870.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1210
     StringRef Name = unwrapOrError(Symbol.getName(), FileName);
     if (Name.empty())
       continue;
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > 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 ?
> ```
> extern const char *const str = "Hello, world!";
> ```
> 
> Compiled with:
> ```
> IBM XL C/C++ for AIX, V16.1.0  (5725-C72, 5765-J12)
> Version: 16.01.0000.0004
> ```
> 
> Has this:
> ```
>                         ***Relocation Information***
>              Vaddr      Symndx  Sign  Fixup     Len      Type  Name
> .data:
>         0x00000000  0x0000000e     0      0  0x001f   Pos_Rel  str
>         0x00000004  0x0000000c     0      0  0x001f   Pos_Rel  **No Symbol**
>         0x00000008  0x0000000c     0      0  0x001f   Pos_Rel  **No Symbol**
> ```
> [ ... ]
> ```
> [12]    m   0x0000000c     .data     1  unamex                    **No Symbol**
> [13]    a4  0x0000000e       0    0     SD       RO    0    0
> ```
Please address: https://reviews.llvm.org/D72973?id=252846#inline-705899.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1168
+    Optional<XCOFF::StorageMappingClass> Smc = getXCOFFSymbolCsectSMC(
+        static_cast<const XCOFFObjectFile *>(Obj), Symbol);
+    return SymbolInfoTy(Addr, Name, Smc, SymbolIndex,
----------------
hubert.reinterpretcast wrote:
> We can use `XCOFFObj` instead of casting `Obj` again.
Please address: https://reviews.llvm.org/D72973?id=254179#inline-705885.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973