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

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

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

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

22

which fields are all zeroed -> with fields all zeroed

23

to the section created

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

41

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

What about Info, and Address and perhaps sh_offset?

76

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

95

Nit: double blank line

96

"The same as above"

114

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

130

Perhaps worth a slight rewording here:

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

142

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

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

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

76

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

tools/yaml2obj/yaml2elf.cpp
301

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

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

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

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

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

142

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
49

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"?

72

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

146

if we have

163–164

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