[PATCH] D69041: [yaml2obj][obj2yaml] - Do not create a symbol table by default.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:209-210
+    // That means we want to create the symbol table for them.
+    if (D->Type == llvm::ELF::SHT_REL || D->Type == llvm::ELF::SHT_RELA ||
+        D->Type == llvm::ELF::SHT_LLVM_ADDRSIG)
+      if (!Doc.Symbols && D->Link.empty())
----------------
grimar wrote:
> MaskRay wrote:
> > rupprecht wrote:
> > > I think SHT_HASH, SHT_GROUP, and SHT_SYMTAB_SHNDX might also belong here, per: http://www.sco.com/developers/gabi/latest/ch4.sheader.html#sh_link
> > > 
> > > At that point, it might look cleaner to refactor out the list as a separate variable that is checked for list/set containment
> > sh_link(.hash) = .dynsym (not .symtab)
> > 
> > For SHT_GROUP and SHT_SYMTAB_SHNDX, we don't have implicit sh_link = .symtab rule.
> > 
> > It does raise the question: shall we keep the number of implicit .symtab rules low? Are we happy to keep the implicit rule for SHT_REL and SHT_RELA?
> > At that point, it might look cleaner to refactor out the list as a separate variable that is checked for list/set containment
> 
> > It does raise the question: shall we keep the number of implicit .symtab rules low? Are we happy to keep the implicit rule for SHT_REL and SHT_RELA?
> 
> Interesting question. So what about we do a change to yaml2obj to remove the additional logic added here?
> I.e.:
> 
> 1) <I've done this> Remove the logic for SHT_LLVM_ADDRSIG from here. SHT_LLVM_ADDRSIG just sets the Link to 0 when there is no .symtab already. So it was a quick change on a test suite side.
> 2) Keep the logic for SHT_REL and SHT_RELA here in this patch for now.
> 3) Remove it and change our logic in a follow up, i.e. change it to stop producing errors
>     for relocation sections when there is no `.symtab` section. Rel[A] Sections will start to get 0 as a Link value.
>     Just like we do for SHT_LLVM_ADDRSIG currently.
All points look good. We can do 1 and 2 in this patch. We can defer 3 to a future change.


================
Comment at: tools/obj2yaml/elf2yaml.cpp:205
+  if (SymTab) {
+    Y->Symbols = std::vector<ELFYAML::Symbol>();
+    if (Error E = dumpSymbols(SymTab, *Y->Symbols))
----------------
Y->Symbols.emplace();


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69041/new/

https://reviews.llvm.org/D69041