This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by MaskRay on Jul 24 2023, 5:31 PM.

Details

Summary

Similar to D96617 for llvm-symbolizer.

This patch matches the GNU objdump -d behavior to suppress printing
labels for mapping symbols. Mapping symbol names don't convey much
information.

When --show-all-symbols (not in GNU) is specified, we still print
mapping symbols.

Note: the for (size_t SI = 0, SE = Symbols.size(); SI != SE;) loops
needs to iterate all mapping symbols, even if they are not displayed.
We use the new field IsMappingSymbol to recognize mapping symbols.
This field also enables simplification after D139131.

ELF/ARM/disassemble-all-mapping-symbols.s is enhanced to add .space 2.
If End = std::min(End, Symbols[SI].Addr); is not correctly set, we
would print a .word.

Diff Detail

Event Timeline

MaskRay created this revision.Jul 24 2023, 5:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 5:31 PM
Herald added a subscriber: emaste. · View Herald Transcript
MaskRay requested review of this revision.Jul 24 2023, 5:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 5:31 PM
MaskRay updated this revision to Diff 543851.Jul 25 2023, 12:06 AM
MaskRay edited the summary of this revision. (Show Details)

fix all tests. add mapping symbol

MaskRay updated this revision to Diff 543860.Jul 25 2023, 12:53 AM
MaskRay edited the summary of this revision. (Show Details)

simplify some code from D139131 with the new field MappingSymbol

I'm OK in principle with the chage, particularly as it matches GNU objdump. I sometimes find mapping symbols useful for Assembly language when mixing Arm and Thumb in the same function, that isn't particularly common in practice though, and we can get them back with --show-all-syms.

Some minor suggestions to the code.

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
35

Would IsMappingSymbol be a better name. It would match the form of IsXCOFF and reads a bit better in statements like if (a.IsMappingSymbol) . Not a strong opinion though.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1691

Was this change intentional?

MaskRay updated this revision to Diff 544032.Jul 25 2023, 10:32 AM
MaskRay marked an inline comment as done.

MappingSymbol => IsMappingSymbol
remove a stray blank line

MaskRay marked an inline comment as done.Jul 25 2023, 10:32 AM
MaskRay added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
1691

Not:) Fixed. I have tried several approaches for this patch, and I forgot to drop this change after abandoning my previous approaches.

MaskRay edited the summary of this revision. (Show Details)Jul 25 2023, 10:33 AM
peter.smith accepted this revision.Jul 25 2023, 10:39 AM

Thanks. LGTM from me. Will be worth leaving some time for other reviewers to comment/object.

This revision is now accepted and ready to land.Jul 25 2023, 10:39 AM
jhenderson added inline comments.Jul 26 2023, 2:20 AM
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
48

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)?

llvm/tools/llvm-objdump/llvm-objdump.cpp
1368–1370

I might be missing something obvious, but I don't see how the code below means the mapping symbol is only stored in AllSymbols if --show-all-symbols is specified?

jobnoorman added inline comments.Jul 26 2023, 2:59 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1379

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
  • 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 and llvm-nm. 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.

MaskRay marked 2 inline comments as done.Jul 26 2023, 10:12 AM
MaskRay added inline comments.
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
48

Sorry, I forgot to rename the parameter. Done.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1379

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.

MaskRay updated this revision to Diff 544421.Jul 26 2023, 10:14 AM
MaskRay marked an inline comment as done.

rename a variable
fix a comment

jhenderson accepted this revision.Jul 27 2023, 12:08 AM

LGTM. Perhaps worth a release note, and probably should wait for @jobnoorman to confirm.

jobnoorman accepted this revision.Jul 27 2023, 12:36 AM
jobnoorman added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
1379

Thanks for the explanation @MaskRay! I might consider creating a patch for SF_Mapping in the future to see if it can simplify some things and we can continue the discussion then.

This revision was landed with ongoing or failed builds.Jul 27 2023, 8:51 PM
This revision was automatically updated to reflect the committed changes.