[yaml2obj][obj2yaml] - Improve how we set/dump the sh_entsize field.

[grimar]

Summary

We already set the sh_entsize field in a single place
for all non-implicit sections.

This patch reorders the logic slightly and with it
we finally have the only one place where the sh_entsize is set.

obj2yaml will not dump the EntSize key for SHT_DYNSYM/SHT_SYMTAB sections anymore,
when the value of sh_entsize is equal to sizeof(Elf_Sym)

Note that this also seems revealed an issue in llvm-objcopy:
Previously yaml2obj set the sh_entsize for the .symtab section to 0x18,
now we it sets it for SHT_SYMTAB sections, i.e. by type.
But the llvm-objcopy/ELF/only-keep-debug.test has a .symtab section of type SHT_STRTAB,
and now yaml2obj sets the sh_entsize to 0 for it.
I had to update the corresponding check lines for ES, but the behavior of
llvm-objcopy should be fixed instead I think.
I've added a TODO and a comment.

Depends on D95354

[jhenderson]

Looks reasonable, but perhaps fixing the llvm-objcopy issue first would be best. What do you think?

[grimar]

In D95364#2522324, @jhenderson wrote:

Looks reasonable, but perhaps fixing the llvm-objcopy issue first would be best. What do you think?

I think it is independent and can be fixed in a follow-up probably.

[grimar]

E.g. it seems that the expected behavior of llvm-objcopy would be to drop the sh_link for symbol table in this case. Because it is of SHT_NOBITS type.
Perhaps I'd wait from some input from @MaskRay as he was the author of the test case I think.

llvm/include/llvm/ObjectYAML/ELFYAML.h

 
 template <class ELFT>
 unsigned getDefaultShEntSize(unsigned EMachine, ELF_SHT SecType,
                              StringRef SecName) {
   if (EMachine == ELF::EM_MIPS && SecType == ELF::SHT_MIPS_ABIFLAGS)
     return sizeof(object::Elf_Mips_ABIFlags<ELFT>);
 
   switch (SecType) {
+  case ELF::SHT_SYMTAB:
+  case ELF::SHT_DYNSYM:
+    return sizeof(typename ELFT::Sym);
   case ELF::SHT_GROUP:
     return sizeof(typename ELFT::Word);
   case ELF::SHT_REL:
     return sizeof(typename ELFT::Rel);
   case ELF::SHT_RELA:
     return sizeof(typename ELFT::Rela);
   case ELF::SHT_RELR:
     return sizeof(typename ELFT::Relr);

llvm/lib/ObjectYAML/ELFEmitter.cpp

for (const std::unique_ptr<ELFYAML::Chunk> &D : Doc.Chunks) {
     } else {
       StringRef LinkSec = getDefaultLinkSec(Sec->Type);
       unsigned Link = 0;
       if (!LinkSec.empty() && !ExcludedSectionHeaders.count(LinkSec) &&
           SN2I.lookup(LinkSec, Link))
         SHeader.sh_link = Link;
     }
 
+    if (Sec->EntSize)
+      SHeader.sh_entsize = *Sec->EntSize;
+    else
+      SHeader.sh_entsize = ELFYAML::getDefaultShEntSize<ELFT>(
+          Doc.Header.Machine.getValueOr(ELF::EM_NONE), Sec->Type, Sec->Name);
+
     // We have a few sections like string or symbol tables that are usually
     // added implicitly to the end. However, if they are explicitly specified
     // in the YAML, we need to write them here. This ensures the file offset
     // remains correct.
     if (initImplicitHeader(CBA, SHeader, Sec->Name,
                            Sec->IsImplicit ? nullptr : Sec))
       continue;
 
for (const std::unique_ptr<ELFYAML::Chunk> &D : Doc.Chunks) {
 
     // Set the offset for all sections, except the SHN_UNDEF section with index
     // 0 when not explicitly requested.
     if (!IsFirstUndefSection || Sec->Offset)
       SHeader.sh_offset = alignToOffset(CBA, SHeader.sh_addralign, Sec->Offset);
 
     assignSectionAddress(SHeader, Sec);
 
-    if (Sec->EntSize)
-      SHeader.sh_entsize = *Sec->EntSize;
-    else
-      SHeader.sh_entsize = ELFYAML::getDefaultShEntSize<ELFT>(
-          Doc.Header.Machine.getValueOr(ELF::EM_NONE), Sec->Type, Sec->Name);
-
     if (IsFirstUndefSection) {
       if (auto RawSec = dyn_cast<ELFYAML::RawContentSection>(Sec)) {
         // We do not write any content for special SHN_UNDEF section.
         if (RawSec->Size)
           SHeader.sh_size = *RawSec->Size;
         if (RawSec->Info)
           SHeader.sh_info = *RawSec->Info;
       }
if (YAMLSec && YAMLSec->Flags)
     SHeader.sh_flags = *YAMLSec->Flags;
   else if (!IsStatic)
     SHeader.sh_flags = ELF::SHF_ALLOC;
 
   // If the symbol table section is explicitly described in the YAML
   // then we should set the fields requested.
   SHeader.sh_info = (RawSec && RawSec->Info) ? (unsigned)(*RawSec->Info)
                                              : findFirstNonGlobal(Symbols) + 1;
-  SHeader.sh_entsize = (YAMLSec && YAMLSec->EntSize)
-                           ? (uint64_t)(*YAMLSec->EntSize)
-                           : sizeof(Elf_Sym);
   SHeader.sh_addralign = YAMLSec ? (uint64_t)YAMLSec->AddressAlign : 8;
 
   assignSectionAddress(SHeader, YAMLSec);
 
   SHeader.sh_offset =
       alignToOffset(CBA, SHeader.sh_addralign, RawSec ? RawSec->Offset : None);
 
   if (RawSec && (RawSec->Content || RawSec->Size)) {
void ELFState<ELFT>::initStrtabSectionHeader(Elf_Shdr &SHeader, StringRef Name,
   if (RawSec && (RawSec->Content || RawSec->Size)) {
     SHeader.sh_size = writeContent(CBA, RawSec->Content, RawSec->Size);
   } else {
     if (raw_ostream *OS = CBA.getRawOS(STB.getSize()))
       STB.write(*OS);
     SHeader.sh_size = STB.getSize();
   }
 
-  if (YAMLSec && YAMLSec->EntSize)
-    SHeader.sh_entsize = *YAMLSec->EntSize;
-
   if (RawSec && RawSec->Info)
     SHeader.sh_info = *RawSec->Info;
 
   if (YAMLSec && YAMLSec->Flags)
     SHeader.sh_flags = *YAMLSec->Flags;
   else if (Name == ".dynstr")
     SHeader.sh_flags = ELF::SHF_ALLOC;
 
else {
         reportError(ShSizeOrErr.takeError());
     }
   } else if (RawSec)
     SHeader.sh_size = writeContent(CBA, RawSec->Content, RawSec->Size);
   else
     llvm_unreachable("debug sections can only be initialized via the 'DWARF' "
                      "entry or a RawContentSection");
 
-  if (YAMLSec && YAMLSec->EntSize)
-    SHeader.sh_entsize = *YAMLSec->EntSize;
-  else if (Name == ".debug_str")
-    SHeader.sh_entsize = 1;
-
   if (RawSec && RawSec->Info)
     SHeader.sh_info = *RawSec->Info;
 
   if (YAMLSec && YAMLSec->Flags)
     SHeader.sh_flags = *YAMLSec->Flags;
   else if (Name == ".debug_str")
     SHeader.sh_flags = ELF::SHF_MERGE | ELF::SHF_STRINGS;
 

llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test

 ## If .symtab or .strtab has the SHF_ALLOC flag, it will be changed to SHT_NOBITS.
 # RUN: yaml2obj --docnum=3 %s -o %t3
 # RUN: llvm-objcopy --only-keep-debug %t3 %t3.dbg
 # RUN: llvm-readelf -S -l %t3.dbg | FileCheck --check-prefix=CHECK3 %s
 
 # CHECK3:      [Nr] Name          Type     Address          Off    Size   ES Flg Lk Inf Al
 # CHECK3:      [ 1] .dynsym       NOBITS   0000000000000000 000040 000018 18   A  2   1 1024
 # CHECK3-NEXT: [ 2] .dynstr       NOBITS   0000000000000018 000040 000001 00   A  0   0  0
-# CHECK3-NEXT: [ 3] .symtab       NOBITS   0000000000000019 000040 000018 18   A  4   1  0
+# CHECK3-NEXT: [ 3] .symtab       NOBITS   0000000000000019 000040 000018 00   A  4   1  0
 # CHECK3-NEXT: [ 4] .strtab       NOBITS   0000000000000031 000040 000001 00   A  0   0  0
 # CHECK3-NEXT: [ 5] .shstrtab     STRTAB   0000000000000000 000040 00002b 00      0   0  1
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_DYN
   Machine: EM_X86_64
 Sections:
   - Name:  .dynsym
     Type:  SHT_DYNSYM
     Flags: [ SHF_ALLOC ]
     Link:  .dynstr
     AddressAlign: 0x400
   - Name:  .dynstr
     Type:  SHT_STRTAB
     Flags: [ SHF_ALLOC ]
   - Name:  .symtab
+## TODO: this should be SHT_SYMTAB, but currently llvm-objcopy reports an error:
+##       error: Symbol table has link index of 4 which is not a string table
     Type:  SHT_STRTAB
     Flags: [ SHF_ALLOC ]
     Link:  .strtab
   - Name:  .strtab
     Type:  SHT_STRTAB
     Flags: [ SHF_ALLOC ]
 DynamicSymbols: []
 Symbols: []

llvm/test/tools/obj2yaml/ELF/entsize.yaml

+## Check how obj2yaml dumps the sh_entsize field.
+
+## Check we don't dump the "EntSize" key for SHT_SYMTAB/SHT_DYNSYM sections
+## when the value of sh_entsize is equal to sizeof(ELF_Sym) == 0x18.
+
 # RUN: yaml2obj %s -o %t
 # RUN: obj2yaml %t | FileCheck %s
 
-## Check obj2yaml is able to dump sh_entsize field of a section.
-
-# CHECK:      - Name:            .rodata.cst4
-# CHECK-NEXT:   Type:            SHT_PROGBITS
-# CHECK-NEXT:   Flags:           [ SHF_ALLOC, SHF_MERGE ]
-# CHECK-NEXT:   EntSize:         0x4
+# CHECK:       - Name:    .rodata.cst4
+# CHECK-NEXT:    Type:    SHT_PROGBITS
+# CHECK-NEXT:    EntSize: 0x4
+# CHECK-NEXT:  - Name: .mysymtab
+# CHECK-NEXT:    Type: SHT_SYMTAB
+# CHECK-NEXT:    Link: .strtab
+# CHECK-NEXT:    Size: 0x0
+# CHECK-NEXT:  - Name: .mydynsym
+# CHECK-NEXT:    Type: SHT_DYNSYM
+# CHECK-NEXT:    Size: 0x0
+# CHECK-NEXT: ...
 
 --- !ELF
 FileHeader:
   Class: ELFCLASS64
   Data:  ELFDATA2LSB
   Type:  ET_REL
 Sections:
-  - Name:            .rodata.cst4
-    Type:            SHT_PROGBITS
-    Flags:           [ SHF_ALLOC, SHF_MERGE ]
-    EntSize:         0x0000000000000004
+  - Name:    .rodata.cst4
+    Type:    SHT_PROGBITS
+    EntSize: 0x4
+  - Name:    .mysymtab
+    Type:    SHT_SYMTAB
+    EntSize: [[SYMTABES=0x18]]
+  - Name:    .mydynsym
+    Type:    SHT_DYNSYM
+    EntSize: [[DYNSYMES=0x18]]
+
+## Document that we are unable to dump a SHT_SYMTAB section when its entry size
+## is not equal to sizeof(ELF_Sym).
+
+# RUN: yaml2obj %s -DSYMTABES=0x19 -o %t2
+# RUN: not obj2yaml %t2 2>&1 | FileCheck %s -DFILE=%t2 --check-prefix=ERR1
+
+# ERR1: Error reading file: [[FILE]]: section [index 2] has invalid sh_entsize: expected 24, but got 25
+
+## Document that we are unable to dump a SHT_DYNSYM section when its entry size
+## is not equal to sizeof(ELF_Sym).
+
+# RUN: yaml2obj %s -DDYNSYMES=0x19 -o %t3
+# RUN: not obj2yaml %t3 2>&1 | FileCheck %s -DFILE=%t3 --check-prefix=ERR2
+
+# ERR2: Error reading file: [[FILE]]: section [index 3] has invalid sh_entsize: expected 24, but got 25

llvm/test/tools/obj2yaml/ELF/implicit-sections-order.yaml

 # OUTPUT-NEXT: Sections:
 # OUTPUT-NEXT:   - Name: .foo.1
 # OUTPUT-NEXT:     Type: SHT_PROGBITS
 # OUTPUT-NEXT:   - Name:    .dynsym
 # OUTPUT-NEXT:     Type:    SHT_DYNSYM
 # OUTPUT-NEXT:     Flags:   [ SHF_ALLOC ]
 # OUTPUT-NEXT:     Address: 0x1000
 # OUTPUT-NEXT:     Link:    .dynstr
-# OUTPUT-NEXT:     EntSize: 0x18
 # OUTPUT-NEXT:   - Name: .foo.2
 # OUTPUT-NEXT:     Type: SHT_PROGBITS
 # OUTPUT-NEXT:   - Name:    .dynstr
 # OUTPUT-NEXT:     Type:    SHT_STRTAB
 # OUTPUT-NEXT:     Flags:   [ SHF_ALLOC ]
 # OUTPUT-NEXT:     Address: 0x2000
 # OUTPUT-NEXT:   - Name: .foo.3
 # OUTPUT-NEXT:     Type: SHT_PROGBITS

 # OUTPUT2-NEXT:     Type: SHT_PROGBITS
 # OUTPUT2-NEXT:   - Name: .foo.3
 # OUTPUT2-NEXT:     Type: SHT_PROGBITS
 # OUTPUT2-NEXT:   - Name:    .symtab
 # OUTPUT2-NEXT:     Type:    SHT_SYMTAB
 # OUTPUT2-NEXT:     Flags:   [ SHF_ALLOC ]
 # OUTPUT2-NEXT:     Address: 0x3000
 # OUTPUT2-NEXT:     Link:    .strtab
-# OUTPUT2-NEXT:     EntSize: 0x18
 # OUTPUT2-NEXT:   - Name: .foo.4
 # OUTPUT2-NEXT:     Type: SHT_PROGBITS
 # OUTPUT2-NEXT:   - Name:    .strtab
 # OUTPUT2-NEXT:     Type:    SHT_STRTAB
 # OUTPUT2-NEXT:     Flags:   [ SHF_ALLOC ]
 # OUTPUT2-NEXT:     Address: 0x4000
 # OUTPUT2-NEXT:   - Name: .foo.5
 # OUTPUT2-NEXT:     Type: SHT_PROGBITS

llvm/test/tools/obj2yaml/ELF/no-symtab.yaml

 
 ## A symbol table without the null entry is non-conforming.
 ## Check we don't print "Symbols" and "DynamicSymbols" keys in this case.
 
 # RUN: yaml2obj --docnum=3 %s -o %t3
 # RUN: obj2yaml %t3 | FileCheck %s --check-prefix=EMPTY
 
 # EMPTY:      Sections:
-# EMPTY-NEXT:   - Name:    .symtab
-# EMPTY-NEXT:     Type:    SHT_SYMTAB
-# EMPTY-NEXT:     Link:    .strtab
-## TODO: we shouldn't dump the default "EntSize" value.
-# EMPTY-NEXT:     EntSize: 0x18
-# EMPTY-NEXT:     Size:    0x0
-# EMPTY-NEXT:   - Name:    .dynsym
-# EMPTY-NEXT:     Type:    SHT_DYNSYM
-# EMPTY-NEXT:     Flags:   [ SHF_ALLOC ]
-## TODO: we shouldn't dump the default "EntSize" value.
-# EMPTY-NEXT:     EntSize: 0x18
-# EMPTY-NEXT:     Size:    0x0
+# EMPTY-NEXT:   - Name:  .symtab
+# EMPTY-NEXT:     Type:  SHT_SYMTAB
+# EMPTY-NEXT:     Link:  .strtab
+# EMPTY-NEXT:     Size:  0x0
+# EMPTY-NEXT:   - Name:  .dynsym
+# EMPTY-NEXT:     Type:  SHT_DYNSYM
+# EMPTY-NEXT:     Flags: [ SHF_ALLOC ]
+# EMPTY-NEXT:     Size:  0x0
 # EMPTY-NEXT: ...
 
 --- !ELF
 FileHeader:
   Class: ELFCLASS64
   Data:  ELFDATA2LSB
   Type:  ET_DYN
 Sections:
   - Name: .symtab
     Type: SHT_SYMTAB
     Size: 0
   - Name: .dynsym
     Type: SHT_DYNSYM
     Size: 0