Skip to content

Conversation

@mistydemeo
Copy link
Contributor

No description provided.

@mistydemeo mistydemeo requested a review from woodruffw June 5, 2018 17:58
:LC_VERSION_MIN_TVOS => "VersionMinCommand",
:LC_VERSION_MIN_WATCHOS => "VersionMinCommand",
:LC_NOTE => "LoadCommand",
:LC_BUILD_VERSION => "VersionMinCommand",
Copy link
Member

@woodruffw woodruffw Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably needs a new class, based on the corresponding structure in LLVM's Mach-O parser:

    struct build_version_command {
        uint32_t    cmd;        /* LC_BUILD_VERSION */
        uint32_t    cmdsize;    /* sizeof(struct build_version_command) plus */
        /* ntools * sizeof(struct build_tool_version) */
        uint32_t    platform;   /* platform */
        uint32_t    minos;      /* X.Y.Z is encoded in nibbles xxxx.yy.zz */
        uint32_t    sdk;        /* X.Y.Z is encoded in nibbles xxxx.yy.zz */
        uint32_t    ntools;     /* number of tool entries following this */
    };

Edit: and this structure comes along for the ride:

     struct build_tool_version {
       uint32_t tool;      // enum for the tool
       uint32_t version;   // version of the tool
  };

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(For comparison, version_min_command has just two fields: version and sdk).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change made and tested with a binary. No build_tool_version yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TwoLevelHintsCommand might be a good reference for adding build_tool_version, since it encapsulates its own table/array structure.

Copy link
Contributor Author

@mistydemeo mistydemeo Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, updated. We weren't handed an offset to read the table from so I'm making the assumption here that it's the offset following view.offset + 24.

While we're not given much info about the tool structure, I am seeing what looks like correct information from what we do have:

irb(main):005:0> command.platform #=> 1, e.g. macOS
irb(main):006:0> command.minos_string #=> "10.14.0"
irb(main):007:0> command.sdk_string #=> "10.14.0"
irb(main):008:0> command.tool_entries.tools.first.tool #=> 3, e.g. ld
irb(main):009:0> command.tool_entries.tools.first.version #=> 26738945, format of this number not documented

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We weren't handed an offset to read the table from so I'm making the assumption here that it's the offset following view.offset + 24.

Yeah, I think this is a reasonable assumption.

:LC_LINKER_OPTIMIZATION_HINT => "LinkeditDataCommand",
:LC_VERSION_MIN_TVOS => "VersionMinCommand",
:LC_VERSION_MIN_WATCHOS => "VersionMinCommand",
:LC_NOTE => "LoadCommand",
Copy link
Member

@woodruffw woodruffw Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on http://llvm.org/doxygen/Support_2MachO_8h_source.html, this is the corresponding structure:

     struct note_command {
       uint32_t cmd;        // LC_NOTE
       uint32_t cmdsize;    // sizeof(struct note_command)
       char data_owner[16]; // owner name for this LC_NOTE
       uint64_t offset;     // file offset of this data
       uint64_t size;       // length of data region
  };

It looks like LC_NOTE is only being used in MH_CORE files, though, so this doesn't need to be implemented in this PR 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I haven't encountered it yet; I just added it since I was adding 0x32, which I have encountered. 😄

@mistydemeo mistydemeo force-pushed the new_load_commands branch 4 times, most recently from 3785fee to 0ec9023 Compare June 5, 2018 19:44
# @return [Integer] the SDK version X.Y.Z packed as x16.y8.z8
attr_reader :sdk

# @return ToolEntries tool entries
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: ToolEntries -> [ToolEntries] for YARD

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

@woodruffw
Copy link
Member

Other than that nit, this looks good to me! Thanks a ton @mistydemeo 😄

@mistydemeo mistydemeo force-pushed the new_load_commands branch from 0ec9023 to 9f8f867 Compare June 5, 2018 21:20
@mistydemeo
Copy link
Contributor Author

Great! Thanks for the review. I'll merge this when CI goes green.

@mistydemeo mistydemeo merged commit bd8aa3c into master Jun 5, 2018
@mistydemeo mistydemeo deleted the new_load_commands branch June 5, 2018 21:29
@lock lock bot added the outdated label Jan 1, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants