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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

Generally LGTM, although I think Ray is a much better person to approve this.



================
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())
----------------
MaskRay wrote:
> 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.
> For SHT_GROUP and SHT_SYMTAB_SHNDX, we don't have implicit sh_link = .symtab rule.

I'm not sure I understand this bit. Why not? According to my link, sh_link for those is "The section header index of the associated symbol table" -- is there a reference saying otherwise? (Not saying you're wrong, just looking for clarification).


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:231
+    ImplicitSections.push_back(".symtab");
+  ImplicitSections.push_back(".strtab");
+  ImplicitSections.push_back(".shstrtab");
----------------
grimar wrote:
> rupprecht wrote:
> > Is `.strtab` also something we can drop if `.symtab` doesn't exist?
> Yes, seems we can do this. I tried and observed a few test case failtures (because of section indices changes mostly it seems),
> and a crash (seems we need to handle a case when there is no "Symbols:" key, but there is a SHT_SYMTAB described.
> It wants to have a .strtab currently).
> Shouldn't be too hard to fix, but if you do not mind, I'd also leave it for a one more independent follow-up.
> 
SGTM, no rush


================
Comment at: test/tools/llvm-objcopy/ELF/add-symbol-new-symbol-visibility.test:18-20
+## TODO: llvm-objcopy crashes without the following line.
+## i.e. it crashes when there is no symbol table in the input.
+Symbols: []
----------------
Hopefully you can revert these files after rL375105


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

https://reviews.llvm.org/D69041