[PATCH] D114225: Add JSONScopedPrinter to llvm-readelf

Mon Dec 6 01:03:18 PST 2021

jhenderson added a comment.

Please remember to update llvm/docs/CommandGuide/llvm-readelf.rst and llvm/docs/CommandGuide/llvm-readobj.rst as appropriate for the new options.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/json-file-summary.test:1
+## Test how we output JSON file summaries.
+
----------------
I'd rename this test file-summary-json.test.

I'd also test the entire output, so that you catch the opening and closing braces (if any) as well as any formatting points. To do this, I'd add `--match-full-lines`, `--strict-whitespace` and `--implicit-check-not={{.}}` to the FileCheck executions. These ensure that a) there is no other output on the same line (by default, FileCheck matches substrings); b) there is no additional whitespace on the line (by default, FileCheck ignores leading and trailing whitespace, and compresses all other whitespace into single ' ' characters); c) ensures the regex pattern `.` doesn't match anything that isn't covered by an existing check.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/json-file-summary.test:4
+## Test outputting file summary for a single file.
+# RUN: yaml2obj %s -o %t.trivial.obj.elf-x86-64
+# RUN: llvm-readobj --elf-output-style=JSON --pretty-print %t.trivial.obj.elf-x86-64 | FileCheck %s -DFILE=%t.trivial.obj.elf-x86-64 --check-prefix=SINGLE
----------------
I'd avoid reusing the same output filename, as it makes things a little easier to debug sometimes when there's a test problem. Probably name the files `%t.single`, `%t.multiple` and `%t.archive`.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/json-file-summary.test:5
+# RUN: yaml2obj %s -o %t.trivial.obj.elf-x86-64
+# RUN: llvm-readobj --elf-output-style=JSON --pretty-print %t.trivial.obj.elf-x86-64 | FileCheck %s -DFILE=%t.trivial.obj.elf-x86-64 --check-prefix=SINGLE
+
----------------
These RUN lines are getting quite long. I'd break them up as in the suggested edit. You can use the `\` to continue a RUN statement onto the next line, so this might be useful too to allow the `FileCheck` command to run across multiple lines, when you add the extra options.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/pretty-print.test:3
+
+## Test JSON with pretty-print off
+# RUN: yaml2obj %s -o %t.trivial.obj.elf-x86-64
----------------

================
Comment at: llvm/test/tools/llvm-readobj/ELF/pretty-print.test:4-5
+## Test JSON with pretty-print off
+# RUN: yaml2obj %s -o %t.trivial.obj.elf-x86-64
+# RUN: llvm-readobj --elf-output-style=JSON %t.trivial.obj.elf-x86-64 | FileCheck -DFILE=%t.trivial.obj.elf-x86-64 %s --check-prefix=NO-PRETTY
+
----------------
Same comments as above re. FileCheck additional options, output file name, and breaking up over multiple lines. Also applies below.

================
Comment at: llvm/tools/llvm-readobj/Opts.td:31
 defm hex_dump : Eq<"hex-dump", "Display the specified section(s) as hexadecimal bytes">, MetaVarName<"<name or index>">;
+def pretty_print : FF<"pretty-print", "Pretty print JSON">;
 def relocs : FF<"relocs", "Display the relocation entries in the file">;
----------------

================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:340

-  if (opts::Output == opts::LLVM || opts::InputFilenames.size() > 1 || A) {
-    Writer.startLine() << "\n";
-    Writer.printString("File", FileStr);
-  }
-  if (opts::Output == opts::LLVM) {
-    Writer.printString("Format", Obj.getFileFormatName());
-    Writer.printString("Arch", Triple::getArchTypeName(Obj.getArch()));
-    Writer.printString(
-        "AddressSize",
-        std::string(formatv("{0}bit", 8 * Obj.getBytesInAddress())));
-    Dumper->printLoadName();
-  }
+  Dumper->printFileSummary(FileStr, Obj, opts::InputFilenames, A);

----------------
Jaysonyan wrote:
> jhenderson wrote:
> > I guess there's a subtle behaviour change as a result of this patch, which wouldn't hurt to be called out in the commit description, but I otherwise think is fine: if a user had exactly one non-ELF object/archive (e.g. a COFF object), and they had previously specified `--elf-output-style=GNU` (or run using llvm-readelf, which has that as the default), they would previously have had no file summary, but now they'd get the default summary, provided by the ObjDumper class. Similarly, if they'd had more than one input, or the objects were in an archive, they'd before have had just the `File` part of the summary, but now they have the full file summary.
> I placed the same `opts::InputFilenames.size() > 1 || A` check inside `GNUELFDumper::printFileSummary` method so I think the behaviour is the same.
> 
> Before, if the `opts::Output` is `LLVM` both `if(...)` statements would trigger. Now the `ObjDumper::printFileSummary` performs all the work in both if statements and would be called for any `Dumper` that is not `GNUELFDumper` or `JSONELFDumper` which should be any case where `opts::Output` is `LLVM`.
> 
> And the `GNUELFDumper::printFileSummary` only prints the `File` part and still does the multiple inputs / archive check. 
No, I don't think the behaviour is the same. Non-ELF objects (e.g. COFF/XCOFF etc) won't use the *ELFDumper classes, regardless of the `--elf-output-style` option's value. This is unchanged from before. The difference is that the `opts::Output` value now doesn't have a direct impact on the output (which is good), and only picks the dumper class to use for ELF objects. This means non-ELF objects will always have the default file summary behaviour.

I think this is a good behaviour change (GNU output style should have no effect for non-ELF objects), but it needs highlighting, as said already.

================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:613-615
+  std::unique_ptr<ListScope> D;
+  if (opts::Output == opts::JSON)
+    D = std::make_unique<ListScope>(*Writer.get());
----------------
Jaysonyan wrote:
> jhenderson wrote:
> > To recap: we only need this scope for JSON, so that there is a top-level object, ensuring we always have valid JSON in the output. What's stopping us making the ListScope a member of the `JSONScopedPrinter` class, so that the scoping is added on construction/destruction of the Writer?
> > 
> > Taking it from another point of view, if you had another piece of client code using `JSONScopedPrinter`, would they expect the output to be a valid JSON object, without additional changes, or expect to need to provide the outer object wrapping themselves? If the former, the scoping belongs to the `JSCONScopedPrinter`.
> I think providing an optional `ListScope`/`DictScope` constructor could be a nice quality-of-life improvement, but I think there should still be a way to construct a `JSONScopedPrinter` without `{}`/`[]` already added. Since technically, `"string"` is still valid json so I wouldn't want to remove the option to output json that don't have an outermost `[]`/`{}`.
Yes, I see your point, and agree with you. Are you suggesting deferring to a separate patch, or as part of the original JSONScopedPrinter patch? I suggest the latter, because it will directly improve the code here, in my opinion.

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

https://reviews.llvm.org/D114225