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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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

> Ping

Sorry, I've been quite busy over the past week, and haven't had time to properly look at this. I've made some basic implementation comments. I don't think the discussion about whether or not this is actually needed has been concluded though. It should be before you invest too much more effort on this. @dblaikie, @MaskRay, thoughts?



================
Comment at: llvm/docs/CommandGuide/llvm-symbolizer.rst:256
 
+  ``JSON`` style provides a machine readable output.
+
----------------



================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:34
 
+Optional<int64_t> fix(const Optional<uint64_t> &V) {
+  if (V.hasValue())
----------------
`fix` and `opt` are not good function names. Fix what? What does "opt" stand for ("optimize", "optional", "opt-out", ...). Please pick more self-explanatory names.


================
Comment at: llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp:40
+
+Optional<llvm::StringRef> opt(const std::string &V, const char *D = "") {
+  if (V != D)
----------------
Why `llvm::StringRef` and not simply `StringRef`? Also the `V` argument should be `StringRef`.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-frame.test:17
+# CHECK-NEXT:  "Size": 1,
+# CHECK-NEXT:  "TagOffset": null
+# CHECK-NEXT:}
----------------
Same as other comments elsewhere - a "null" value doesn't make much sense to me to be printed. I'd omit it entirely. If it can ever be set, you ened specific testing for that too.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json.test:7
+
+#      NO-INLINES:some text
+# NO-INLINES-NEXT:{
----------------
This line isn't valid JSON. I think we should either omit it entirely or encode it in a JSON object somehow, so that we consumers can still read the output.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json.test:14
+# NO-INLINES-NEXT:  "Line": 3,
+# NO-INLINES-NEXT:  "Source": null,
+# NO-INLINES-NEXT:  "StartFileName": "/tmp{{/|\\\\}}x.c",
----------------
In both the tests and examples, "Source" is always "null", which doesn't look right to me? Are you doing something incorrect, or should we just not emit this output.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json.test:72
+# INLINE-A2L-NEXT:      "FileName": "/tmp{{/|\\\\}}x.c",
+# INLINE-A2L-NEXT:      "FunctionName": null,
+# INLINE-A2L-NEXT:      "Line": 3,
----------------
FunctionName is always null here. I think what you actually want to do if -f=0 is to omit the line entirely. The same probably goes for other fields with options that enable/disable them.


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json.test:90
+# INLINE-A2L-NEXT:      "Column": 0,
+# INLINE-A2L-NEXT:      "Discriminator": 0,
+# INLINE-A2L-NEXT:      "FileName": "/tmp{{/|\\\\}}x.c",
----------------
You need testing where the Discriminator is non-zero.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96883