[llvm-readobj] Fixes malformed json on JSON printed corefiles

[feg208]

This patch fixes an issue where, when printing corefile notes with llvm-readobj as json, the dumper generated llvm formatted output which isn't valid json. This alters the dumper to, in the NT_FILE, note, dump properly formatted json data.

Prior to this patch the JSON output was formatted like:

               "Mapping": [
                 "Start": 4096,
                 "End": 8192,
                 "Offset": 12288,
                 "Filename": "/path/to/a.out"
               ],

Whereas now it is formatted as:

              "Mappings": [
                {
                  "Start": 4096,
                  "End": 8192,
                  "Offset": 12288,
                  "Filename": "/path/to/a.out"
                },

Which is valid. Additionally the LLVM output has changed to match the structure of the JSON output (i.e. instead of lists of keys it is a list of dictionaries)

[llvmbot]

@llvm/pr-subscribers-llvm-binary-utilities

Author: Fred Grim (feg208)

Changes

This patch fixes an issue where, when printing corefile notes with llvm-readobj as json, the dumper generated llvm formatted output which isn't valid json. This alters the dumper to, in the NT_FILE, note use a JSON particular method and dump properly formatted json data.

---

Full diff: https://github.com/llvm/llvm-project/pull/92835.diff

2 Files Affected:

• (modified) llvm/test/tools/llvm-readobj/ELF/note-core-ntfile.test (+38)
• (modified) llvm/tools/llvm-readobj/ELFDumper.cpp (+27-12)

diff --git a/llvm/test/tools/llvm-readobj/ELF/note-core-ntfile.test b/llvm/test/tools/llvm-readobj/ELF/note-core-ntfile.test
index 752cb723cd221..dab539821f6de 100644
--- a/llvm/test/tools/llvm-readobj/ELF/note-core-ntfile.test
+++ b/llvm/test/tools/llvm-readobj/ELF/note-core-ntfile.test
@@ -3,6 +3,8 @@
 # RUN: yaml2obj %s -o %t.o
 # RUN: llvm-readelf --notes %t.o | FileCheck %s --check-prefix=GNU
 # RUN: llvm-readobj --notes %t.o | FileCheck %s --check-prefix=LLVM
+# RUN: llvm-readelf --elf-output-style=JSON --pretty-print --notes %t.o | FileCheck %s --check-prefix=JSON
+# RUN: llvm-readobj --elf-output-style=JSON --pretty-print --notes %t.o | FileCheck %s --check-prefix=JSON
 
 ## llvm-mc doesn't support generating ET_CORE files; the 'Content' field was
 ## generated with the following steps:
@@ -93,3 +95,39 @@ ProgramHeaders:
 # LLVM-NEXT:     }
 # LLVM-NEXT:   }
 # LLVM-NEXT: ]
+
+# JSON:      "Notes": [
+# JSON-NEXT:  {
+# JSON-NEXT:      "NoteSection": {
+# JSON-NEXT:          "Name": "<?>",
+# JSON-NEXT:          "Offset": 120,
+# JSON-NEXT:          "Size": 148,
+# JSON-NEXT:          "Note": {
+# JSON-NEXT:              "Owner": "CORE",
+# JSON-NEXT:              "Data size": 128,
+# JSON-NEXT:              "Type": "NT_FILE (mapped files)",
+# JSON-NEXT:              "Page Size": 4096,
+# JSON-NEXT:              "Mappings": [
+# JSON-NEXT:                {
+# JSON-NEXT:                  "Start": 4096,
+# JSON-NEXT:                  "End": 8192,
+# JSON-NEXT:                  "Offset": 12288,
+# JSON-NEXT:                  "Filename": "/path/to/a.out"
+# JSON-NEXT:                },
+# JSON-NEXT:                {
+# JSON-NEXT:                  "Start": 16384,
+# JSON-NEXT:                  "End": 20480,
+# JSON-NEXT:                  "Offset": 24576,
+# JSON-NEXT:                  "Filename": "/path/to/libc.so"
+# JSON-NEXT:                },
+# JSON-NEXT:                {
+# JSON-NEXT:                  "Start": 28672,
+# JSON-NEXT:                  "End": 32768,
+# JSON-NEXT:                  "Offset": 36864,
+# JSON-NEXT:                  "Filename": "[stack]"
+# JSON-NEXT:                }
+# JSON-NEXT:            ]
+# JSON-NEXT:          }
+# JSON-NEXT:      }
+# JSON-NEXT: }
+# JSON-NEXT: ]
diff --git a/llvm/tools/llvm-readobj/ELFDumper.cpp b/llvm/tools/llvm-readobj/ELFDumper.cpp
index a752cc4015293..4558dcdf62bfe 100644
--- a/llvm/tools/llvm-readobj/ELFDumper.cpp
+++ b/llvm/tools/llvm-readobj/ELFDumper.cpp
@@ -180,6 +180,16 @@ struct GroupSection {
   std::vector<GroupMember> Members;
 };
 
+struct CoreFileMapping {
+  uint64_t Start, End, Offset;
+  StringRef Filename;
+};
+
+struct CoreNote {
+  uint64_t PageSize;
+  std::vector<CoreFileMapping> Mappings;
+};
+
 namespace {
 
 struct NoteType {
@@ -762,6 +772,7 @@ template <typename ELFT> class LLVMELFDumper : public ELFDumper<ELFT> {
                                           const unsigned SecNdx);
   virtual void printSectionGroupMembers(StringRef Name, uint64_t Idx) const;
   virtual void printEmptyGroupMessage() const;
+  virtual void printCoreNote(const CoreNote &Note, ScopedPrinter &W) const;
 
   ScopedPrinter &W;
 };
@@ -793,6 +804,8 @@ template <typename ELFT> class JSONELFDumper : public LLVMELFDumper<ELFT> {
 
   void printEmptyGroupMessage() const override;
 
+  void printCoreNote(const CoreNote &Note, ScopedPrinter &W) const override;
+
 private:
   std::unique_ptr<DictScope> FileScope;
 };
@@ -5736,16 +5749,6 @@ static AMDGPUNote getAMDGPUNote(uint32_t NoteType, ArrayRef<uint8_t> Desc) {
   }
 }
 
-struct CoreFileMapping {
-  uint64_t Start, End, Offset;
-  StringRef Filename;
-};
-
-struct CoreNote {
-  uint64_t PageSize;
-  std::vector<CoreFileMapping> Mappings;
-};
-
 static Expected<CoreNote> readCoreNote(DataExtractor Desc) {
   // Expected format of the NT_FILE note description:
   // 1. # of file mappings (call it N)
@@ -7838,7 +7841,7 @@ static bool printLLVMOMPOFFLOADNoteLLVMStyle(uint32_t NoteType,
   return true;
 }
 
-static void printCoreNoteLLVMStyle(const CoreNote &Note, ScopedPrinter &W) {
+template <class ELFT> void LLVMELFDumper<ELFT>::printCoreNote(const CoreNote &Note, ScopedPrinter &W) const {
   W.printNumber("Page Size", Note.PageSize);
   for (const CoreFileMapping &Mapping : Note.Mappings) {
     ListScope D(W, "Mapping");
@@ -7849,6 +7852,18 @@ static void printCoreNoteLLVMStyle(const CoreNote &Note, ScopedPrinter &W) {
   }
 }
 
+template <class ELFT> void JSONELFDumper<ELFT>::printCoreNote(const CoreNote &Note, ScopedPrinter &W) const {
+  W.printNumber("Page Size", Note.PageSize);
+  ListScope D(W, "Mappings");
+  for (const CoreFileMapping &Mapping : Note.Mappings) {
+    auto MappingDict = std::make_unique<DictScope>(W);
+    W.printHex("Start", Mapping.Start);
+    W.printHex("End", Mapping.End);
+    W.printHex("Offset", Mapping.Offset);
+    W.printString("Filename", Mapping.Filename);
+  }
+}
+
 template <class ELFT> void LLVMELFDumper<ELFT>::printNotes() {
   ListScope L(W, "Notes");
 
@@ -7916,7 +7931,7 @@ template <class ELFT> void LLVMELFDumper<ELFT>::printNotes() {
             Descriptor, ELFT::Endianness == llvm::endianness::little,
             sizeof(Elf_Addr));
         if (Expected<CoreNote> N = readCoreNote(DescExtractor)) {
-          printCoreNoteLLVMStyle(*N, W);
+          printCoreNote(*N, W);
           return Error::success();
         } else {
           return N.takeError();

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[jh7370]

Rather than adding a near-duplicate method to work around the fact that the existing code doesn't do what we need it to for JSON output, could we instead simply change the existing code to produce output that is valid (i.e. replace the existing function with the new one you've written)? It would mean that the LLVM output would change, but I don't think this is an issue: as far as I'm aware, we don't promise full output compatibility between LLVM releases of the LLVM-style output. We'd want a release note in this case, to advise of both the JSON fix and the change in LLVM style.

[feg208]

It would mean that the LLVM output would change, but I don't think this is an issue: as far as I'm aware, we don't promise full output compatibility between LLVM releases of the LLVM-style output. We'd want a release note in this case, to advise of both the JSON fix and the change in LLVM style.

Sure. As long as that is ok. I didn't really know what the expectation for the llvm format was.

[feg208]

@jh7370 I made the recommended changes and updated the llvm style test

[ilovepi]

This looks good from my perspective. and I agree w/ @jh7370 about the compatibility and approach. Please do wait for his LGTM before landing, though.

Can you also include an example of the bad vs. corrected JSON in the commit message? Something minimal to show what's changed is probably fine.

[feg208]

Can you also include an example of the bad vs. corrected JSON in the commit message? Something minimal to show what's changed is probably fine.

Done!

[ilovepi]

Totally up to you, but I'd just show the relevant bits.

"Mapping": [
     "Start": 4096,
     "End": 8192,
     "Offset": 12288,
     "Filename": "/path/to/a.out"
],

"Mappings": [
  {
     "Start": 4096,
     "End": 8192,
     "Offset": 12288,
     "Filename": "/path/to/a.out"
  },
 ...
]

[jh7370]

Code looks good, barring a doc issue. Please also update the PR description to match the current change, in particular noting that the LLVM output has been structurally changed to make it work for JSON output too.

[jh7370]

Something about this text is causing the pre-merge check for the doc build to fail. I think it's because you've not included a short-hand for the link (e.g. like the #90474 in the example above).

[feg208]

Code looks good, barring a doc issue. Please also update the PR description to match the current change, in particular noting that the LLVM output has been structurally changed to make it work for JSON output too.

@jh7370 I altered the documentation and shortened the commit message along the lines laid out in @ilovepi's comments

[jh7370]

In the PR description:

NT_FILE, note use

I think "note" here should be "now".

LGTM, with that and the other inline comment.