[PATCH] D109452: using symbol index and qualname for --sym --symbol--description for llvm-objdump for xcoff

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added a comment.

In D109452#2991032 <https://reviews.llvm.org/D109452#2991032>, @jhenderson wrote:

> I'd like other XCOFF people to look at this and decide if the output is a good style for them.

Generally, yes. Having the containing csect information with the section information prevents confusion for developers making used of the GNU `__section__` attribute in C/C++. The ability to differentiate symbols with the same "unqualified" name by means of adding the storage mapping class qualification in the style of the assembly syntax is also very helpful. Lastly, the index information is useful for cross-referencing purposes.



================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test:36
+; SYM-DES-NEXT: 00000000 l       .text (idx: 1) .text[PR]
+; SYM-DES-NEXT: 00000000 l       .text(Csect: (idx: 1) .text[PR])  (idx: 3) ._Z3bari
+; SYM-DES-NEXT: 00000030 l       .text(Csect: (idx: 1) .text[PR])  (idx: 5) ._Z3fooi
----------------
In keeping with "idx" being not capitalized, "csect" should not be either. Note also that the style used for prose text in the IBM documentation has the capitalized form of "csect" as "CSECT".


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2048
     outs() << SectionName;
+    if (O->isXCOFF()) {
+      Optional<SymbolRef> SymRef = getXCOFFSymbolContainingSymbolRef(
----------------
The containing csect presents as an annotation "within" the section field. It may be friendlier if the delimiters of the section field were more explicit:
```
00000000000000c0 l [.text (csect: (idx: 2) .foov[PR])] (idx: 3) .foov
```
and without --symbol-description
```
00000000000000c0 l [.text (csect: .foov)] .foov
```


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2070
 
   if (Common || O->isELF()) {
     uint64_t Val =
----------------
@DiggerLin, at least some XCOFF symbols have size fields. This could be handled in a separate patch if that is your preference (in which case, please add a TODO comment to the code).

@jhenderson, at least for XCOFF, I would prefer to always have the field (even if empty). I am not convinced that having it present only for "common" symbols for non-ELF formats is good.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2112
+  if (O->isXCOFF() && SymbolDescription)
+    SymName = getXCOFFSymbolDescription(createSymbolInfo(O, Symbol), SymName);
+
----------------
jhenderson wrote:
> Seems to me like you're changing the name to add the description. But then the variable is no longer just the name, which could cause issues in the future.
> 
> How about you just print the name before this check, then print the description (inside the if), and finally the new line?
Maybe if the index was not prefixed, that would work: `.foov[PR](idx: 3)`. `--symbol-description` already presents with the current ordering in conjunction with, e.g., `-Dr`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109452