[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())
----------------
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?


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:212
+      if (!Doc.Symbols && D->Link.empty())
+        Doc.Symbols = std::vector<ELFYAML::Symbol>();
+
----------------
Doc.Symbols.emplace()

Same below.


================
Comment at: test/tools/yaml2obj/implicit-sections.test:88
+
+## Check we don't add a symbol table when no "Symbols" tag is spicified.
+
----------------
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).


================
Comment at: test/tools/yaml2obj/implicit-sections.test:91
+# RUN: yaml2obj --docnum=3 %s -o %t3
+# RUN: llvm-readelf -S %t3 | FileCheck %s --check-prefix=NOSYMTAB
+
----------------
This can be `FileCheck /dev/null --implicit-check-not=.symtab` if you feel it is clearer.


================
Comment at: test/tools/yaml2obj/implicit-sections.test:102
+
+## Check we add a symbol table when "Symbols" tag is spicified.
+
----------------
Typo: spicified -> specified


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

https://reviews.llvm.org/D69041