[PATCH] D39582: Add ELF dynamic symbol support to yaml2obj/obj2yaml

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

In general this looks like a good approach to me.  I have concerns about deciding section indexes for users as mentioned but other than that I just have a couple nits.



================
Comment at: tools/yaml2obj/yaml2elf.cpp:162-163
   unsigned getDotShStrTabSecNo() const { return Doc.Sections.size() + 3; }
-  unsigned getSectionCount() const { return Doc.Sections.size() + 4; }
+  unsigned getDotDynSymSecNo() const { return Doc.Sections.size() + 4; }
+  unsigned getDotDynStrSecNo() const { return Doc.Sections.size() + 5; }
+  unsigned getSectionCount() const {
----------------
.dyn* sections are going to be allocated and so we need to be able place them next to certain other allocated sections so that we can't put them in program headers. It won't be sufficient to decide for the user, at least not this way. One way might be to require that the user add a .dynsym section if they want to use DynamicSymbols and then using the index of that section if they do. This will require that you compute the size of the .dynsym and .dynstr sections early enough for the standard layout algorithm to work.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:301
   zero(SHeader);
-  SHeader.sh_name = DotShStrtab.getOffset(".symtab");
-  SHeader.sh_type = ELF::SHT_SYMTAB;
-  SHeader.sh_link = getDotStrTabSecNo();
+  bool IsStatic = STType == SymtabType::Static ? true : false;
+  SHeader.sh_name = DotShStrtab.getOffset(IsStatic ? ".symtab" : ".dynsym");
----------------
Can this just be "bool IsStatic = STType == SymtabType::Static"? Better yet do we really need SymtabType? What if we just pass "IsStatic" as a parameter?


================
Comment at: tools/yaml2obj/yaml2elf.cpp:311
   SHeader.sh_addralign = 8;
 
   std::vector<Elf_Sym> Syms;
----------------
.dynsym should be allocated so sh_flags should be set to SHF_ALLOC if IsStatic is true. 


https://reviews.llvm.org/D39582