Page MenuHomePhabricator

[yaml2obj/obj2yaml][MachO] Allow setting custom section data
ClosedPublic

Authored by seiya on Aug 6 2019, 4:06 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

seiya created this revision.Aug 6 2019, 4:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2019, 4:06 AM

Have you rebased recently? I feel like the content in yaml2macho.cpp may have moved to the ObjectYAML library recently.

llvm/lib/Object/MachOObjectFile.cpp
1948 ↗(On Diff #213578)

A 32-bit Offset but 64-bit Size seems off to me. What are the limitations on the two fields in Mach-O's file format?

llvm/test/ObjectYAML/MachO/section_data.yaml
1 ↗(On Diff #213578)

Probably worth a header comment summarizing the purpose of the test, e.g. "Show that yaml2obj supports custom section data for Mach-O YAML inputs."

3 ↗(On Diff #213578)

Nit: add a blank line before the YAML.

40 ↗(On Diff #213578)

Move the check before the yaml. Flow should be:

## Comment
# RUN:
# CHECK:
Yaml

## Comment
# RUN:
# CHECK:
Yaml
42 ↗(On Diff #213578)

The size of content -> The content size

equals to -> equals

101 ↗(On Diff #213578)

size of content -> content size

102 ↗(On Diff #213578)

are filled -> is filled
by zero -> with zeroes

llvm/test/ObjectYAML/MachO/virtual_section.yaml
178 ↗(On Diff #213578)

Why are these no longer CHECK-NEXT? Should you be testing the new stuff before it?

Have you rebased recently? I feel like the content in yaml2macho.cpp may have moved to the ObjectYAML library recently.

I hadn't actually done that yet but just did so in rL368021

seiya updated this revision to Diff 214064.Aug 7 2019, 10:09 PM
seiya marked 10 inline comments as done.

Addressed review comments.

seiya marked an inline comment as done.Aug 7 2019, 10:29 PM
seiya added inline comments.
llvm/lib/Object/MachOObjectFile.cpp
1948 ↗(On Diff #213578)

This comes from the definition of struct section_64 [1].

[1]: https://github.com/opensource-apple/cctools/blob/master/include/mach-o/loader.h#L427-L428

llvm/test/ObjectYAML/MachO/virtual_section.yaml
178 ↗(On Diff #213578)

I removed the -NEXT because the newly added content fields were lengthy but it was bad idea. I've replaced them with regular expressions instead.

llvm/tools/obj2yaml/macho2yaml.cpp
21 ↗(On Diff #214064)

I'd like to move this function into somewhere else instead of copying from MachOEmitter.cpp but I couldn't locate the appropriate place. Any suggestions?

alexshap added inline comments.Aug 7 2019, 10:39 PM
llvm/include/llvm/ObjectYAML/MachOYAML.h
203 ↗(On Diff #214064)

khm, to be honest, I would not place it here , these template specializations provide the static method "mapping" only. Somehow, I don't see where this method ("validate") is being used at the moment - maybe we can either delete it or hide it inside MachOYAML.cpp

jhenderson added inline comments.Aug 8 2019, 2:25 AM
llvm/lib/ObjectYAML/MachOEmitter.cpp
322 ↗(On Diff #214064)

Could you add the missing trailing full stop whilst you're here, please?

llvm/test/ObjectYAML/MachO/section_data.yaml
44 ↗(On Diff #214064)

Delete the second "content".

llvm/tools/obj2yaml/macho2yaml.cpp
21 ↗(On Diff #214064)

Two suggestions:

  1. MachOYAML.cpp/.h
  2. BinaryFormat/MachO.h (assuming it could be useful in a wider context).
seiya updated this revision to Diff 214320.Aug 9 2019, 12:27 AM
seiya marked 7 inline comments as done.

Addressed review comments.

seiya marked an inline comment as done.Aug 9 2019, 12:28 AM
seiya added inline comments.
llvm/include/llvm/ObjectYAML/MachOYAML.h
203 ↗(On Diff #214064)

validate is an optional method in MappingTraits and it's implemented in MachOYAML.cpp.

I think this should be here to validate obviously invalid input by YAMLTraits.h since it provides human-friendly error messages like:

YAML:30:9: error: Section size must be greater than or equal to the content size
      - sectname:        __data
        ^
yaml2obj: error: Failed to parse YAML input!
llvm/lib/ObjectYAML/MachOYAML.cpp
294 ↗(On Diff #214320)

Here.

llvm/tools/obj2yaml/macho2yaml.cpp
21 ↗(On Diff #214064)

Moved into the BinaryFormat/MachO.h. Thanks!

jhenderson accepted this revision.Aug 9 2019, 8:24 AM

LGTM. You might want @alexshap to confirm before landing this though.

This revision is now accepted and ready to land.Aug 9 2019, 8:24 AM

LG, thanks for working on this!

llvm/include/llvm/ObjectYAML/MachOYAML.h
203 ↗(On Diff #214064)

fair enough

alexshap accepted this revision.Aug 9 2019, 1:26 PM
seiya updated this revision to Diff 215823.Aug 18 2019, 10:30 PM

Rebase for the sake of arc patch.

This revision was automatically updated to reflect the committed changes.