[PATCH] D72313: [llvm-symbolizer]Fix printing of malformed address values not passed via stdin

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-symbolizer/invalid-input-address.test:3-4
 # simply echoes it as per other malformed input addresses.
-RUN: llvm-symbolizer --obj=addr.exe 0x10000000000000000 | FileCheck %s
+RUN: echo '"some text"' 0x40054d '"some text2"' > %t.rsp
+RUN: echo -e 'some text\n0x40054d\nsome text2\n' > %t.inp
 
----------------
I don't think the "good" address is important here, so you can remove it. Also, the comment is referring to the original RUN line that was there before, so move the echo runs to nearer where they're used.


================
Comment at: llvm/test/tools/llvm-symbolizer/invalid-input-address.test:6
 
-CHECK-NOT: {{.}}
-CHECK:     0x10000000000000000
-CHECK-NOT: {{.}}
+RUN: llvm-symbolizer --obj=addr.exe 0x10000000000000000 | FileCheck -check-prefix=LARGE_ADDR %s
+
----------------
I think `LARGE-ADDR` would be preferable here.


================
Comment at: llvm/test/tools/llvm-symbolizer/invalid-input-address.test:8
+
+RUN: llvm-symbolizer -print-address -obj=%p/Inputs/addr.exe < %t.inp | FileCheck %s
+RUN: llvm-symbolizer -print-address -obj=%p/Inputs/addr.exe "some text" 0x40054d "some text2" | FileCheck %s
----------------
jhenderson wrote:
> print-address isn't important for these checks (no address is applicable for bad inputs).
> 
> Also, prefer '--' for options isntead of '-'.
> 
> Also, I'd recommend naming the CHECK prefixes here something like "LLVM" and llvm-addr2line's to "GNU".
You should add a comment before this line showing what these are intended to test.


================
Comment at: llvm/test/tools/llvm-symbolizer/invalid-input-address.test:8-10
+RUN: llvm-symbolizer -print-address -obj=%p/Inputs/addr.exe < %t.inp | FileCheck %s
+RUN: llvm-symbolizer -print-address -obj=%p/Inputs/addr.exe "some text" 0x40054d "some text2" | FileCheck %s
+RUN: llvm-symbolizer -print-address -obj=%p/Inputs/addr.exe @%t.rsp | FileCheck %s
----------------
print-address isn't important for these checks (no address is applicable for bad inputs).

Also, prefer '--' for options isntead of '-'.

Also, I'd recommend naming the CHECK prefixes here something like "LLVM" and llvm-addr2line's to "GNU".


================
Comment at: llvm/test/tools/llvm-symbolizer/invalid-input-address.test:16-18
+LARGE_ADDR-NOT: {{.}}
+LARGE_ADDR:     0x10000000000000000
+LARGE_ADDR-NOT: {{.}}
----------------
Let's put the checks nearer their corresponding RUN lines, like this:

```
RUN: llvm-symbolizer ... | FileCheck --check-prefix=LARGE-ADDR %s

LARGE-ADDR: ...

RUN: llvm-symbolizer ... | FileCheck --check-prefix=BAD-INPUT %s

BAD-INPUT: ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72313