[PATCH] D156190: [llvm-objdump] -d: don't display mapping symbols as labels

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.


================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:48
   SymbolInfoTy(uint64_t Addr, StringRef Name, uint8_t Type,
-               bool IsXCOFF = false)
-      : Addr(Addr), Name(Name), Type(Type), IsXCOFF(IsXCOFF), HasType(true) {}
+               bool FormatSpecific = false, bool IsXCOFF = false)
+      : Addr(Addr), Name(Name), Type(Type), IsMappingSymbol(FormatSpecific),
----------------
jhenderson wrote:
> It's not clear to me purely from the name that `FormatSpecific` and `IsMappingSymbol` should be synonymous. Coudl this parameter simply be called `IsMappingSymbol` too (or possibly `IsELFMappingSymbol`)?
Sorry, I forgot to rename the parameter. Done.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1379
+          if (Name.consume_front("$") && Name.size() &&
+              strchr("adtx", Name[0])) {
+            AllMappingSymbols[*SecI].emplace_back(Address - SectionAddr,
----------------
jobnoorman wrote:
> This recognizes every non-`STT_SECTION` symbol that has `SF_FormatSpecific` set with a name that starts with `$[adtx]` as a mapping symbol. I suppose this is correct for the currently supported targets but I see two problems with this:
> - It partially duplicates the logic [here](https://github.com/llvm/llvm-project/blob/efa43d785ee600ef4cc14589e4777264f0613ec9/llvm/include/llvm/Object/ELFObjectFile.h#L742-L763)
> - What if some target has `SF_FormatSpecific` non-mapping, non-`STT_SECTION` symbols with the same naming pattern?
> 
> It seems to me that `SF_FormatSpecific` is too overloaded. Would it make sense to add a new flag, say `SF_Mapping`? This would also benefit a number of tools that now have duplicate the logic to recognize mapping symbols. For example, [llvm-objdump](https://github.com/llvm/llvm-project/blob/efa43d785ee600ef4cc14589e4777264f0613ec9/llvm/tools/llvm-objdump/llvm-objdump.cpp#L500-L522) and [llvm-nm](https://github.com/llvm/llvm-project/blob/744a968f91f7bb92594a422c1b71f03a47c2415d/llvm/tools/llvm-nm/llvm-nm.cpp#L1801-L1808). Note that the detection is always slightly different which may lead to subtle bugs.
> 
> I understand that this is something that should probably not be addresses by this patch but I thought it might be a good place to start the discussion.
Maybe. I think a separate `SF_Mapping` is clearer but likely won't simplify much. The distinction mostly matters for llvm-nm and llvm-objdump and the two tools need special handling anyway.

STT_FILE/STT_SECTION/mapping symbols are hidden in GNU nm by different means.
`nm -a` displays BFD_DEBUGGING symbols, e.g. STT_FILE/STT_SECTION symbols.
`nm --special-syms` is for mapping symbols.
For RISC-V, nm has another behavior https://sourceware.org/bugzilla/show_bug.cgi?id=27584 that we don't completely port. Actually, I don't know if filtering out `.L0` in nm by default is a good behavior. In GNU nm, whether BFD is configured with RISC-V makes the behavior different as well.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156190