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

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham marked 10 inline comments as done.
simon_tatham added inline comments.


================
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
----------------
jhenderson wrote:
> simon_tatham wrote:
> > jhenderson wrote:
> > > 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: ....
> > > ```
> > I had a try at this, but I'm afraid I couldn't see how to make it test the things I want tested.
> > 
> > The problem is that the `-NEXT` suffix doesn't apply between different FileCheck prefixes. If I write, for example,
> > ```
> > COMMON: some header
> > FOO-NEXT: line involving foo
> > BAR-NEXT: line involving bar
> > ```
> > then I'd like `--check-prefixes=COMMON,BAR` to enforce that the bar line shows up immediately after the header line, and there isn't an intervening line of any kind. But in fact the `BAR-NEXT` check provokes an error message from FileCheck that there should have been a previous `BAR` check for it to be next to. It apparently means "must be on the next line from the previous check with the same prefix", not "... with any currently enabled prefix".
> > 
> > So I think if I converted this test into your suggested style, I'd lose the ability to have `NEXT` checks at all, so I'd have to have a pair of prefixes for each piece of output, denoting "this line is / is not expected to appear in the file" ...
> > ```
> > # AMAP:     00000000 <$a.0>:
> > # AMAP-NOT: 00000000 <$a.0>:
> > # AAAA:     00000000 <aaaa>:
> > # AAAA-NOT: 00000000 <aaaa>:
> > ```
> > and then each RUN line would have to have an absolutely enormous collection of check-prefixes specifying every single line it both did //and didn't// want.
> > 
> > I can give that a try if you really want me to, but are you //sure// it's clearer? The effect from my point of view is that all the details of what makes one test run different from another are now way off to the right and smushed into a long undistinguished list of keywords, and you have to cross-refer to the checks anyway to make sense of them, instead of laid out in a table of "here is what this test expects to see".
> > The problem is that the -NEXT suffix doesn't apply between different FileCheck prefixes.
> 
> I'm 95% certain that this is incorrect, as I just tested it out locally with the following test passing fine for me:
> ```
> # RUN: echo foo > %t.txt
> # RUN: echo baz >> %t.txt
> # RUN: FileCheck %s --input-file=%t.txt --check-prefixes=FOO,BAZ
> 
> # FOO: foo
> # BAR-NEXT: bar
> # BAZ-NEXT: baz
> ```
> 
> Did you perhaps accidentally omit the `COMMON` from one of your FileCheck prefix sets? The only rule for -NEXT/-EMPTY commands is that there has to be one regular check (across all prefix sets) before the first -NEXT/-EMPTY.
You're right, it does work the way you say. I had indeed missed having an initial regular check, because I started off with a `-NOT` check, which doesn't count. But I was misled by FileCheck's error message: if I adjust your demo so that its first check is `FOO-NOT`, then I see
```z.test:3:3: error: found 'BAZ-NEXT' without previous 'BAZ: line```
which is what led me to think it worked the way I said!


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1510-1511
+        // StringRefs pointing into it.
+        for (const std::string &DemangledName : DemangledSymNamesHere)
+          SymNamesHere.push_back(DemangledName);
+      } else {
----------------
jhenderson wrote:
> I feel like this should be simplfiable to a single line, possibly using something like `SymNamesHere.insert(SymNamesHere.begin(), DemangledSymNamesHere.begin(), DemangledSymNamesHere.end());` although maybe it's unnecessarily complex. (Same goes for the loop immediately below in the `else`).
I'm afraid I don't know enough about that kind of STL iterator idiom to see how you'd do it in the `else` loop, where you not only have to iterate over `SymbolsHere` but also extract the `Name` field of each one. You'd need some kind of lambda, or templated field extraction gadget, or something, surely?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131589