[PATCH] D56569: [ObjectYAML][yaml2obj][ELF] Add basic support for dynamic entries

[Armando Montanez via Phabricator via llvm-commits]

amontanez marked an inline comment as done.
amontanez added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/ELFYAML.h:94
+  ELF_DT Tag;
+  Optional<uint64_t> Val;
+  Optional<llvm::yaml::Hex64> Ptr;
----------------
ruiu wrote:
> amontanez wrote:
> > ruiu wrote:
> > > jhenderson wrote:
> > > > Somewhat moot if you follow @ruiu's suggestion (which I support), but this should be an int64_t.
> > > Hmm, why is this of type `StringRef`? Isn't this `uint64_t`?
> > Making `Value` a StringRef gives more flexibility for how the value of a dynamic entry can be populated. I've listed a few examples below that illustrate how this is currently used.
> > 
> > Using a section name rather than manually entering the address of a section:
> > ```
> > Tag: DT_STRTAB
> > Value: .dynstr
> > ```
> > 
> > 
> > Using a string that will be added to .dynstr:
> > ```
> > Tag: DT_SONAME
> > Value: libsomething.so
> > ```
> > 
> > Or, the simple case of using a numeric value:
> > ```
> > Tag: DT_STRSZ
> > Value:  0xa0
> > ```
> > 
>   Tag: DT_STRTAB
>   Value: .dynstr
> 
> What if two or more sections have the same name ".dynstr"? In ELF, at least in spirit, section names are not significant.
> 
> I feel that in order to discuss the final file format of the text-based ELF in YAML, code review is not the best place. Also, I think that we shouldn't discuss one YAML tag at a time. I'd rather want to take a look at the final form of the text-based ELF file so that I can get a big picture. Do you have such example? It doesn't have to be a formal document or anything, but just an example or two of a text-based ELF file should suffice.
> What if two or more sections have the same name ".dynstr"? In ELF, at least in spirit, section names are not significant.

I completely agree with your sentiment here. yaml2elf requires section names to be unique (see `buildSectionIndex()`), and a large portion of the tool's design depends on this. I know this isn't the case for regular ELF files, but it's a design choice this tool makes and a non-trivial amount of yaml2elf would have to be re-written to change that.

I'll draft up a few examples and add them to the main comment thread of this patch so we can focus on that discussion.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56569