Change ELF tools to allow multiple sections per file.

[pcc]

Summary

This is how multi-partition combined output files are going to look. If we
see multiple sections, the tools will just read the first one.

[grimar]

I think you can add a test using yaml2obj probably. Can you?
Also, does it really pass existent test cases? (I would expect to see some of them testing "Multiple SHT_*" message)

And ideal behavior will be to dump all the sections, right?
Perhaps it worth to report a bug then. It is probably possible to work on that with use of yaml2obj tool
for creating the tests.

[jhenderson]

In D62349#1515398, @grimar wrote:

I think you can add a test using yaml2obj probably. Can you?
Also, does it really pass existent test cases? (I would expect to see some of them testing "Multiple SHT_*" message)

And ideal behavior will be to dump all the sections, right?
Perhaps it worth to report a bug then. It is probably possible to work on that with use of yaml2obj tool
for creating the tests.

You might want to enhance yaml2obj to allow multiple sections with the same name, although I don't think it's strictly needed. On taking the first one, do GNU tools do this? Or do they take the last one/emit an error/print all? I see no reason to emit an error here, so the change in behaviour is good, but if it picks the last, we should probably do the same. As @grimar mentions, if GNU dumps all, we should file bugs.

On the LLVM-specific sections, I think it would make more sense to just do what we do with the normal sections.

llvm/include/llvm/Object/ELFObjectFile.h

948–949 ↗

(On Diff #201078)

What about this case?

llvm/tools/llvm-readobj/ELFDumper.cpp

1429–1430 ↗

(On Diff #201078)

Ditto.

1434–1435 ↗

(On Diff #201078)

Ditto.

[MaskRay]

LGTM. I suppose you'll add the tests in D62350, but please wait for others' opinions.

[MaskRay]

llvm/tools/llvm-readobj/ELFDumper.cpp

1429–1430 ↗

(On Diff #201078)

See D62350. Only dynamic sections are duplicated for partitions.

Static symbol table, etc, are still singleton.

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

1429–1430 ↗

(On Diff #201078)

The principle is more general though. For most of these tools, there's no reason why there can't be multiple sections of these other types.

[pcc]

I think some of the GNU tools dump the first one, while others dump all of them. For example, nm dumps first, readelf dumps all. I agree with @grimar that regardless of what the GNU tools do, I think the end goal should be to dump all of them in all tools somehow. I filed https://bugs.llvm.org/show_bug.cgi?id=42013 to track this behaviour. But to start with, dumping the first one is better than erroring.

I'll see if yaml2obj will let me write tests for this. It looks like the current behaviour is untested, which might mean that things are harder than I'm expecting.

llvm/tools/llvm-readobj/ELFDumper.cpp

1429–1430 ↗

(On Diff #201078)

My initial inclination was to do only what was required for partitions, but after thinking about this for a bit, I think it's fine to do this for all sections. Although it means that we'll be ignoring some sections in some cases, that's simply a limitation of our tools and there's nothing wrong with the files themselves.

[pcc]

Address review comments

[grimar]

llvm/test/Object/multiple-sections.yaml

2 ↗

(On Diff #201372)

I would suggest to use
llvm-readobj -a --elf-cg-profile --addrsig

(llvm-readelf is a GNU style and seems does not implement dumpers for versioning sections and SHT_LLVM_* sections yet.
Also, -a or --headers does not trigger --elf-cg-profile/--addrsig it seems).

And then perhaps I would check that at least something was dumped:

# CHECK:      Version symbols
# CHECK-NEXT:   Section Name: .versym

# CHECK: SHT_GNU_verdef
# CHECK: SHT_GNU_verneed
# CHECK: CGProfile
# CHECK: Addrsig

4 ↗

(On Diff #201372)

Please add a short description of the test in comment.
(i.e. "here we are testing that multiple sections in a object does not trigger error", or something like that)

21 ↗

(On Diff #201372)

You do not need AddressAlign field here and below I think.

78 ↗

(On Diff #201372)

If I correctly understand you need this symbol to trigger creation of the first .symtab.
But does not seem you need .text section, can it probably be local?

Symbols:         
  - Name:            f

llvm/tools/llvm-readobj/ELFDumper.cpp

1410 ↗

(On Diff #201372)

I think the following is more correct:

if (!DynSymRegion.Size) {
  DynSymRegion = createDRIFrom(&Sec);
  // This is only used (if Elf_Shdr present)for naming section in GNU style
  DynSymtabName = unwrapOrError(Obj->getSectionName(&Sec));
  DynamicStringTable = unwrapOrError(Obj->getStringTableForSymtab(Sec));
}

[ruiu]

LGTM

llvm/trunk/include/llvm/Object/ELFObjectFile.h

if (!SectionsOrErr)
     return SectionsOrErr.takeError();
 
   const Elf_Shdr *DotDynSymSec = nullptr;
   const Elf_Shdr *DotSymtabSec = nullptr;
   ArrayRef<Elf_Word> ShndxTable;
   for (const Elf_Shdr &Sec : *SectionsOrErr) {
     switch (Sec.sh_type) {
     case ELF::SHT_DYNSYM: {
-      if (DotDynSymSec)
-        return createError("More than one dynamic symbol table!");
+      if (!DotDynSymSec)
         DotDynSymSec = &Sec;
       break;
     }
     case ELF::SHT_SYMTAB: {
-      if (DotSymtabSec)
-        return createError("More than one static symbol table!");
+      if (!DotSymtabSec)
         DotSymtabSec = &Sec;
       break;
     }
     case ELF::SHT_SYMTAB_SHNDX: {
       auto TableOrErr = EF.getSHNDXTable(Sec);
       if (!TableOrErr)
         return TableOrErr.takeError();
       ShndxTable = *TableOrErr;
       break;

llvm/trunk/test/Object/multiple-sections.yaml

+# RUN: yaml2obj %s -o %t.o
+# RUN: llvm-readobj -a --elf-cg-profile --addrsig %t.o | FileCheck %s
+
+# Test that multiple sections with the same type does not trigger an error.
+
+# CHECK: ElfHeader {
+# CHECK: SHT_GNU_verdef {
+# CHECK: SHT_GNU_verneed {
+# CHECK: CGProfile [
+# CHECK: Addrsig [
+
+--- !ELF
+FileHeader:      
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:        
+  - Name:            .symtab2
+    Type:            SHT_SYMTAB
+    Link:            .strtab
+    Content:         ''
+    EntSize:         24
+  - Name:            .versym
+    Type:            SHT_GNU_versym
+    Entries:         [ ]
+  - Name:            .versym2
+    Type:            SHT_GNU_versym
+    Entries:         [ ]
+  - Name:            .verdef
+    Type:            SHT_GNU_verdef
+    Info:            0x0000000000000000
+    Entries:
+  - Name:            .verdef2
+    Type:            SHT_GNU_verdef
+    Info:            0x0000000000000000
+    Entries:
+  - Name:            .verneed
+    Type:            SHT_GNU_verneed
+    Info:            0x0000000000000000
+    Dependencies:
+  - Name:            .verneed2
+    Type:            SHT_GNU_verneed
+    Info:            0x0000000000000000
+    Dependencies:
+  - Name:            .llvm.call-graph-profile
+    Type:            SHT_LLVM_CALL_GRAPH_PROFILE
+    Content:         ''
+    EntSize:         16
+  - Name:            .llvm.call-graph-profile2
+    Type:            SHT_LLVM_CALL_GRAPH_PROFILE
+    Content:         ''
+    EntSize:         16
+  - Name:            .llvm_addrsig
+    Type:            SHT_LLVM_ADDRSIG
+    Content:         ''
+  - Name:            .llvm_addrsig2
+    Type:            SHT_LLVM_ADDRSIG
+    Content:         ''
+Symbols:         
+  - Name:            f
+...

llvm/trunk/tools/llvm-readobj/ELFDumper.cpp

 ELFDumper<ELFT>::ELFDumper(const object::ELFObjectFile<ELFT> *ObjF,
     ScopedPrinter &Writer)
     : ObjDumper(Writer), ObjF(ObjF) {
   const ELFFile<ELFT> *Obj = ObjF->getELFFile();
 
   for (const Elf_Shdr &Sec : unwrapOrError(Obj->sections())) {
     switch (Sec.sh_type) {
     case ELF::SHT_SYMTAB:
-      if (DotSymtabSec != nullptr)
-        reportError("Multiple SHT_SYMTAB");
+      if (!DotSymtabSec)
         DotSymtabSec = &Sec;
       break;
     case ELF::SHT_DYNSYM:
-      if (DynSymRegion.Size)
-        reportError("Multiple SHT_DYNSYM");
+      if (!DynSymRegion.Size) {
         DynSymRegion = createDRIFrom(&Sec);
-      // This is only used (if Elf_Shdr present)for naming section in GNU style
+        // This is only used (if Elf_Shdr present)for naming section in GNU
+        // style
         DynSymtabName = unwrapOrError(Obj->getSectionName(&Sec));
         DynamicStringTable = unwrapOrError(Obj->getStringTableForSymtab(Sec));
+      }
       break;
     case ELF::SHT_SYMTAB_SHNDX:
       ShndxTable = unwrapOrError(Obj->getSHNDXTable(Sec));
       break;
     case ELF::SHT_GNU_versym:
-      if (SymbolVersionSection != nullptr)
-        reportError("Multiple SHT_GNU_versym");
+      if (!SymbolVersionSection)
         SymbolVersionSection = &Sec;
       break;
     case ELF::SHT_GNU_verdef:
-      if (SymbolVersionDefSection != nullptr)
-        reportError("Multiple SHT_GNU_verdef");
+      if (!SymbolVersionDefSection)
         SymbolVersionDefSection = &Sec;
       break;
     case ELF::SHT_GNU_verneed:
-      if (SymbolVersionNeedSection != nullptr)
-        reportError("Multiple SHT_GNU_verneed");
+      if (!SymbolVersionNeedSection)
         SymbolVersionNeedSection = &Sec;
       break;
     case ELF::SHT_LLVM_CALL_GRAPH_PROFILE:
-      if (DotCGProfileSec != nullptr)
-        reportError("Multiple .llvm.call-graph-profile");
+      if (!DotCGProfileSec)
         DotCGProfileSec = &Sec;
       break;
     case ELF::SHT_LLVM_ADDRSIG:
-      if (DotAddrsigSec != nullptr)
-        reportError("Multiple .llvm_addrsig");
+      if (!DotAddrsigSec)
         DotAddrsigSec = &Sec;
       break;
     }
   }
 
   loadDynamicTable(Obj);
 
   if (opts::Output == opts::GNU)
     ELFDumperStyle.reset(new GNUStyle<ELFT>(Writer, this));