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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In addition to me inline comments, I think one bit of testing you still need is showing whether the demangled or raw name is used for the priority ordering of symbol names.



================
Comment at: llvm/test/tools/llvm-objdump/ELF/data-vs-code-priority.test:1
+## Test that code symbols take priority over data symbols if both are
+## defined at the same address during disassembly.
----------------
This is going to need some kind of `REQUIRES` directive, since it's testing disassembly.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/data-vs-code-priority.test:4
+##
+## Prior to D131589, llvm-objdump would select the alphabetically last
+## symbol at each address. To demonstrate that it's now choosing by
----------------
"Prior to D131589" is going to be pretty meaningless in the future. I'd just get rid of this whole sentence (and delete the "now" from the next sentence), or at least replace this bit with the more generic "Previously".


================
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
----------------
Do you think it would be worth also showing which symbols are printed?


================
Comment at: llvm/test/tools/llvm-objdump/ELF/data-vs-code-priority.test:29-33
+  - Name:  .text
+    Type:  SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x4
+    Content:         010000e3020000e3030000e3040000e3
----------------
I prefer to line up the values in a block so that they all start at the same column, like in the suggestion. It marginally helps with readability, I find.


================
Comment at: llvm/test/tools/llvm-objdump/multiple-symbols.test:1
+# RUN: yaml2obj %s -o %t.o
+# RUN: llvm-objdump --triple armv8a -d %t.o | FileCheck --check-prefix=DEFAULT %s
----------------
I'd move your comment about what the test does to the start of the file here.

Also, this test will need a `REQUIRES` directive too, as it won't work if the build doesn't have the relevant target configured.


================
Comment at: llvm/test/tools/llvm-objdump/multiple-symbols.test:4
+# RUN: llvm-objdump --triple armv8a --show-all-symbols -d %t.o | FileCheck --check-prefix=ALLSYMS %s
+# RUN: llvm-objdump --triple armv8a --disassemble-symbols=aaaa -d %t.o | FileCheck --check-prefix=AAAA %s
+# RUN: llvm-objdump --triple armv8a --disassemble-symbols=bbbb -d %t.o | FileCheck --check-prefix=BBBB %s
----------------
I'd put blank lines between the closely-related groups here. For example, you could have the first two runs in one group, the next 6 in another and the remainder in a third.

I'd then label each group with a comment explaining what's special about that set of test cases.


================
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,
----------------
Nit: here and elsewhere, I think the canonical spelling is ARM.


================
Comment at: llvm/test/tools/llvm-objdump/multiple-symbols.test:29
+## alphabetic, so it will be the alphabetically later symbol in each case: of
+## the names aaaa,bbbb for the Arm function it picks bbbb, and of cccc,dddd for
+## the Thumb function it picks dddd.
----------------
I'd suggest this fomatting for the groups, because it looks initially like you've just omitted a space after the comma!


================
Comment at: llvm/test/tools/llvm-objdump/multiple-symbols.test:33
+# DEFAULT-NOT: >:
+# DEFAULT:       00000000 <bbbb>:
+# DEFAULT-NEXT:         0: e0800080      add     r0, r0, r0, lsl #1
----------------
Rather than half a dozen different CHECK patterns, which are mostly duplicates, I'd consider using multiple check prefixes in each test case to enable/disable the relevant parts. For example, you'd have one prefix for each of the symbols, and then another prefix for each of the disassembly blocks. You could use an `--implicit-check-not` to the FileCheck commands to ensure other stuff isn't printed incorrectly, instead of the `-NOT` style too, but up to you.

Rough example:
```
# AMAP: 00000000 <$a.0>:
# AAAA: 00000000 <aaaa>:
# BBBB: 00000000 <bbbb>:
# CODE1: <first functions code>
# CODE1-NEXT: ...
...
# TMAP: ....
```


================
Comment at: llvm/test/tools/llvm-objdump/multiple-symbols.test:101-103
+## Similarly for the other two functions. This time we must check that the
+## aaaa/bbbb block of code was not disassembled _before_ the output we're
+## expecting.
----------------
Bearing in mind that --disassemble-symbol should already have testing elsewhere, what does this second block of code + function symbols give us that the first block alone doesn't?


================
Comment at: llvm/test/tools/llvm-objdump/multiple-symbols.test:136
+
+## And here's the input object file.
+
----------------
Delete this comment - it's obvious that it's the input file by virtue of it being YAML.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1583
 
-      bool CheckARMELFData = hasMappingSymbols(Obj) &&
-                             Symbols[SI].Type != ELF::STT_OBJECT &&
----------------
The change of this logic should correspond to some sort of test case, I think, but I don't think I see anything?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131589