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

[Konrad Wilhelm Kleine via Phabricator via llvm-commits]

kwk added a comment.

@grimar I don't understand why you created this patch, when I'm working on it already. That's a waste of my time.



================
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 ||
----------------
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`?


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:232
+  ImplicitSections.push_back(".strtab");
+  ImplicitSections.push_back(".shstrtab");
+
----------------
Change to `ImplicitSections.insert(ImplicitSections.end(), {".strtab", ".shstrtab"});`


================
Comment at: test/Object/invalid.test:295
     ShOffset: 0xFFFFFFFF
+Symbols: []
 
----------------
Isn't `Symbols:` enough?


================
Comment at: test/tools/llvm-objcopy/ELF/segment-test-remove-section.test:45
       - Section: .text3
+## TODO: without the following line (i.e. without an empty symbol table),
+## llvm-objcopy adds an empty .strtab section. It doesn't look correct.
----------------
I suggest to use `TODO(username): `.


================
Comment at: test/tools/yaml2obj/implicit-sections-types.test:28
+## Needed to force the creation of the .symtab.
+Symbols:
 ## Needed to force the creation of the .dynsym and .dynstr.
----------------
See, apparently it is enough ;)


================
Comment at: test/tools/yaml2obj/implicit-sections.test:88
+
+## Check we don't add a symbol table when no "Symbols" tag is spicified.
+
----------------
Great, that you've put this all into one file.


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

https://reviews.llvm.org/D69041