[PATCH] D114225: Add JSONScopedPrinter to llvm-readelf

Tue Dec 7 03:31:50 PST 2021

jhenderson added inline comments.

================
Comment at: llvm/docs/CommandGuide/llvm-readelf.rst:77
+ ``GNU``, and ``JSON``. ``LLVM`` output is an expanded and structured format.
+ ``GNU`` (the default) output mimics the equivalent GNU :program:`readelf` output.
+ ``JSON`` is json formatted output intended for machine consumption.
----------------
This line might need reflowing, to keep to a standard width (80 presumably).

================
Comment at: llvm/docs/CommandGuide/llvm-readelf.rst:78
+ ``GNU`` (the default) output mimics the equivalent GNU :program:`readelf` output.
+ ``JSON`` is json formatted output intended for machine consumption.

----------------
JSON is an acronym, so should always be all-caps.

================
Comment at: llvm/docs/CommandGuide/llvm-readelf.rst:133
+
+ When used with :option:`--elf-output-style`, will pretty print JSON output.
+
----------------
I'd suggest ", ... JSON output will be formatted in a more readable format." instead of the "will ..." part of the sentence.

================
Comment at: llvm/docs/CommandGuide/llvm-readobj.rst:189
+ structured format. ``GNU`` output mimics the equivalent GNU :program:`readelf`
+ output. ``JSON`` is json formatted output intended for machine consumption.

----------------

================
Comment at: llvm/docs/CommandGuide/llvm-readobj.rst:213
+
+ When used with :option:`--elf-output-style` set to ``JSON``, it will pretty print JSON output.
+
----------------
Use the same description as the other doc.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/file-summary-json.test:4
+## Test outputting file summary for a single file.
+# RUN: yaml2obj %s -o %t.single
+# RUN: llvm-readobj --elf-output-style=JSON --pretty-print %t.single | \
----------------
I'm being dumb: you only need one yaml2obj invocation for all test inputs. Delete the others, and rename the file `%t` or `%t.o` or similar.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/file-summary-json.test:6-7
+# RUN: llvm-readobj --elf-output-style=JSON --pretty-print %t.single | \
+# RUN: FileCheck %s -DFILE="%t.single" --check-prefix=SINGLE \
+# RUN: --match-full-lines --strict-whitespace --implicit-check-not={{.}}
+
----------------
Add some indentation to make it clear these are continuation lines. Same elsewhere.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/file-summary-json.test:56
+# RUN: yaml2obj %s -o %t.single
+# RUN: llvm-ar rc %t.archive-single %t.single
+# RUN: llvm-readobj --elf-output-style=JSON --pretty-print %t.archive-single | \
----------------
`llvm-ar r` doesn't delete the existing archive or its contents, which can lead to unexpected results, if the test were aborted/updated in the future. Delete the archive file before recreating it here. Same below.

================
Comment at: llvm/test/tools/llvm-readobj/ELF/pretty-print.test:6
+# RUN: llvm-readobj --elf-output-style=JSON %t.pretty-off | \
+# RUN: FileCheck %s --DFILE="%t.pretty-off" -check-prefix=NO-PRETTY \
+# RUN: --strict-whitespace --implicit-check-not={{.}}
----------------
`-D` is usually a single-dash option; `--check-prefix` usually has two dashes.

================
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:
> > 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.
> Sorry, don't mean to drag this on but I wanted to ensure we're on the same page.
> 
> `opts::Output` by default is initialized to `LLVM` so if `--elf-output-style` is not specified, then `opts::Output` will be `LLVM`. So I believe that even beforehand non-ELF objects would still have file summary information. Looking at a readobj tests with COFF [[ https://github.com/llvm/llvm-project/blob/main/lld/test/COFF/hello32.test#L8 | here ]] it looks like it's printing file summary info despite not being an ELF object.
Default and `--elf-output-style=LLVM` behaviours are unchanged. I'm not talking about those. I'm talking about the following command-line:
```
$ llvm-readobj coff.obj --elf-output-style=GNU
```
Before this had no file summary. Now it does. I think it is okay if it does, but I think this should be called out in the commit description.

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

https://reviews.llvm.org/D114225