Page MenuHomePhabricator

[yaml2obj][MachO] Don't fill dummy data for virtual sections
ClosedPublic

Authored by seiya on Jun 6 2019, 6:30 PM.

Details

Summary

Currently, MachOWriter::writeSectionData writes dummy data (0xdeadbeef) to fill section data areas in the file even if the section is a virtual one. Since virtual sections don't occupy any space in the file, writing dummy data could results the "OS.tell() - fileStart <= Sec.offset" assertion failure.

This patch fixes the bug by simply not writing any dummy data for virtual sections.

Diff Detail

Repository
rL LLVM

Event Timeline

seiya created this revision.Jun 6 2019, 6:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2019, 6:30 PM
seiya added a comment.Jun 6 2019, 6:35 PM

Added reviewers who may be relevant to part and my GSoC mentors.

khm, could you please explain the rationale for this change ? If yaml2obj completely ignores virtual sections we won't be able to create a MachO binary containing them, moreover, even in some simple cases the round trip conversion yaml -> obj -> yaml will create a completely different binary. Having ability to create binaries with virtual sections overall seems to be useful for testing various tools etc.

seiya retitled this revision from [yaml2obj][MachO] Support virtual sections to [yaml2obj][MachO] Don't fill dummy data for virtual sections.Jun 7 2019, 4:12 AM
seiya edited the summary of this revision. (Show Details)
seiya added a comment.EditedJun 7 2019, 4:22 AM

khm, could you please explain the rationale for this change ? If yaml2obj completely ignores virtual sections we won't be able to create a MachO binary containing them, moreover, even in some simple cases the round trip conversion yaml -> obj -> yaml will create a completely different binary. Having ability to create binaries with virtual sections overall seems to be useful for testing various tools etc.

The description was insufficient and updated the summary. This patch is intended for fixing the problem so that we can create a MachO binary containing virtual sections

Currently, an assertion (OS.tell() - fileStart <= Sec.offset ...) in yaml2macho.cpp could fails if there are virtual sections in the yaml file: it writes Sec.size-sized dummy data as section data even if the section is virtual and finally OS.tell() - fileStart goes beyond the file offset of the next section (Sec.offset).

This patch fixes it by simply not (what "ignore" meant in the previous summary) writing dummy data if the section is virtual.

alexshap accepted this revision.Jun 10 2019, 2:50 PM

ok, this makes sense to me now, thanks for the explanation. LGTM with a couple of inline comments

llvm/tools/yaml2obj/yaml2macho.cpp
265 ↗(On Diff #203474)

I have a couple of comments here:

  1. in general i think this should be moved out of yaml2macho.cpp into the library (libObject ?)

because I believe it's not the only place where this might be useful. I think this can be done as a follow up though.

  1. the test probably can be a bit more robust (and check all these cases at once), but I don't insist.
This revision is now accepted and ready to land.Jun 10 2019, 2:50 PM
compnerd added inline comments.
llvm/test/ObjectYAML/MachO/virtual_section.yaml
85 ↗(On Diff #203474)

I think that the __TEXT,__eh_frame is unnecessary for the test as is __LD,__compact_unwind and ___DATA,__common. It would be nice to have the TLS and the large zero-filled section in the test.

seiya marked an inline comment as done.Jun 10 2019, 6:24 PM
seiya added inline comments.
llvm/tools/yaml2obj/yaml2macho.cpp
265 ↗(On Diff #203474)

I was wondering about the first comment. Some methods like MachOObjectFile::isSectionVirtual in libObject are what we need but ObjectYAML employs its own data structure MachOYAML::Object.

Adding some macros or inline functions into include/llvm/BinaryFormat/MachO.h and refactoring libObject as well as here using them would be better way I suppose. This should be separate patch by the way.

seiya updated this revision to Diff 203955.EditedJun 10 2019, 7:16 PM
  • Added a TLS section in the test.
  • Removed irrelevant sections such as __TEXT,__eh_frame.
seiya marked an inline comment as done.Jun 10 2019, 7:26 PM
seiya added inline comments.
llvm/test/ObjectYAML/MachO/virtual_section.yaml
85 ↗(On Diff #203474)

Hmm, in my environment (macOS 10.14.5), clang -m32 emits a corrupted object file if the input has large (4GiB) zero-filled section. Do you know an example of S_GB_ZEROFILL sections?

That said, possibly S_GB_ZEROFILL is no longer used because we use 64-bit binaries.

alexshap added inline comments.Jun 11 2019, 3:51 AM
llvm/tools/yaml2obj/yaml2macho.cpp
265 ↗(On Diff #203474)

yeah

i think we can commit this

seiya added a comment.Sun, Jun 16, 6:52 PM

Thank you I'll commit this.

This revision was automatically updated to reflect the committed changes.