[PATCH] D114225: Add JSONScopedPrinter to llvm-readelf

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:7342
+  FileScope = std::make_unique<DictScope>(this->W, FileStr);
+  DictScope D(this->W, "FileSummary");
+  this->W.printString("File", FileStr);
----------------
Jaysonyan wrote:
> jhenderson wrote:
> > Isn't this a behaviour change? Is it necesssary? If so, I'd defer it to a separate patch, if at all possible (or put it in a prerequisite patch), so that this patch is a pure NFC if a user doesn't specify `--elf-output-style=json`. I'm also slightly surprised I didn't see any test failures?
> > 
> > I'd suggest in this patch to call out to the base class version to do most of the work, although there's now a slight impedence to doing that in the form of the additional new line printed in one case but not the other, which I'd query.
> > 
> > The base class version was doing the JSON style version before, so I wonder if there's a good reason for this and the base class to diverge?
> Sorry, I'm not sure if I fully understand what constitutes as a behaviour change. This method would only be run when the user has specified `--elf-output-style=JSON` (which is introduced in this patch) so it shouldn't impact any existing behaviour.
> 
> The base class version calls `W.startLine()` which isn't well supported with `JSONScopedPrinter` and my expectation was to override any methods in the base class which use `startLine()`. Beyond that, the `FileScope` is needed in this implementation based on our previous discussion (D114225#inline-1092086) to achieve the desired JSON output discussed in the initial patch (D111658#3072213). If we relied solely on the base class, we wouldn't get each file in their own `{}`.
> 
> In my opinion, it makes sense to not break this change out into its own patch because it relies on the existence of a `JSONELFDumper` which is introduced in this patch and this patch relies on this change so that the output is in a reasonable format (each file has it's own object). 
> Sorry, I'm not sure if I fully understand what constitutes as a behaviour change. This method would only be run when the user has specified --elf-output-style=JSON (which is introduced in this patch) so it shouldn't impact any existing behaviour.

Apologies, I think I was looking at the "changes since previous action" view, so didn't notice that this was in the JSONELFDumper function.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:241
+      error("--elf-output-style value should be either 'LLVM', 'GNU', or "
+            "'JSON' but was '" +
+            OutputStyleChoice + "'");
----------------
Missed that you need a comma here.


================
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);
 
----------------
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.


================
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());
----------------
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`.


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

https://reviews.llvm.org/D114225