[llvm-readobj] Fix "Section" output when emitting relocations in JSON

[paulkirth]

Summary

Prior to this patch, the JSON output would emit an invalid key from the
shared LLVM implementation. This caused llvm-readobj to output invalid
JSON. This patch introduces a small helper function to print the
relocation information differently between the LLVM and JSON formats.

Before this patch:

"Relocations": [Section (2) .rel.text {

  {
    "Relocation": {
      "Offset": 0,
      "Type": {
        "Value": "R_X86_64_NONE",
        "RawValue": 0
      },
      "Symbol": {
        "Value": "rel_0",
        "RawValue": 1
      }
    }
  },
  ...

After this patch:

"Relocations": [
   {
     "SectionIdx": 2,
     "Relocs": [
       {
         "Relocation": {
           "Offset": 0,
           "Type": {
             "Name": "R_X86_64_NONE",
             "Value": 0
           },
           "Symbol": {
             "Name": "rel_0",
             "Value": 1
           }
         }
       },
       ...

[jhenderson]

Please provide before and after examples, and test cases for the fix. Also, please fix the title to use "llvm-readobj" in the commit message/patch description, not simply "readobj".

[paulkirth]

Rebase

• Update summary
• Add tests
• Simplify implementation to avoid code duplication

[paulkirth]

Rebase to fix patch application

[paulkirth]

Rebase

[paulkirth]

Rebase

[paulkirth]

Rebase.

[aidengrossman]

llvm/tools/llvm-readobj/ELFDumper.cpp

6715

If this section is already getting moved around, it might be worth it to transition to a DictScope here as well (similar to what I did in D143310, but with proper string formatting logic) to leave all the formatting in a centralized place and simplify this function.

[paulkirth]

Rebase.

[paulkirth]

Address comments

• Use scoped dict to simplify implementation and be more consistent

[paulkirth]

Rebase.

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

6712

I wonder if this would be better named printRelocationSectionInfo, since it's not printing any info about the relocations themselves.

7625

Any particular reason we don't just name this key "SectionIndex" or similar?

[paulkirth]

llvm/tools/llvm-readobj/ELFDumper.cpp

6712

That's a good point. I'll update this to reflect.

7625

No, I don't recall any a particular reason. I just went w/ the Idx suffix since it was still clear, and is a pretty common way to shorten or identify index variables. I don't have a preference here so I can change this when I address the other comments.

[paulkirth]

Rebase, address comments, and update tests.

[paulkirth]

Move test code into existing test file

[paulkirth]

Rebase

[paulkirth]

Fix bad rebase between two stacked diffs. I accidentally merged the Group section commit into this one, this commit should restore the correct split between the two.

[paulkirth]

Fix formatting

[jhenderson]

Looks good, with one nit.

llvm/tools/llvm-readobj/ELFDumper.cpp

6717

Nit: blank line between this and the next function.

[paulkirth]

Add missing blank line between functions.

[paulkirth]

Rebase and remove const qualifier from StringRef.

[paulkirth]

Add back funcion removed when rebasing

[paulkirth]

Add missing blank line.

llvm/test/tools/llvm-readobj/ELF/llvm-vs-json-format.test

 # LLVM:      Name: foo
 # LLVM:      Other [ (0x80)
 # LLVM-NEXT:   STO_AARCH64_VARIANT_PCS (0x80)
 # LLVM-NEXT: ]
 # LLVM:      Name: bar
 # LLVM:      Other: 0
 
 # JSON:                "Name": {
-# JSON-NEXT:                "Value": "",
+# JSON-NEXT:                "Name": "",
 # JSON:                "Other": {
-# JSON-NEXT:             "RawFlags": 0,
+# JSON-NEXT:             "Value": 0,
 # JSON-NEXT:             "Flags": []
 # JSON-NEXT:           },
 # JSON:                "Name": {
-# JSON-NEXT:                "Value": "foo",
+# JSON-NEXT:                "Name": "foo",
 # JSON:                "Other": {
-# JSON-NEXT:             "RawFlags": 128,
+# JSON-NEXT:             "Value": 128,
 # JSON-NEXT:             "Flags": [
 # JSON-NEXT:               {
 # JSON-NEXT:                 "Name": "STO_AARCH64_VARIANT_PCS",
 # JSON-NEXT:                 "Value": 128
 # JSON-NEXT:               }
 # JSON-NEXT:             ]
 # JSON-NEXT:           },
 
 # JSON:                "Name": {
-# JSON-NEXT:                "Value": "bar",
+# JSON-NEXT:                "Name": "bar",
 # JSON:                "Other": {
-# JSON-NEXT:             "RawFlags": 0,
+# JSON-NEXT:             "Value": 0,
 # JSON-NEXT:             "Flags": []
 # JSON-NEXT:           },
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL

 # LLVM-64-NEXT:    0x0 R_X86_64_NONE rela_0 0x0
 # LLVM-64-NEXT:    0x1 R_X86_64_PC32 rela_neg 0xFFFFFFFFFFFFFFFF
 # LLVM-64-NEXT:    0x5 R_X86_64_PLT32 rela_pos 0x2
 # LLVM-64-NEXT:    0xFFFFFFFFFFFFFFFF R_X86_64_64 rela_minneg 0x8000000000000000
 # LLVM-64-NEXT:    0x9 R_X86_64_32S rela_maxpos 0x7FFFFFFFFFFFFFFF
 # LLVM-64-NEXT:  }
 # LLVM-64-NEXT:]
 
-#      JSON-64:      "Relocations": [Section (2) .rel.text {
-#      JSON-64:        {
-# JSON-64-NEXT:          "Relocation": {
+#      JSON-64:  "Relocations": [
+# JSON-64-NEXT:    {
+# JSON-64-NEXT:      "SectionIdx": 2,
+# JSON-64-NEXT:        "Relocs": [
+# JSON-64-NEXT:          {
+# JSON-64-NEXT:            "Relocation": {
 # JSON-64-NEXT:            "Offset": 0,
 # JSON-64-NEXT:            "Type": {
-# JSON-64-NEXT:              "Value": "R_X86_64_NONE",
-# JSON-64-NEXT:              "RawValue": 0
+# JSON-64-NEXT:              "Name": "R_X86_64_NONE",
+# JSON-64-NEXT:              "Value": 0
 # JSON-64-NEXT:            },
 # JSON-64-NEXT:            "Symbol": {
-# JSON-64-NEXT:              "Value": "rel_0",
-# JSON-64-NEXT:              "RawValue": 1
+# JSON-64-NEXT:              "Name": "rel_0",
+# JSON-64-NEXT:              "Value": 1
 # JSON-64-NEXT:            }
 # JSON-64-NEXT:          }
 # JSON-64-NEXT:        },
 # JSON-64-NEXT:        {
 # JSON-64-NEXT:          "Relocation": {
 # JSON-64-NEXT:            "Offset": 1,
 # JSON-64-NEXT:            "Type": {
-# JSON-64-NEXT:              "Value": "R_X86_64_PC32",
-# JSON-64-NEXT:              "RawValue": 2
+# JSON-64-NEXT:              "Name": "R_X86_64_PC32",
+# JSON-64-NEXT:              "Value": 2
 # JSON-64-NEXT:            },
 # JSON-64-NEXT:            "Symbol": {
-# JSON-64-NEXT:              "Value": "rel_neg",
-# JSON-64-NEXT:              "RawValue": 2
+# JSON-64-NEXT:              "Name": "rel_neg",
+# JSON-64-NEXT:              "Value": 2
 # JSON-64-NEXT:            }
 # JSON-64-NEXT:          }
 # JSON-64-NEXT:        },
 # JSON-64-NEXT:        {
 # JSON-64-NEXT:          "Relocation": {
 # JSON-64-NEXT:            "Offset": 5,
 # JSON-64-NEXT:            "Type": {
-# JSON-64-NEXT:              "Value": "R_X86_64_PLT32",
-# JSON-64-NEXT:              "RawValue": 4
+# JSON-64-NEXT:              "Name": "R_X86_64_PLT32",
+# JSON-64-NEXT:              "Value": 4
 # JSON-64-NEXT:            },
 # JSON-64-NEXT:            "Symbol": {
-# JSON-64-NEXT:              "Value": "rel_pos",
-# JSON-64-NEXT:              "RawValue": 3
+# JSON-64-NEXT:              "Name": "rel_pos",
+# JSON-64-NEXT:              "Value": 3
 # JSON-64-NEXT:            }
 # JSON-64-NEXT:          }
 # JSON-64-NEXT:        },
 # JSON-64-NEXT:        {
 # JSON-64-NEXT:          "Relocation": {
 # JSON-64-NEXT:            "Offset": 9,
 # JSON-64-NEXT:            "Type": {
-# JSON-64-NEXT:              "Value": "R_X86_64_64",
-# JSON-64-NEXT:              "RawValue": 1
+# JSON-64-NEXT:              "Name": "R_X86_64_64",
+# JSON-64-NEXT:              "Value": 1
 # JSON-64-NEXT:            },
 # JSON-64-NEXT:            "Symbol": {
-# JSON-64-NEXT:              "Value": "rel_64",
-# JSON-64-NEXT:              "RawValue": 4
+# JSON-64-NEXT:              "Name": "rel_64",
+# JSON-64-NEXT:              "Value": 4
 # JSON-64-NEXT:            }
 # JSON-64-NEXT:          }
-# JSON-64-NEXT:        }}
-# JSON-64-NEXT:  Section (3) .rela.text {
-# JSON-64-NEXT:  ,
-# JSON-64-NEXT:        {
+# JSON-64-NEXT:        }
+# JSON-64-NEXT:     ]
+# JSON-64-NEXT:   },
+# JSON-64-NEXT:   {
+# JSON-64-NEXT:     "SectionIdx": 3,
+# JSON-64-NEXT:     "Relocs": [
+# JSON-64-NEXT:       {
 # JSON-64-NEXT:          "Relocation": {
 # JSON-64-NEXT:            "Offset": 0,
 # JSON-64-NEXT:            "Type": {
-# JSON-64-NEXT:              "Value": "R_X86_64_NONE",
-# JSON-64-NEXT:              "RawValue": 0
+# JSON-64-NEXT:              "Name": "R_X86_64_NONE",
+# JSON-64-NEXT:              "Value": 0
 # JSON-64-NEXT:            },
 # JSON-64-NEXT:            "Symbol": {
-# JSON-64-NEXT:              "Value": "rela_0",
-# JSON-64-NEXT:              "RawValue": 5
+# JSON-64-NEXT:              "Name": "rela_0",
+# JSON-64-NEXT:              "Value": 5
 # JSON-64-NEXT:            },
 # JSON-64-NEXT:            "Addend": 0
 # JSON-64-NEXT:          }
 # JSON-64-NEXT:        },
 # JSON-64-NEXT:        {
 # JSON-64-NEXT:          "Relocation": {
 # JSON-64-NEXT:            "Offset": 1,
 # JSON-64-NEXT:            "Type": {
-# JSON-64-NEXT:              "Value": "R_X86_64_PC32",
-# JSON-64-NEXT:              "RawValue": 2
+# JSON-64-NEXT:              "Name": "R_X86_64_PC32",
+# JSON-64-NEXT:              "Value": 2
 # JSON-64-NEXT:            },
 # JSON-64-NEXT:            "Symbol": {
-# JSON-64-NEXT:              "Value": "rela_neg",
-# JSON-64-NEXT:              "RawValue": 6
+# JSON-64-NEXT:              "Name": "rela_neg",
+# JSON-64-NEXT:              "Value": 6
 # JSON-64-NEXT:            },
 # JSON-64-NEXT:            "Addend": 18446744073709551615
 # JSON-64-NEXT:          }
 # JSON-64-NEXT:        },
 # JSON-64-NEXT:        {
 # JSON-64-NEXT:          "Relocation": {
 # JSON-64-NEXT:            "Offset": 5,
 # JSON-64-NEXT:            "Type": {
-# JSON-64-NEXT:              "Value": "R_X86_64_PLT32",
-# JSON-64-NEXT:              "RawValue": 4
+# JSON-64-NEXT:              "Name": "R_X86_64_PLT32",
+# JSON-64-NEXT:              "Value": 4
 # JSON-64-NEXT:            },
 # JSON-64-NEXT:            "Symbol": {
-# JSON-64-NEXT:              "Value": "rela_pos",
-# JSON-64-NEXT:              "RawValue": 7
+# JSON-64-NEXT:              "Name": "rela_pos",
+# JSON-64-NEXT:              "Value": 7
 # JSON-64-NEXT:            },
 # JSON-64-NEXT:            "Addend": 2
 # JSON-64-NEXT:          }
 # JSON-64-NEXT:        },
 # JSON-64-NEXT:        {
 # JSON-64-NEXT:          "Relocation": {
 # JSON-64-NEXT:            "Offset": 18446744073709551615,
 # JSON-64-NEXT:            "Type": {
-# JSON-64-NEXT:              "Value": "R_X86_64_64",
-# JSON-64-NEXT:              "RawValue": 1
+# JSON-64-NEXT:              "Name": "R_X86_64_64",
+# JSON-64-NEXT:              "Value": 1
 # JSON-64-NEXT:            },
 # JSON-64-NEXT:            "Symbol": {
-# JSON-64-NEXT:              "Value": "rela_minneg",
-# JSON-64-NEXT:              "RawValue": 8
+# JSON-64-NEXT:              "Name": "rela_minneg",
+# JSON-64-NEXT:              "Value": 8
 # JSON-64-NEXT:            },
 # JSON-64-NEXT:            "Addend": 9223372036854775808
 # JSON-64-NEXT:          }
 # JSON-64-NEXT:        },
 # JSON-64-NEXT:        {
 # JSON-64-NEXT:          "Relocation": {
 # JSON-64-NEXT:            "Offset": 9,
 # JSON-64-NEXT:            "Type": {
-# JSON-64-NEXT:              "Value": "R_X86_64_32S",
-# JSON-64-NEXT:              "RawValue": 11
+# JSON-64-NEXT:              "Name": "R_X86_64_32S",
+# JSON-64-NEXT:              "Value": 11
 # JSON-64-NEXT:            },
 # JSON-64-NEXT:            "Symbol": {
-# JSON-64-NEXT:              "Value": "rela_maxpos",
-# JSON-64-NEXT:              "RawValue": 9
+# JSON-64-NEXT:              "Name": "rela_maxpos",
+# JSON-64-NEXT:              "Value": 9
 # JSON-64-NEXT:            },
 # JSON-64-NEXT:            "Addend": 9223372036854775807
 # JSON-64-NEXT:          }
-# JSON-64-NEXT:        }}
-#      JSON-64:      ]
+# JSON-64-NEXT:        }
+# JSON-64-NEXT:      ]
+# JSON-64-NEXT:    }
+# JSON-64-NEXT:  ]
 
 ## Show that --expand-relocs expands the relocation dump for LLVM style but
 ## the JSON output is always expanded
 
 # RUN: llvm-readobj -r --expand-relocs %t64 \
 # RUN:   | FileCheck %s --check-prefix=LLVM-EXPAND-64 --match-full-lines --strict-whitespace
 # RUN: llvm-readobj -r --expand-relocs %t64 --elf-output-style=JSON --pretty-print \
 # RUN:   | FileCheck %s --check-prefix=JSON-64

llvm/tools/llvm-readobj/ELFDumper.cpp

 protected:
   void printSymbolOtherField(const Elf_Sym &Symbol) const;
   virtual void printExpandedRelRelaReloc(const Relocation<ELFT> &R,
                                          const StringRef SymbolName,
                                          const StringRef RelocName);
   virtual void printDefaultRelRelaReloc(const Relocation<ELFT> &R,
                                         const StringRef SymbolName,
                                         const StringRef RelocName);
+  virtual void printRelocationInfo(const Elf_Shdr &Sec, const StringRef Name,
+                                   const unsigned SecNdx);
 
   ScopedPrinter &W;
 };
 
 // JSONELFDumper shares most of the same implementation as LLVMELFDumper except
 // it uses a JSONScopedPrinter.
 template <typename ELFT> class JSONELFDumper : public LLVMELFDumper<ELFT> {
 public:
   LLVM_ELF_IMPORT_TYPES_ELFT(ELFT)
 
   JSONELFDumper(const object::ELFObjectFile<ELFT> &ObjF, ScopedPrinter &Writer)
       : LLVMELFDumper<ELFT>(ObjF, Writer) {}
 
   void printFileSummary(StringRef FileStr, ObjectFile &Obj,
                         ArrayRef<std::string> InputFilenames,
                         const Archive *A) override;
   virtual void printZeroSymbolOtherField(const Elf_Sym &Symbol) const override;
 
   void printDefaultRelRelaReloc(const Relocation<ELFT> &R,
                                 const StringRef SymbolName,
                                 const StringRef RelocName) override;
 
+  void printRelocationInfo(const Elf_Shdr &Sec, const StringRef Name,
+                           const unsigned SecNdx) override;
+
 private:
   std::unique_ptr<DictScope> FileScope;
 };
 
 } // end anonymous namespace
 
 namespace llvm {
 
template <class ELFT> void LLVMELFDumper<ELFT>::printRelocations() {
   ListScope D(W, "Relocations");
 
   for (const Elf_Shdr &Sec : cantFail(this->Obj.sections())) {
     if (!isRelocationSec<ELFT>(Sec))
       continue;
 
     StringRef Name = this->getPrintableSectionName(Sec);
     unsigned SecNdx = &Sec - &cantFail(this->Obj.sections()).front();
-    W.startLine() << "Section (" << SecNdx << ") " << Name << " {\n";
-    W.indent();
-    this->printRelocationsHelper(Sec);
-    W.unindent();
-    W.startLine() << "}\n";
+    printRelocationInfo(Sec, Name, SecNdx);
   }
 }
 
 template <class ELFT>
 void LLVMELFDumper<ELFT>::printRelrReloc(const Elf_Relr &R) {
   W.startLine() << W.hex(R) << "\n";
 }
 
void LLVMELFDumper<ELFT>::printDefaultRelRelaReloc(const Relocation<ELFT> &R,
   OS << W.hex(R.Offset) << " " << RelocName << " "
      << (!SymbolName.empty() ? SymbolName : "-");
   if (R.Addend)
     OS << " " << W.hex((uintX_t)*R.Addend);
   OS << "\n";
 }
 
 template <class ELFT>
+void LLVMELFDumper<ELFT>::printRelocationInfo(const Elf_Shdr &Sec,
+                                              const StringRef Name,
+                                              const unsigned SecNdx) {
+  W.startLine() << "Section (" << SecNdx << ") " << Name << " {\n";
+  W.indent();
+  this->printRelocationsHelper(Sec);
+  W.unindent();
+  W.startLine() << "}\n";
+}
+
+template <class ELFT>
 void LLVMELFDumper<ELFT>::printRelRelaReloc(const Relocation<ELFT> &R,
                                             const RelSymbol<ELFT> &RelSym) {
   StringRef SymbolName = RelSym.Name;
   SmallString<32> RelocName;
   this->Obj.getRelocationTypeName(R.Type, RelocName);
 
   if (opts::ExpandRelocs) {
     printExpandedRelRelaReloc(R, SymbolName, RelocName);

 }
 
 template <class ELFT>
 void JSONELFDumper<ELFT>::printDefaultRelRelaReloc(const Relocation<ELFT> &R,
                                                    const StringRef SymbolName,
                                                    const StringRef RelocName) {
   this->printExpandedRelRelaReloc(R, SymbolName, RelocName);
 }
+
+template <class ELFT>
+void JSONELFDumper<ELFT>::printRelocationInfo(const Elf_Shdr &Sec,
+                                              const StringRef Name,
+                                              const unsigned SecNdx) {
+  DictScope Group(this->W);
+  this->W.printNumber("SectionIdx", SecNdx);
+  ListScope D(this->W, "Relocs");
+  this->printRelocationsHelper(Sec);
+}