[PATCH] D111658: Add JSON output skeleton to llvm-readelf

Wed Nov 3 01:59:20 PDT 2021

jhenderson added inline comments.

================
Comment at: llvm/docs/CommandGuide/llvm-readelf.rst:74
+ ``GNU`` (the default) output mimics the equivalent GNU :program:`readelf` output.
+ ``JSON`` is json formatted output intended for machine consumption (not all
+ options are currently supported in this mode). 
----------------
I'd be inclined to remove the "not all options are..." bit, as that's already the case for GNU versus LLVM style, and it will avoid the comment rotting as functionality is added.

================
Comment at: llvm/docs/CommandGuide/llvm-readobj.rst:189-190
+ ``GNU`` (the default) output mimics the equivalent GNU :program:`readelf` output.
+ ``JSON`` is json formatted output intended for machine consumption (not all
+ options are currently supported in this mode). 

----------------
Same comments.

================
Comment at: llvm/include/llvm/Support/JSONScopedPrinter.h:24
+        JOS(OS, 2) {}
+  static bool classof(const ScopedPrinterBase *SPB) {
+    return SPB->getKind() == SPK_JSONScopedPrinter;
----------------
Please add a blank line between the constructor and this function.

================
Comment at: llvm/include/llvm/Support/JSONScopedPrinter.h:74
+
+  void printString(StringRef Label, const std::string &Value) override {
+    JOS.attribute(Label, Value);
----------------
I'm not sure you need this particular function. `const std::string &` should implicitly convert to `StringRef`. (I see it was there before, but I'd see whether you can delete this overload entirely).

================
Comment at: llvm/include/llvm/Support/JSONScopedPrinter.h:78
+
+  void printString(StringRef Label, const char *Value) override {
+    JOS.attribute(Label, Value);
----------------
Ditto. `const char *` implicitly converts to `StringRef`.

================
Comment at: llvm/include/llvm/Support/JSONScopedPrinter.h:113
+  }
+
+};
----------------
Nit: delete blank line at end of class.

================
Comment at: llvm/include/llvm/Support/JSONScopedPrinter.h:115
+};
+
+} // namespace llvm
----------------
Nit: delete this blank line too.

================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:82
+  virtual ~ScopedPrinterBase() {}
+  ScopedPrinterKind getKind() const { return Kind; }
+
----------------
Do you need this and the `classof` functions? As far as I can tell, they aren't used anywhere.

================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:99-100
+
+protected:
+  ScopedPrinterBase(ScopedPrinterKind Kind) : Kind(Kind) {}
+};
----------------
Up to you, but I don't think there's a particular need to make this constructor protected, since the class is an abstract class, so can't be directly instantiated.

================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:103
+
+class ScopedPrinter : public ScopedPrinterBase {
+public:
----------------
Slight naming quibble: at the moment, it's not clear why `ScopedPrinter` is not the base class, and why `JSONScopedPrinter` is a sibling class (shares same base class) and not a derived class of `ScopedPrinter`. I'd make `ScopedPrinter` the base class, and call this derived class something else. Perhaps `UnstructuredScopedPrinter` or `PlainScopedPrinter` (other ideas welcome)?

================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:250
+                                            ScopedPrinterBase *WriterBase) {
+  ScopedPrinter *Writer = cast<ScopedPrinter>(WriterBase);
+  return std::make_unique<COFFDumper>(&Obj, *Writer);
----------------
Do you need this case purely because you're avoiding changing `COFFDumper` to own a `ScopedPrinterBase` rather than a `ScopedPrinter`, or does the `COFFDumper` make use of functionality not in the base class? If the former, just change to using the base class all the way. If the latter, that suggests there's something not quite right with our design, as ideally the dumpers shouldn't need to know which variety of dumper they're using - what functionality is in the subclass?

================
Comment at: llvm/tools/llvm-readobj/COFFDumper.cpp:624
     DictScope D(W, "ImageFileHeader");
-    W.printEnum  ("Machine", Obj->getMachine(),
-                    makeArrayRef(ImageFileMachineType));
+    W.printEnum("Machine", Obj->getMachine(),
+                makeArrayRef(ImageFileMachineType));
----------------
It looks to me like you've clang-formatted the whole file. Don't do this as part of this patch - only format the bits of the file you've changed (you may be able to get away with a separate patch to reformat the whole file though).

================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:805
+    JSONScopedPrinter *JSONWriter = cast<JSONScopedPrinter>(WriterBase);
+    return std::make_unique<JSONELFDumper<ELFT>>(Obj, *JSONWriter);
+  }
----------------
In a similar manner to the COFF dumper earlier, it would be ideal if you could avoid the cast, and just pass a ScopedPrinterBase instance into the `ELFDumper` constructors. This would allow you to avoid many of the changes you've made where you pass in a ScopedPrinter now, and decouple the printer style from the actual scoped printer (in some cases, this may be all you need to get some functions to work for JSON style).

================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:7424
+
+// NEW JSON ELF DUMPER
+
----------------
Don't bother with these sorts of comments: they don't add any long-term benefit (and will rot very quickly).

================
Comment at: llvm/tools/llvm-readobj/ObjDumper.cpp:84
     }
-    W.startLine() << format("[%6tx] ", CurrentWord - StrContent);
-    printAsPrintable(W.startLine(), CurrentWord, WordSize);
-    W.startLine() << '\n';
+    if (ScopedPrinter *W = dyn_cast<ScopedPrinter>(WriterBase)) {
+      W->startLine() << format("[%6tx] ", CurrentWord - StrContent);
----------------
Perhaps do this slightly differently, so that we can leave a clear TODO here for JSON output:

```
if (auto *W = dyn_cast<JSONScopedPrinter>(WriterBase))
  continue; // TODO: implement for JSON printer.
ScopedPrinter *W = cast<ScopedPrinter>(WriterBase);
```

Ideally of course, we'd find a way to avoid needing the casting at all. This might be achievable by splitting the iteration away from the printing: keep the iteration in this function, but have a `printStringListEntry` function that is called per iteration, and is a virtual function in the ScopedPrinterBase class, implemented in the subclasses.

================
Comment at: llvm/tools/llvm-readobj/ObjDumper.cpp:136

 void ObjDumper::printSectionsAsString(const object::ObjectFile &Obj,
                                       ArrayRef<std::string> Sections) {
----------------
For this and the similar printSectionsAsHex, I wonder if it would make sense to make this a virtual function that the JSONELFDumper implements differently, again to avoid the casting. Thoughts?

================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:336-346
+  if (opts::Output == opts::JSON) {
+    JSONScopedPrinter *JSONWriter = cast<JSONScopedPrinter>(WriterBase);
+    JSONWriter->objectBegin();
+    JSONWriter->attributeBegin(FileStr);
+    JSONWriter->objectBegin();
+  } else if (opts::Output == opts::LLVM || opts::InputFilenames.size() > 1 ||
+             A) {
----------------
How about the following:

1) Add a `printFileHeader()` function to the `ScopedPrinterBase` interface. For the `JSONScopedPrinter`, it does the objectBegin calls etc. For the `ScopedPrinter`, it does the LLVM/filename count checks (it might even do the later Format/Arch pritning too in this case).
2) Add a `endFile()` function which is a noop for `ScopedPrinter` but for the `JSONScopedPrinter` does the objectEnd calls etc.

This will avoid needing additional checks for `opts::JSON`, and helps hide some of the details a little further - ideally once you've created a `ScopedPrinterBase` instance, you should be able to avoid ever referencing the subclasses.

================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:348
   if (opts::Output == opts::LLVM) {
-    Writer.printString("Format", Obj.getFileFormatName());
-    Writer.printString("Arch", Triple::getArchTypeName(Obj.getArch()));
-    Writer.printString(
+    ScopedPrinter *Writer = cast<ScopedPrinter>(WriterBase);
+    Writer->printString("Format", Obj.getFileFormatName());
----------------
I don't think you need this cast here?

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

-  ScopedPrinter Writer(fouts());
+  std::unique_ptr<ScopedPrinterBase> WriterBase = createWriter();
+
----------------
I think it's probably fine to just leave this called `Writer` - the fact that it's a base class shouldn't be important to the later code.

================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:634
+
+  if (opts::Output == opts::JSON) {
+    JSONScopedPrinter *JSONWriter = cast<JSONScopedPrinter>(WriterBase.get());
----------------
In a similar manner to my earlier comment about files, how about `startOutput` and `endOutput` functions?

================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:640
   for (const std::string &I : opts::InputFilenames)
-    dumpInput(I, Writer);
+    dumpInput(I, WriterBase.get());

----------------
You should be able to avoid some of the changes with pointer versus reference, by doing `*WriterBase.get()`;

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

https://reviews.llvm.org/D111658