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

Thu Oct 17 07:02:23 PDT 2019

grimar added inline comments.

================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:208
+    // Some sections wants to link to .symtab by default.
+    // That means we want to create the symbol table for them.
+    if (D->Type == llvm::ELF::SHT_REL || D->Type == llvm::ELF::SHT_RELA ||
----------------
kwk wrote:
> MaskRay wrote:
> > `if (Doc.Symbols) continue;` can be factored out.
> I agree with this optimization.
> 
> But I also wonder why you say that some section link to `.symtab` by default. So far I've only seen explicit references to `.symtab` in the `Link: .symtab` entry of a section in a test file. So why not change the test files to point to an existing section (you changed most of them anyways)? Or will a an empty `Link: ` entry automatically point to `.symtab`?
> But I also wonder why you say that some section link to .symtab by default. So far I've only seen explicit references to .symtab in the Link: .symtab entry of a section in a test file.
> Or will a an empty Link:  entry automatically point to .symtab?

For SHT_REL[A] we set it to `.symtab` when there is no `Link` at all (and .strtab is mandatory to have in this case atm):
https://github.com/llvm-mirror/llvm/blob/master/lib/ObjectYAML/ELFEmitter.cpp#L722

The same we do for `SHT_LLVM_ADDRSIG` (though in this case there will be no error when .strtab is missing).
https://github.com/llvm-mirror/llvm/blob/master/lib/ObjectYAML/ELFEmitter.cpp#L1007

================
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:
> 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.

================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:214-218
+    // Also sections might refer to the symbol table explicitly via Link.
+    // In this case we also must provide it, even when the YAML document does
+    // not have the "Symbols" tag.
+    if (!Doc.Symbols && D->Link == ".symtab")
+      Doc.Symbols = std::vector<ELFYAML::Symbol>();
----------------
rupprecht wrote:
> This case is interesting, partly because IIRC we raise an error if someone specifies a link to a section that does not exist, so special casing this seems unexpected. However, I imagine many tests would need to be fixed and it's maybe not worth it -- have you checked how many tests would fail if you didn't special case this?
Good point. There were about 20 of tests, but I've fixed them instead. It reduces the number of rules, what makes sence too I think.

================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:231
+    ImplicitSections.push_back(".symtab");
+  ImplicitSections.push_back(".strtab");
+  ImplicitSections.push_back(".shstrtab");
----------------
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.

================
Comment at: test/Object/invalid.test:295
     ShOffset: 0xFFFFFFFF
+Symbols: []

----------------
kwk wrote:
> Isn't `Symbols:` enough?
Yes, but `[ ]` shows the intention a bit better I think. 

================
Comment at: test/tools/yaml2obj/implicit-sections.test:88
+
+## Check we don't add a symbol table when no "Symbols" tag is spicified.
+
----------------
MaskRay wrote:
> kwk wrote:
> > Great, that you've put this all into one file.
> Typo: spicified
> 
> I think the YAML term is `mapping key`, or `key`, not `tag`. `Tag` refers to a different thing (https://yaml.org/spec/1.2/spec.html#id2764295).
> I think the YAML term is mapping key, or key, not tag. Tag refers to a different thing (https://yaml.org/spec/1.2/spec.html#id2764295).

Seems might want to fix another comments we already have :)

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

https://reviews.llvm.org/D69041