[PATCH] D131589: [llvm-objdump] Handle multiple syms at same addr in disassembly.

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham added a comment.

I've left most of your comments un-replied-to so far, because I need to think harder about the choice of symbols to display, as mentioned in one of my inline comments below.



================
Comment at: llvm/test/tools/llvm-objdump/ELF/data-vs-code-priority.test:17-20
+# CHECK: movw r0, #1
+# CHECK: movw r0, #2
+# CHECK: movw r0, #3
+# CHECK: movw r0, #4
----------------
jhenderson wrote:
> Do you think it would be worth also showing which symbols are printed?
Hmmm. Now I look more closely, there's still something a bit odd here.

The symbols that //are// printed are the ones beginning with `B`, in every case. (Exactly as the unmodified llvm-objdump would have done.) The presence of a non-data symbol at each location has caused the section contents to be disassembled as code, but the non-data symbol isn't winning the contest in every case to be the one printed. I wonder if that inconsistency might be confusing? If we think the code symbol is more important from a disassembly perspective, perhaps we should make it the one displayed, as well, for consistency? 

Otherwise you end up printing a data symbol name followed by code, which looks confusing. I feel as if we ought to print a data symbol with data, or a code symbol with code, but not a confusing mixture. I'll have a rethink.


================
Comment at: llvm/test/tools/llvm-objdump/multiple-symbols.test:21
+
+## The test input file contains an Arm and a Thumb function, each with two
+## function-type symbols defined at its entry point. Also, because it's Arm,
----------------
jhenderson wrote:
> Nit: here and elsewhere, I think the canonical spelling is ARM.
I do have to keep remembering that LLVM's canonical spelling isn't the same as Arm's canonical spelling. My fingers have a very strong habit of typing it the way //we// spell it, for obvious reasons!


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1583
 
-      bool CheckARMELFData = hasMappingSymbols(Obj) &&
-                             Symbols[SI].Type != ELF::STT_OBJECT &&
----------------
jhenderson wrote:
> The change of this logic should correspond to some sort of test case, I think, but I don't think I see anything?
It turned out that I had trouble thinking of something that would have //changed// as a result of removing this section!

The intention of the old code here is to avoid checking mapping symbols if we're starting disassembly at an `STT_OBJECT` symbol. But `STT_OBJECT` symbols are handled by the previous if statement by going to `dumpELFData` and then terminating this loop iteration, so it's difficult for one to get as far as here in the first place.

If there is any case that could have got here at all without being eaten by the previous test, it must be a confusing edge case of some kind and I haven't put my finger on it yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131589