Page MenuHomePhabricator

[yaml2obj] - Do not ignore explicit addresses for .dynsym and .dynstr
ClosedPublic

Authored by grimar on Feb 13 2019, 2:35 AM.

Details

Summary

This fixes https://bugs.llvm.org/show_bug.cgi?id=40339

Previously if the addresses were set in YAML they were ignored for
.dynsym and .dynstr sections. The patch fixes that.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Feb 13 2019, 2:35 AM
grimar marked an inline comment as done.Feb 13 2019, 5:53 AM
grimar added inline comments.
test/tools/yaml2obj/dynsym-dynstr-addr.yaml
4 ↗(On Diff #186608)

I will change soes->does on next patch update/before the commit.

jhenderson added inline comments.Feb 18 2019, 6:01 AM
test/tools/yaml2obj/dynsym-dynstr-addr.yaml
5 ↗(On Diff #186608)

Nit: .dynstr -> .dynsym in one case

tools/yaml2obj/yaml2elf.cpp
324–325 ↗(On Diff #186608)

Nit: Let clang-format reflow your comments across multiple lines, rather than doing it by hand.

377 ↗(On Diff #186608)

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.

grimar marked 2 inline comments as done.Feb 18 2019, 6:18 AM
grimar added inline comments.
tools/yaml2obj/yaml2elf.cpp
324–325 ↗(On Diff #186608)

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).

377 ↗(On Diff #186608)

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.

grimar marked an inline comment as done.Feb 18 2019, 6:23 AM
grimar added inline comments.
tools/yaml2obj/yaml2elf.cpp
324–325 ↗(On Diff #186608)

"So you" -> "Do you"

grimar updated this revision to Diff 187251.Feb 18 2019, 8:32 AM
grimar marked an inline comment as done.
  • Fix comment in the test case.
jhenderson accepted this revision.Feb 19 2019, 2:00 AM

LGTM.

tools/yaml2obj/yaml2elf.cpp
324–325 ↗(On Diff #186608)

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.

377 ↗(On Diff #186608)

Never mind, I think I misread the code the first time.

This revision is now accepted and ready to land.Feb 19 2019, 2:00 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2019, 4:14 AM