[llvm][yaml2obj] no more implicit ELF .symtab section

[kwk]

Summary

Before yaml2obj <FILE> would always add an implicit .symtab section
even if there were no symbols defined in the YAML <FILE>. Now, only
when there's a Symbols entry, we will generate a .symtab section.

Old minidebuginfo tests that manually removed the .symtab section
have been adjusted because it is no longer needed.

[kwk]

• Silence FileCheck in test

[kwk]

• restore formatting

[labath]

+llvm yaml2obj folks.

Thank you for creating this diff. Please see comments inline.

lldb/test/Shell/ObjectFile/ELF/no-symtab-generated.yaml

1 ↗

(On Diff #224838)

This test (and the next one) should live in the llvm subtree, as they're really testing yaml2obj, not lldb (instead of lldb, you can use something like llvm-objdump/readobj to inspect the generated files).

llvm/include/llvm/ObjectYAML/ELFYAML.h

379–380

This creates an inconsistency between how .symtab and .dynsym sections are handled, which is hard to justify. I actually prefer the Optional<> version, because that enables one to say "the symtab section is there, but it is _empty_", something which is impossible with the way .dynsym emission works (the section gets surpressed if it is empty), but the question is, shouldn't then the .dynsym emission work the same way ? Have you looked at by any chance how hard it would be to change .dynsym to follow the same pattern?

(In any case, I think this is up to yaml2obj maintainers to decide how to handle this. I'll add some to this review.)

[kwk]

Move tests over to llvm/llvm/test/ObjectYAML/ELF/.

[kwk]

llvm/include/llvm/ObjectYAML/ELFYAML.h

379–380

@labath I haven't looked but will investigate how hard it is to declare Optional<std::vector<Symbol>> DynamicSymbols;. That's what I understood at least.

[kwk]

• moved test files over to llvm subtree

[kwk]

Moved yaml2obj tests under llvm subtree.

[MaskRay]

This is not correct. A lot of yaml2obj tests fail with:

yaml2obj: error: unknown section referenced: '.symtab' by YAML section '.rela.text'

The problem is that .symtab can be the sh_link field of relocation sections, .llvm_addrsig, and some sections that may be added in the future.

Making .symtab conditionally define is difficult. I'll take a closer look tomorrow.

[kwk]

In D68943#1708068, @MaskRay wrote:

This is not correct. A lot of yaml2obj tests fail with:

yaml2obj: error: unknown section referenced: '.symtab' by YAML section '.rela.text'

@MaskRay I'm so sorry. You're absolutely right. I was working only in LLDB for the last months and used check-lldb only. That's probably because I missed those errors.

The problem is that .symtab can be the sh_link field of relocation sections, .llvm_addrsig, and some sections that may be added in the future.

Making .symtab conditionally define is difficult. I'll take a closer look tomorrow.

Thank you. Meanwhile I will run more tests to see what exactly fails.

[grimar]

I have not looked what is needed to fix yaml2obj testcases yet, but below there are
few first comments.

Also, 2 LLVM side tests added should live in "llvm/test/tools/yaml2obj" I think (not in llvm/test/ObjectYAML/ELF/),
because this is actually the place where we test "yaml2obj" usually.

llvm/lib/ObjectYAML/ELFEmitter.cpp

513

It doesn't look correct. We should never use dynamic symbols instead of static symbols I believe.

llvm/test/ObjectYAML/ELF/no-symtab-generated.yaml

14 ↗

(On Diff #224852)

You do not need any sections.

18 ↗

(On Diff #224852)

We often use the following order:

1. Run lines
2. Check lines
3. YAML

I.e. the order you have used in symtab-generated.yaml. I'd suggest to be consistent and stick to it.

Also, probably would be better to have a single test case file, i.e. to merge them. You can see other our tests which do that.

llvm/test/ObjectYAML/ELF/symtab-generated.yaml

1 ↗

(On Diff #224852)

We usually use ## for comments in yaml2obj tests.

31 ↗

(On Diff #224852)

You do not need Entry.

33 ↗

(On Diff #224852)

You do not need any sections it seems.

41 ↗

(On Diff #224852)

Having only a "Name" should be enough.

[kwk]

• Remove unnecessary section
• Have consitent ordering of run, check and YAML lines
• use ## for comments in YAML
• Remove not needed Entry and Sections
• Only have Name for a symbol
• Moved test files from llvm/test/ObjectYAML/ELF/ to llvm/test/tools/yaml2obj/
• Fixup for test files

[kwk]

@grimar I've tackled the most obvious of your comments. I will now see what fails in llvm/test/tools/yaml2obj.

[kwk]

I noticed that the order of sections does matter to some tests. Before the .symtab section was always the first one and now it is the last of the implicit sections being created. Some Link: X checks break because X points to a different section. In order to compensate for this I will make .symtab the first of the implicit sections again. In my local testing this already helps a bit. In cases where we refer to a section by number and expect .symtab to be the section being referenced, I have two options: 1st) create a symbol to force .symtab to be implicitly created or 2nd) change the name of the expected section. Just tell me what you want.

[grimar]

In D68943#1709330, @kwk wrote:

I noticed that the order of sections does matter to some tests. Before the .symtab section was always the first one and now it is the last of the implicit sections being created. Some Link: X checks break because X points to a different section. In order to compensate for this I will make .symtab the first of the implicit sections again.

Please upload the version with this change and I'll try to debug to take a look what happens.

[grimar]

Ok, I was able to debug it finally.

I think we should add a .symtab in a few more cases implicitly.
For example, when we have a SHT_RELA/SHT_REL section that has an empty Link,
i.e. it refers to .symtab by default and we should provide it.
There are other cases, like when Link just explicitly refers to .symtab.

Here is a patch based on this one.
It is unpolished, but shows the idea. With it all yaml2obj tests pass.

patch.diff7 KBDownload

[MaskRay]

In D68943#1709998, @grimar wrote:

Ok, I was able to debug it finally.

I think we should add a .symtab in a few more cases implicitly.
For example, when we have a SHT_RELA/SHT_REL section that has an empty Link,
i.e. it refers to .symtab by default and we should provide it.
There are other cases, like when Link just explicitly refers to .symtab.

Here is a patch based on this one.
It is unpolished, but shows the idea. With it all yaml2obj tests pass.

patch.diff7 KBDownload

Nice! Can you post a differential for the yaml2obj change? I believe the differential can focus on lldb and remove yaml2obj changes (kwk will still get credited for the test cleanups).

[grimar]

In D68943#1710525, @MaskRay wrote:

In D68943#1709998, @grimar wrote:

Ok, I was able to debug it finally.

I think we should add a .symtab in a few more cases implicitly.
For example, when we have a SHT_RELA/SHT_REL section that has an empty Link,
i.e. it refers to .symtab by default and we should provide it.
There are other cases, like when Link just explicitly refers to .symtab.

Here is a patch based on this one.
It is unpolished, but shows the idea. With it all yaml2obj tests pass.

patch.diff7 KBDownload

Nice! Can you post a differential for the yaml2obj change? I believe the differential can focus on lldb and remove yaml2obj changes (kwk will still get credited for the test cleanups).

Yes. It needs polishing + fixes for obj2yaml (I do not think it compiled for me at all). Also probably makes sence to add tests for obj2yaml too, I need to take a look what it does
when the object doesn't have symbol table with such change. I'll try to suggest a patch today.

[labath]

Thank you guys for jumping onto this. This will be very useful in lldb.

[grimar]

I've posted a change for yaml2obj/obj2yaml here: D69041.

[kwk]

Abandoning for D69041

lldb/test/Shell/ObjectFile/ELF/minidebuginfo-corrupt-xz.yaml

 # REQUIRES: lzma
 
 # This test checks that an error occurs when a corrupted
 # .gnu_debugdata section is trying to be xz uncompressed.
 
 # RUN: yaml2obj %s > %t.obj
 
-# TODO(kwk): once yaml2obj doesn't auto-generate a .symtab section
-# when there's none in YAML, remove the following line:
-
-# RUN: llvm-objcopy --remove-section=.symtab %t.obj
-
 # RUN: %lldb -b -o 'image dump symtab' %t.obj 2>&1 | FileCheck %s
 
 # CHECK: warning: (x86_64) {{.*}}.obj An error occurred while decompression the section .gnu_debugdata: lzma_stream_buffer_decode()=lzma error: LZMA_DATA_ERROR
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB

lldb/test/Shell/ObjectFile/ELF/minidebuginfo-find-symbols.yaml

 # REQUIRES: lzma
 
 # RUN: yaml2obj %s > %t.obj
 
-# TODO(kwk): once yaml2obj doesn't auto-generate a .symtab section
-# when there's none in YAML, remove the following line:
-
-# RUN: llvm-objcopy --remove-section=.symtab %t.obj
-
 # RUN: %lldb -b -o 'image dump symtab' %t.obj | FileCheck %s
 
 # CHECK: [ 0] 1 X Code 0x00000000004005b0 0x000000000000000f 0x00000012 multiplyByFour
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB

lldb/test/Shell/ObjectFile/ELF/minidebuginfo-no-lzma.yaml

 # REQUIRES: !lzma
 
 # This test checks that a warning is printed when we're trying
 # to decompress a .gnu_debugdata section when no LZMA support was compiled in.
 
 # RUN: yaml2obj %s > %t.obj
 
-# TODO(kwk): once yaml2obj doesn't auto-generate a .symtab section
-# when there's none in YAML, remove the following line:
-
-# RUN: llvm-objcopy --remove-section=.symtab %t.obj
-
 # RUN: %lldb -b -o 'image dump symtab' %t.obj 2>&1 | FileCheck %s
 
 # CHECK: warning: (x86_64) {{.*}}.obj No LZMA support found for reading .gnu_debugdata section
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB

llvm/include/llvm/ObjectYAML/ELFYAML.h

 struct Object {
   FileHeader Header;
   std::vector<ProgramHeader> ProgramHeaders;
   std::vector<std::unique_ptr<Section>> Sections;
   // Although in reality the symbols reside in a section, it is a lot
   // cleaner and nicer if we read them from the YAML as a separate
   // top-level key, which automatically ensures that invariants like there
   // being a single SHT_SYMTAB section are upheld.
-  std::vector<Symbol> Symbols;
+  Optional<std::vector<Symbol>> Symbols;
   std::vector<Symbol> DynamicSymbols;
 };
 
 } // end namespace ELFYAML
 } // end namespace llvm
 
 LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::ELFYAML::AddrsigSymbol)
 LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::ELFYAML::StackSizeEntry)
 LLVM_YAML_IS_SEQUENCE_VECTOR(llvm::ELFYAML::DynamicEntry)

llvm/lib/ObjectYAML/ELFEmitter.cpp

if (!D->Name.empty())
       DocSections.insert(D->Name);
 
   // Insert SHT_NULL section implicitly when it is not defined in YAML.
   if (Doc.Sections.empty() || Doc.Sections.front()->Type != ELF::SHT_NULL)
     Doc.Sections.insert(
         Doc.Sections.begin(),
         std::make_unique<ELFYAML::Section>(
             ELFYAML::Section::SectionKind::RawContent, /*IsImplicit=*/true));
   
-  std::vector<StringRef> ImplicitSections = {".symtab", ".strtab", ".shstrtab"};
+  std::vector<StringRef> ImplicitSections = {".strtab", ".shstrtab"};
+  if (Doc.Symbols) 
+    ImplicitSections.push_back(".symtab");
   if (!Doc.DynamicSymbols.empty())
     ImplicitSections.insert(ImplicitSections.end(), {".dynsym", ".dynstr"});
 
   // Insert placeholders for implicit sections that are not
   // defined explicitly in YAML.
   for (StringRef SecName : ImplicitSections) {
     if (DocSections.count(SecName))
       continue;

 
 template <class ELFT>
 void ELFState<ELFT>::initSymtabSectionHeader(Elf_Shdr &SHeader,
                                              SymtabType STType,
                                              ContiguousBlobAccumulator &CBA,
                                              ELFYAML::Section *YAMLSec) {
 
   bool IsStatic = STType == SymtabType::Static;
-  const auto &Symbols = IsStatic ? Doc.Symbols : Doc.DynamicSymbols;
+  const auto &Symbols = (IsStatic && Doc.Symbols) ? *Doc.Symbols : Doc.DynamicSymbols;
 
   ELFYAML::RawContentSection *RawSec =
       dyn_cast_or_null<ELFYAML::RawContentSection>(YAMLSec);
   if (RawSec && !Symbols.empty() && (RawSec->Content || RawSec->Size)) {
     if (RawSec->Content)
       reportError("cannot specify both `Content` and " +
                   (IsStatic ? Twine("`Symbols`") : Twine("`DynamicSymbols`")) +
                   " for symbol table section '" + RawSec->Name + "'");
template <class ELFT> void ELFState<ELFT>::buildSymbolIndexes() {
   auto Build = [this](ArrayRef<ELFYAML::Symbol> V, NameToIdxMap &Map) {
     for (size_t I = 0, S = V.size(); I < S; ++I) {
       const ELFYAML::Symbol &Sym = V[I];
       if (!Sym.Name.empty() && !Map.addName(Sym.Name, I + 1))
         reportError("repeated symbol name: '" + Sym.Name + "'");
     }
   };
 
-  Build(Doc.Symbols, SymN2I);
+  if (Doc.Symbols)
+    Build(*Doc.Symbols, SymN2I);
   Build(Doc.DynamicSymbols, DynSymN2I);
 }
 
 template <class ELFT> void ELFState<ELFT>::finalizeStrings() {
   // Add the regular symbol names to .strtab section.
-  for (const ELFYAML::Symbol &Sym : Doc.Symbols)
+  if (Doc.Symbols)
+    for (const ELFYAML::Symbol &Sym : *Doc.Symbols)
       DotStrtab.add(ELFYAML::dropUniqueSuffix(Sym.Name));
   DotStrtab.finalize();
 
   // Add the dynamic symbol names to .dynstr section.
   for (const ELFYAML::Symbol &Sym : Doc.DynamicSymbols)
     DotDynstr.add(ELFYAML::dropUniqueSuffix(Sym.Name));
 
   // SHT_GNU_verdef and SHT_GNU_verneed sections might also
   // add strings to .dynstr section.

llvm/test/tools/yaml2obj/symtab-generated.yaml

+## In this test we define a "Symbols" entry in the YAML and expect a .symtab
+## section to be generated. Notice that this is automatically added even though
+## we don't have .symtab listed in the "Sections" entry.
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj --sections %t | FileCheck -v --color --dump-input=always %s
+
+# CHECK:      Sections [
+# CHECK:        Section {
+# CHECK:          Name: .symtab (19)
+# CHECK-NEXT:     Type: SHT_SYMTAB (0x2)
+# CHECK-NEXT:     Flags [ (0x0)
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Address: 0x0
+# CHECK-NEXT:     Offset: 0x68
+# CHECK-NEXT:     Size: 48
+# CHECK-NEXT:     Link: 1
+# CHECK-NEXT:     Info: 2
+# CHECK-NEXT:     AddressAlignment: 8
+# CHECK-NEXT:     EntrySize: 24
+# CHECK-NEXT:  }
+# CHECK-NEXT:]
+# CHECK-EMPTY:
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Symbols:
+- Name:            main
+...

llvm/test/tools/yaml2obj/symtab-not-generated.yaml

+## In this test we don't explicitly define a "Symbols" entry in the YAML and
+## expect no .symtab section to be generated.
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readobj --sections %t | FileCheck %s
+
+# CHECK-NOT: Name: .symtab
+# CHECK-NOT: Type: SHT_SYMTAB
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS32
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_386

llvm/tools/obj2yaml/elf2yaml.cpp

if (!SymTab || SymTabShndx->sh_link != SymTab - Sections.begin())
           obj2yaml_error::not_implemented,
           "only SHT_SYMTAB_SHNDX associated with SHT_SYMTAB are supported");
 
     auto TableOrErr = Obj.getSHNDXTable(*SymTabShndx);
     if (!TableOrErr)
       return TableOrErr.takeError();
     ShndxTable = *TableOrErr;
   }
-  if (SymTab)
-    if (Error E = dumpSymbols(SymTab, Y->Symbols))
+  if (SymTab && Y->Symbols)
+    if (Error E = dumpSymbols(SymTab, *Y->Symbols))
       return std::move(E);
   if (DynSymTab)
     if (Error E = dumpSymbols(DynSymTab, Y->DynamicSymbols))
       return std::move(E);
 
   for (const Elf_Shdr &Sec : Sections) {
     switch (Sec.sh_type) {
     case ELF::SHT_DYNAMIC: {