[PATCH] D58168: [yaml2obj] - Do not ignore explicit addresses for .dynsym and .dynstr

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: tools/yaml2obj/yaml2elf.cpp:324-325
 
+  // If .dynsym section is explicitly described in the YAML
+  // then we want to use its section address.
+  if (!IsStatic) {
----------------
grimar wrote:
> grimar wrote:
> > jhenderson wrote:
> > > Nit: Let clang-format reflow your comments across multiple lines, rather than doing it by hand.
> > Clang format would produce the following:
> > 
> > ```
> >   // If .dynsym section is explicitly described in the YAML then we want to use
> >   // its section address.
> > ```
> > 
> > The first line is long, while the second is too short. It does not look nice. So you still want me to do that?
> > ( I am often doing that by hand in such cases, btw).
> "So you" -> "Do you"
> 
I guess it's a personal style thing. It just looks weird when I see short lines in comments. I don't feel strongly about it if you don't want to do it.


================
Comment at: tools/yaml2obj/yaml2elf.cpp:377
+    if (SecNdx < Doc.Sections.size())
+      SHeader.sh_addr = Doc.Sections[SecNdx]->Address;
+  }
----------------
grimar wrote:
> jhenderson wrote:
> > Do you think this should have a sanity assertion to show that Doc.Sections[SecNdx] is the right section? If I'm right, this only works because an auto-generated .dynstr happens to be added to the end of the sections, but it would not be unreasonable to change that behaviour.
> I am using `getDotDynStrSecNo()` here to get the section index.
> It implementation just maps a section name to index:
> https://github.com/llvm-mirror/llvm/blob/master/tools/yaml2obj/yaml2elf.cpp#L178
> 
> We build the `SN2I` mapping in `buildSectionIndex` here:
> https://github.com/llvm-mirror/llvm/blob/master/tools/yaml2obj/yaml2elf.cpp#L605
> (i.e. before calling this place)
> 
> So ordering is not important I believe.
Never mind, I think I misread the code the first time.


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

https://reviews.llvm.org/D58168