This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj] - Add a support for defining null sections in YAMLs.
ClosedPublic

Authored by grimar on Jul 18 2019, 5:04 AM.

Details

Summary

ELF spec shows (Figure 4-10: Section Header Table Entry:Index 0,
http://www.sco.com/developers/gabi/latest/ch4.sheader.html)
that section header at index 0 (null section) can have sh_size and
sh_link fields set to non-zero values.

It says (https://docs.oracle.com/cd/E19683-01/817-3677/6mj8mbtc9/index.html):

If the number of sections is greater than or equal to SHN_LORESERVE (0xff00),
this member has the value zero and the actual number of section header table
entries is contained in the sh_size field of the section header at index 0.
Otherwise, the sh_size member of the initial entry contains 0.

and:

If the section name string table section index is greater than or equal to SHN_LORESERVE
(0xff00), this member has the value SHN_XINDEX (0xffff) and the actual index of the section
name string table section is contained in the sh_link field of the section header at index 0.
Otherwise, the sh_link member of the initial entry contains 0.

At this moment it is not possible to create custom section headers at index 0 using yaml2obj.
This patch implements this.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Jul 18 2019, 5:04 AM

In the description:

name string table section is contained in thesh_link field of the section header at index 0.

Typo: thesh_link -> the sh_link

tools/yaml2obj/yaml2elf.cpp
135 ↗(On Diff #210529)

Period at the end.

grimar edited the summary of this revision. (Show Details)Jul 19 2019, 3:21 AM
grimar updated this revision to Diff 210792.Jul 19 2019, 3:23 AM
grimar marked an inline comment as done.
  • Addressed review comment.
jhenderson added inline comments.Jul 19 2019, 5:09 AM
test/tools/yaml2obj/elf-custom-null-section.yaml
6 ↗(On Diff #210792)

Calling this prefix CASE1 when it is used by two test cases is a bit confusion. I recommend "DEFAULT" or something like that.

22 ↗(On Diff #210792)

which fields are all zeroed -> with fields all zeroed

23 ↗(On Diff #210792)

to the section created

(don't need both the "normally" and "by default").

41 ↗(On Diff #210792)

Specify Link explicitly with an integer here (i.e. Link: 0).

What about Info, and Address and perhaps sh_offset?

76 ↗(On Diff #210792)

Similar to above, this is called "CASE3" but is used in two different test cases. Perhaps REDEFINE?

95 ↗(On Diff #210792)

Nit: double blank line

96 ↗(On Diff #210792)

"The same as above"

114 ↗(On Diff #210792)

a error -> an error
to unknown -> to an unknown

130 ↗(On Diff #210792)

Perhaps worth a slight rewording here:

"Check that null section fields are set to zero, if they are unspecified."

142 ↗(On Diff #210792)

How about a test case for a SHT_NULL section that is not the first listed section? I don't think there's anything in the ELF spec saying that you can't have one elsewhere in the section header table.

tools/yaml2obj/yaml2elf.cpp
301 ↗(On Diff #210792)

I wonder if it would make sense to treat the null section the same as any other implicit section? What do you think?

It says (https://docs.oracle.com/cd/E19683-01/817-3677/6mj8mbtc9/index.html):

The Solaris Linker and Libraries Guide can serve as a reference if the official ELF spec is missing something that we agree upon, but for the SHN_LORESERVE thing you can just quote http://www.sco.com/developers/gabi/latest/ch4.eheader.html :

e_shnum
...
If the number of sections is greater than or equal to SHN_LORESERVE (0xff00), this member has the value zero and the actual number of section header table entries is contained in the sh_size field of the section header at index 0. (Otherwise, the sh_size member of the initial entry contains 0.)

e_shstrndx
...
If the section name string table section index is greater than or equal to SHN_LORESERVE (0xff00), this member has the value SHN_XINDEX (0xffff) and the actual index of the section name string table section is contained in the sh_link field of the section header at index 0. (Otherwise, the sh_link member of the initial entry contains 0.)

MaskRay added inline comments.Jul 21 2019, 10:43 PM
test/tools/yaml2obj/elf-custom-null-section.yaml
46 ↗(On Diff #210792)

CASE2 -> a descriptive name. (I'd call it EXPLICIT-ZERO but there may be a better name).

76 ↗(On Diff #210792)

Or
CASE3 -> SEC-REF-LINK
CASE4 -> NUMERAL-LINK

tools/yaml2obj/yaml2elf.cpp
301 ↗(On Diff #210792)

How to do that? SHN_UNDEF (of SHT_NULL) is the first section, different from other implicit sections...

grimar marked 3 inline comments as done.Jul 21 2019, 11:44 PM
grimar added inline comments.
test/tools/yaml2obj/elf-custom-null-section.yaml
41 ↗(On Diff #210792)

What about Info, and Address and perhaps sh_offset?

I focused on a use cases which are correct from ELF spec view in this patch.

Adding support for another fields can be a part of follow-up refactoring.
(I think we might be able to handle SHT_NULL in the same loop as other sections in initSectionHeaders,
that would allow to support setting all other fields at once).

tools/yaml2obj/yaml2elf.cpp
301 ↗(On Diff #210792)

Yes, it is very different. I also do not see a clean way to handle it as a regular implicit section.
It would require lot of changes everywhere, at least because we place implicit sections to the end,
what doesn't work for SHT_NULL.

But seems the solution is to land D64999 first. It should allow to significantly simplify this patch.

grimar planned changes to this revision.Jul 22 2019, 6:30 AM
grimar marked 3 inline comments as done.
grimar updated this revision to Diff 211098.Jul 22 2019, 7:38 AM
grimar marked 11 inline comments as done and 2 inline comments as done.
  • Reimplemented.

Now rebased on top of D65087.

test/tools/yaml2obj/elf-custom-null-section.yaml
41 ↗(On Diff #210792)

What about Info, and Address and perhaps sh_offset?

Oops, I misread slightly. I added Info and Address zeroed fields here.

This patch still doesn't support setting them to something else though.
I didn't add ShOffset or ShSize, this is out of the area of this patch and
anyways not supported either.

46 ↗(On Diff #210792)

I used ZERO-VS-NONZERO because the test case above already explicitly defines the zero section.

142 ↗(On Diff #210792)

I think that wasn't never intentionally supported, I added a test to check we do not crash.
Not sure if we want to check something else here.

jhenderson added inline comments.Jul 22 2019, 9:38 AM
test/tools/yaml2obj/elf-custom-null-section.yaml
48 ↗(On Diff #211098)

I don't like "ZERO-VS-NONZERO" here, because it's not really anything to do with that. This test case is for testing that other sections can be added after the NULL section. "ZERO-VS-NONZERO" doesn't clarify that. How about "OTHER-SECTION"?

71 ↗(On Diff #211098)

Nit: Link: 0 (be consistent with above change).

145 ↗(On Diff #211098)

if we have

162–163 ↗(On Diff #211098)

Perhaps worth setting some fields to different explicit values, to show that it's possible.

grimar updated this revision to Diff 211257.Jul 23 2019, 12:49 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
jhenderson accepted this revision.Jul 23 2019, 3:23 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Jul 23 2019, 3:23 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2019, 4:04 AM