[llvm-readobj] Output valid JSON for GroupSections

[paulkirth]

Summary

The current implementation output the LLVM formatted heading for group
sections, which was not valid JSON. This patch provides two small
customization points that allow the heading to vary between the two
implementations, and another that allows the section members to be
output as valid JSON objects.

[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

• avoid duplication in original patch
• add tests to check the JSON and LLVM differences

[paulkirth]

Rebase to fix patch application

[paulkirth]

Rebase

[paulkirth]

Rebase

[paulkirth]

Rebase.

[paulkirth]

Rebase.

[paulkirth]

Rebase.

[paulkirth]

Rebase.

[jhenderson]

Looks essentially good. Just a few nits and my ongoing comment about testing.

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

414

As in the other cases, I think you're better off moving this test case into the standard test case for printing group sections.

llvm/tools/llvm-readobj/ELFDumper.cpp

719–722

I would consider reordering the order these methods are in. Specifically, getGroupSectionHeaderName is used before the others, so it should be first, and printEmptyGroupMessage is used last, so it should be last in order.

Also applies to the definition order and other places where the function overrides are declared.

747–748

[paulkirth]

Rebase. I had accidentally merged this into an earlier revision in the stack.

[paulkirth]

Reorder methods.

[jhenderson]

LGTM, with nit.

llvm/tools/llvm-readobj/ELFDumper.cpp

7683

Nit: don't have an extra blank line at EOF (the file should end with precisely one \n).

[paulkirth]

Remove newline at EOF.

[paulkirth]

Fix formatting

[paulkirth]

Rebase.

• remove const qualifiers from StringRef

[paulkirth]

Fix Rebase

[paulkirth]

arc diff git merge-base HEAD origin --update D137095

[paulkirth]

Rebase

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

 # JSON-EXPAND-64-NEXT:      Offset: 0x9
 # JSON-EXPAND-64-NEXT:      Type: R_X86_64_32S (11)
 # JSON-EXPAND-64-NEXT:      Symbol: rela_maxpos (9)
 # JSON-EXPAND-64-NEXT:      Addend: 0x7FFFFFFFFFFFFFFF
 # JSON-EXPAND-64-NEXT:    }
 # JSON-EXPAND-64-NEXT:  }
 # JSON-EXPAND-64-NEXT:]
 
-
-
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Sections:
   - Name: .text
Symbols:
   - Name:    rela_minneg
     Section: .text
     Value:   3
   - Name:    rela_maxpos
     Section: .text
     Value:   0xFFFFFFFFFFFFFFFF
 
 
+## Check that group sections are consistent between LLVM and JSON formats
+
+# RUN: yaml2obj %s --docnum=3 -o %t-sg.o
+# RUN: llvm-readobj --elf-section-groups %t-sg.o | FileCheck %s --check-prefix=LLVM-SG
+# RUN: llvm-readobj --elf-section-groups %t-sg.o --elf-output-style=JSON --pretty-print | FileCheck %s --check-prefix=JSON-SG
+
+# LLVM-SG:      Groups {
+# LLVM-SG-NEXT:   Group {
+# LLVM-SG-NEXT:     Name: .group
+# LLVM-SG-NEXT:     Index: 1
+# LLVM-SG-NEXT:     Link: 7
+# LLVM-SG-NEXT:     Info: 1
+# LLVM-SG-NEXT:     Type: COMDAT
+# LLVM-SG-NEXT:     Signature: foo
+# LLVM-SG-NEXT:     Section(s) in group [
+# LLVM-SG-NEXT:       .text.foo
+# LLVM-SG-NEXT:       .rela.text.foo
+# LLVM-SG-NEXT:     ]
+# LLVM-SG-NEXT:   }
+# LLVM-SG-NEXT:   Group {
+# LLVM-SG-NEXT:     Name: .group1
+# LLVM-SG-NEXT:     Index: 2
+# LLVM-SG-NEXT:     Link: 7
+# LLVM-SG-NEXT:     Info: 2
+# LLVM-SG-NEXT:     Type: COMDAT
+# LLVM-SG-NEXT:     Signature: bar
+# LLVM-SG-NEXT:     Section(s) in group [
+# LLVM-SG-NEXT:       .text.bar
+# LLVM-SG-NEXT:       .rela.text.bar
+# LLVM-SG-NEXT:     ]
+# LLVM-SG-NEXT:   }
+
+#      JSON-SG: "Groups": {
+# JSON-SG-NEXT:   "Group": {
+# JSON-SG-NEXT:     "Name": {
+# JSON-SG-NEXT:       "Name": ".group",
+# JSON-SG-NEXT:       "Value": 16
+# JSON-SG-NEXT:     },
+# JSON-SG-NEXT:     "Index": 1,
+# JSON-SG-NEXT:     "Link": 7,
+# JSON-SG-NEXT:     "Info": 1,
+# JSON-SG-NEXT:     "Type": {
+# JSON-SG-NEXT:       "Name": "COMDAT",
+# JSON-SG-NEXT:       "Value": 1
+# JSON-SG-NEXT:     },
+# JSON-SG-NEXT:     "Signature": "foo",
+# JSON-SG-NEXT:     "GroupSections": [
+# JSON-SG-NEXT:       {
+# JSON-SG-NEXT:         "Name": ".text.foo",
+# JSON-SG-NEXT:         "Index": 3
+# JSON-SG-NEXT:       },
+# JSON-SG-NEXT:       {
+# JSON-SG-NEXT:         "Name": ".rela.text.foo",
+# JSON-SG-NEXT:         "Index": 4
+# JSON-SG-NEXT:       }
+# JSON-SG-NEXT:     ]
+# JSON-SG-NEXT:   },
+# JSON-SG-NEXT:   "Group": {
+# JSON-SG-NEXT:     "Name": {
+# JSON-SG-NEXT:       "Name": ".group1",
+# JSON-SG-NEXT:       "Value": 64
+# JSON-SG-NEXT:     },
+# JSON-SG-NEXT:     "Index": 2,
+# JSON-SG-NEXT:     "Link": 7,
+# JSON-SG-NEXT:     "Info": 2,
+# JSON-SG-NEXT:     "Type": {
+# JSON-SG-NEXT:       "Name": "COMDAT",
+# JSON-SG-NEXT:       "Value": 1
+# JSON-SG-NEXT:     },
+# JSON-SG-NEXT:     "Signature": "bar",
+# JSON-SG-NEXT:     "GroupSections": [
+# JSON-SG-NEXT:       {
+# JSON-SG-NEXT:         "Name": ".text.bar",
+# JSON-SG-NEXT:         "Index": 5
+# JSON-SG-NEXT:       },
+# JSON-SG-NEXT:       {
+# JSON-SG-NEXT:         "Name": ".rela.text.bar",
+# JSON-SG-NEXT:         "Index": 6
+# JSON-SG-NEXT:       }
+# JSON-SG-NEXT:     ]
+# JSON-SG-NEXT:   }
+# JSON-SG-NEXT: }
+
+--- !ELF
+FileHeader:
+  Class: ELFCLASS64
+  Data:  ELFDATA2LSB
+  Type:  ET_REL
+Sections:
+  - Name: .group
+    Type: SHT_GROUP
+    Link: [[SYMTAB1=.symtab]]
+    Info: foo
+    Members:
+      - SectionOrType: GRP_COMDAT
+      - SectionOrType: .text.foo
+      - SectionOrType: [[MEMBER1=.rela.text.foo]]
+    ShSize: [[SECSIZE1=<none>]]
+    ShName: [[GROUP1SHNAME=<none>]]
+  - Name: .group1
+    Type: SHT_GROUP
+    Link: [[SYMTAB2=.symtab]]
+    Info: bar
+    Members:
+      - SectionOrType: GRP_COMDAT
+      - SectionOrType: [[MEMBER2=.text.bar]]
+      - SectionOrType: .rela.text.bar
+    ShSize: [[SECSIZE2=<none>]]
+  - Name:  .text.foo
+    Type:  SHT_PROGBITS
+  - Name: .rela.text.foo
+    Type: SHT_RELA
+    Link: .symtab
+    Info: .text.foo
+  - Name:   .text.bar
+    Type:   SHT_PROGBITS
+    ShName: [[TEXTBARSHNAME=<none>]]
+  - Name: .rela.text.bar
+    Type: SHT_RELA
+    Link: .symtab
+    Info: .text.bar
+  - Name: .symtab
+    Type: SHT_SYMTAB
+    Link: [[SYMTABLINK=.strtab]]
+  - Name:    .strtab
+    Type:    SHT_STRTAB
+    Content: [[STRTABCONTENT=<none>]]
+Symbols:
+  - Name:    foo
+    Section: .text.foo
+    StName:  [[SYM1STNAME=<none>]]
+  - Name:    bar
+    Section: .text.bar
+    StName:  [[SYM2STNAME=<none>]]
+
+

llvm/tools/llvm-readobj/ELFDumper.cpp

protected:
   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);
+  virtual void printEmptyGroupMessage() const;
+  virtual void printSectionGroupMembers(const StringRef Name,
+                                        uint64_t Idx) const;
+  virtual std::string getGroupSectionHeaderName() const;
 
   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;
+  void printEmptyGroupMessage() const override;
+
+  void printSectionGroupMembers(const StringRef Name,
+                                uint64_t Idx) const override;
+  std::string getGroupSectionHeaderName() const override;
 
 private:
   std::unique_ptr<DictScope> FileScope;
 };
 
 } // end anonymous namespace
 
 namespace llvm {
template <class ELFT> void LLVMELFDumper<ELFT>::printGroupSections() {
   DenseMap<uint64_t, const GroupSection *> Map = mapSectionsToGroups(V);
   for (const GroupSection &G : V) {
     DictScope D(W, "Group");
     W.printNumber("Name", G.Name, G.ShName);
     W.printNumber("Index", G.Index);
     W.printNumber("Link", G.Link);
     W.printNumber("Info", G.Info);
     W.printHex("Type", getGroupType(G.Type), G.Type);
-    W.startLine() << "Signature: " << G.Signature << "\n";
+    W.printString("Signature", G.Signature);
 
-    ListScope L(W, "Section(s) in group");
+    ListScope L(W, getGroupSectionHeaderName());
     for (const GroupMember &GM : G.Members) {
       const GroupSection *MainGroup = Map[GM.Index];
       if (MainGroup != &G)
         this->reportUniqueWarning(
             "section with index " + Twine(GM.Index) +
             ", included in the group section with index " +
             Twine(MainGroup->Index) +
             ", was also found in the group section with index " +
             Twine(G.Index));
-      W.startLine() << GM.Name << " (" << GM.Index << ")\n";
+      printSectionGroupMembers(GM.Name, GM.Index);
     }
   }
 
   if (V.empty())
-    W.startLine() << "There are no group sections in the file.\n";
+    printEmptyGroupMessage();
+}
+
+template <class ELFT>
+void LLVMELFDumper<ELFT>::printSectionGroupMembers(const StringRef Name,
+                                                   uint64_t Idx) const {
+  W.startLine() << Name << " (" << Idx << ")\n";
+}
+
+template <class ELFT>
+std::string LLVMELFDumper<ELFT>::getGroupSectionHeaderName() const {
+  return "Section(s) in group";
 }
 
 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;
void LLVMELFDumper<ELFT>::printRelocationInfo(const Elf_Shdr &Sec,
                                               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>::printEmptyGroupMessage() const {
+  W.startLine() << "There are no group sections in the file.\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) {

 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);
 }
+
+template <class ELFT> void JSONELFDumper<ELFT>::printEmptyGroupMessage() const {
+  // JSON output does not need to print anything for empty groups
+}
+
+template <class ELFT>
+void JSONELFDumper<ELFT>::printSectionGroupMembers(const StringRef Name,
+                                                   uint64_t Idx) const {
+  DictScope Grp(this->W);
+  this->W.printString("Name", Name);
+  this->W.printNumber("Index", Idx);
+}
+
+template <class ELFT>
+std::string JSONELFDumper<ELFT>::getGroupSectionHeaderName() const {
+  return "GroupSections";
+}