[PATCH] D96883: Add support for JSON output style to llvm-symbolizer

[George Rimar via Phabricator via llvm-commits]

grimar accepted this revision.
grimar added a comment.
This revision is now accepted and ready to land.

I think I can accept it (with nits fixed), so LGTM. I've added few last minor comments/notes.
(there also seems to be some issues with English grammar in comments, but it is not a place where I can help much).

I'd suggest to hold this a bit for a day or two after you do an update to see if there are other people who might also have
comments/suggestions/objections for this patch.

In D96883#2624732 <https://reviews.llvm.org/D96883#2624732>, @aorlov wrote:

> Note I have reused the existing binary addr.exe for tests. There is the file addr.inp with the valid address for addr.exe. Unfortunately addr.inp also contains some invalid arguments to combine multiple cases in a single test.

Well, `addr.inp` is an input file that contains 3 entries:

  some text
  0x40054d
  some text2

If you didn't want to use all of them, you could either don't use this particular file at all probably, or you could add a new input, e.g. you could add a `addr2.inp` that fits needs of your test(s).
I.e. generally you shouldn't reuse an existent inputs, if you think there should be better/different ones for your test cases.

I am not suggesting doing any changes though, as testing a mix of valid and invalid entries makes sense to me and we do the same sometimes for other tools, like llvm-readelf.
I.e. the current version of tests with new comments looks generally fine to me.



================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:48
+    J.attribute("FileName", Info.FileName);
+  // Print only valid line and column to reduce a noise in the output
+  if (Info.Line)
----------------
Missing a full stop after `output`.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-code.test:29
+
+## Invalid first argument before any valid one, no-inlines
+#      NO-INLINES:{"Error":{"Code":22,"Message":"unable to parse arguments: some text"}}
----------------
MIssing full stop.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-code.test:41
+
+## Invalid first argument before any valid one, inlines
+#      INLINE:{"Error":{"Code":22,"Message":"unable to parse arguments: some text"}}
----------------
Missing full stop.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-frame.test:19
+
+## Resolve out of range address. Expected an empy array.
+# RUN: llvm-symbolizer "FRAME %t.o 1000000" --output-style=JSON \
----------------
`empy` -> `empty`.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-frame.test:41
+##
+## clang++ --target=i386-linux-gnu frame.cpp -g -std=c++11 -S -o frame.s 
+ 
----------------
aorlov wrote:
> grimar wrote:
> > Adding an exact clang version used might be helpfull.
> You can see the exact clang version 13.0.0 at the end of the generated code.
You mean the `info_string0`?

```
.Linfo_string0:
	.asciz	"clang version 13.0.0" # string offset=0
```

It is actually a string that is unimprortant for the test. I.e. it is a piece of input asm that
can be just removed like you did for other unused sections already and nothing should change.

It is a bit strange to force user to read an assembly to find out how it was generated instead
of reading the comment that has an intention to descibe it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883