This is an archive of the discontinued LLVM Phabricator instance.

[yaml2obj/obj2yaml] - Make RawContentSection::Content and RawContentSection::Size optional
ClosedPublic

Authored by grimar on Jun 6 2019, 7:38 AM.

Details

Summary

This is a follow-up for D62809.

Content and Size fields should be optional as was discussed in comments
of the D62809's thread. With that, we can describe a specific string table and
symbol table sections in a more correct way and also show appropriate errors.

The patch adds lots of test cases where the behavior is described in details.

Diff Detail

Event Timeline

grimar created this revision.Jun 6 2019, 7:38 AM
grimar updated this revision to Diff 203365.Jun 6 2019, 7:43 AM
  • Minor cometic code change in writeRawSectionData.
grimar updated this revision to Diff 203366.Jun 6 2019, 7:53 AM
  • Use llvm-objdump -s instead of llvm-objdump -D for dumping sections content.
jhenderson added inline comments.Jun 6 2019, 8:57 AM
test/tools/yaml2obj/section-size-content.yaml
8

greater -> greater than

24

and the -> and that the

26

I've found it sometimes useful for each test case output to be a separate file i.e. use %t, %t2, %t3 etc instead of always %t.

82

In this case the zeroes padding is added -> In this case zeroes are added as padding

83

What is the bit in brackets referring to?

103–105

I think this case is the same as the --docnum=2 test case, right? You can use --section-data to get the section data if you want via llvm-readelf too.

125

or -> nor

test/tools/yaml2obj/strtab-implicit-sections-size-content.yaml
4

for string -> for a string

5

I think you can simplify this to "yaml2obj writes the default content."

26

remove "along" (probably should be "alone" but still not neeed).

27

"The content is filled with zeroes in this case."

52

Remove "along"

76

remove "or equal to"

76–77

"In this case zeroes are added as padding after the specified content"

Remove the bit in parentheses.

101–102

"when size is equal to content size." Then delete the rest of the comment.

test/tools/yaml2obj/symtab-implicit-sections-size-content.yaml
5

Same comments from the previous test apply here throughout this test.

63

emit the custom -> emit custom

82

Ditto

85

You should be able to use llvm-readelf --section-data or --hex-dump here and below rather than this to check the output.

104–106

See comments in previous test for this and next test case.

tools/yaml2obj/yaml2elf.cpp
403

You probably need some test cases for DynamicSymbols as well as static Symbols.

grimar marked 3 inline comments as done.Jun 6 2019, 9:26 AM
grimar added inline comments.
test/tools/yaml2obj/section-size-content.yaml
83

What is the bit in brackets referring to?

We have content of size 1 and Size == 3:

Content: "FF"
Size:    3

I.e. I check here that "we can specify both Size and Content when size is greater than...."

Since you've noticed (thanks!) that test below (--docnum=6) is actually a duplicate of --docnum=2,
I'll rewrite this comment.

103–105

I think this case is the same as the --docnum=2.

Oh, indeed. I'll fix.

tools/yaml2obj/yaml2elf.cpp
403

OK, I'll add.

Do you think it worth to add tests for other 2 string sections (.shstrtab, .dynstr) too?

jhenderson added inline comments.Jun 7 2019, 2:19 AM
tools/yaml2obj/yaml2elf.cpp
403

I was wondering this. Those use the same code paths as .strtab, I believe, and there is no differences in behaviour, I think? I don't think it hurts to explicitly test them though necessarily, but don't think it critical by any means.

grimar marked an inline comment as done.Jun 7 2019, 2:31 AM
grimar added inline comments.
tools/yaml2obj/yaml2elf.cpp
403

I was wondering this. Those use the same code paths as .strtab, I believe, and there is no differences in behaviour, I think?

Yes, the path is the same and the difference should be negligible. Probably no need to duplicate the existing large test case I think.

grimar updated this revision to Diff 203542.EditedJun 7 2019, 5:50 AM
grimar marked 20 inline comments as done.
  • Addressed review comments.

Upd: initially I wrote here that it depends on D63002 now,
but it wasn't true. A new test case added in the latest diff contains
the following TODO:

## TODO: .dynstr here is needed to set a proper Link for .dynsym.
## Otherwize llvm-readobj would report an error.
## We might want to fix llvm-readobj.

I had to add .dynstr because otherwise llvm-readobj does not want
to dump the object. (because .dynsym's Link field == 0).

Without .dynstr added this would depend on D63002.

I am going to investigate how much hard to fix the llvm-readobj.

jhenderson added inline comments.Jun 7 2019, 7:38 AM
test/tools/yaml2obj/dynsymtab-implicit-sections-size-content.yaml
5 ↗(On Diff #203542)

Maybe worth explicitly saying "dynamic symbol table section"

98 ↗(On Diff #203542)

As noted above, this deserves a reference to a bug that has been filed.

The comment needs some updates too:
Otherwize (should be Otherwise by the way) llvm-readobj would report -> Without this, llvm-readeobj reports...

136 ↗(On Diff #203542)

Ditto.

145 ↗(On Diff #203542)

after the specified content

177 ↗(On Diff #203542)

See above.

217 ↗(On Diff #203542)

See above.

test/tools/yaml2obj/section-size-content.yaml
106

after content specified -> after the specified content

test/tools/yaml2obj/strtab-implicit-sections-size-content.yaml
99

Same as above.

test/tools/yaml2obj/symtab-implicit-sections-size-content.yaml
128

See above.

grimar updated this revision to Diff 203792.Jun 10 2019, 3:08 AM
grimar marked 7 inline comments as done.
  • Addressed review comments.

Looks like you missed some comments?

test/tools/yaml2obj/section-size-content.yaml
106

This hasn't been done?

test/tools/yaml2obj/strtab-implicit-sections-size-content.yaml
99

Ditto

test/tools/yaml2obj/symtab-implicit-sections-size-content.yaml
128

Ditto

grimar updated this revision to Diff 203808.Jun 10 2019, 5:14 AM
grimar marked an inline comment as done.
  • Addressed review comments.
test/tools/yaml2obj/section-size-content.yaml
106

Searched for these wording in the wrong file it seeems. Sorry :(

This revision is now accepted and ready to land.Jun 10 2019, 5:30 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2019, 5:40 AM