This is an archive of the discontinued LLVM Phabricator instance.

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

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

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

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
2

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

4

Nit: add a blank line before the YAML.

41

Move the check before the yaml. Flow should be:

## Comment
# RUN:
# CHECK:
Yaml

## Comment
# RUN:
# CHECK:
Yaml
43

The size of content -> The content size

equals to -> equals

102

size of content -> content size

103

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

llvm/test/ObjectYAML/MachO/virtual_section.yaml
179

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

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
179

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

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?

llvm/include/llvm/ObjectYAML/MachOYAML.h
203

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

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

llvm/test/ObjectYAML/MachO/section_data.yaml
44

Delete the second "content".

llvm/tools/obj2yaml/macho2yaml.cpp
21

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

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

Here.

llvm/tools/obj2yaml/macho2yaml.cpp
21

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

fair enough

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.