Page MenuHomePhabricator

[Object] Extend MachOUniversalBinary::getObjectForArch
ClosedPublic

Authored by alexshap on Sep 18 2019, 2:35 AM.

Details

Summary

A particular slice of a universal binary can be either a MachO object or an archive
(although the class MachOUniversalBinary doesn't store them explicitly, instead, they can be constructed "on demand"
via calling the method ObjectForArch::getAsObjectFile or ObjectForArch::getAsArchive)
Before this patch MachOUniversalBinary had only a method (getObjectForArch)
to look up a slice for a particular architecture assuming that the slice
is a MachO object. We generalize it and make it return the corresponding instance of ObjectForArch instead;

Test plan: make check-all

Diff Detail

Event Timeline

alexshap created this revision.Sep 18 2019, 2:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2019, 2:35 AM
alexshap updated this revision to Diff 220639.Sep 18 2019, 2:46 AM
compnerd added inline comments.Sep 18 2019, 9:07 AM
include/llvm/Object/MachOUniversal.h
162 ↗(On Diff #220639)

ObjectForArch? That seems like an odd type. Why not use Binary if you are looking for a common base type for an object file and an archive?

mtrent added inline comments.Sep 18 2019, 11:22 AM
include/llvm/Object/MachOUniversal.h
162 ↗(On Diff #220639)

IIUC, the idea is to restrict the kind of things that one might reasonably find in a universal file. For example, constrain the output from getObjectForArch to be either a Mach-O file or an Archive. Binary would imply you would have to deal with the possibility that a universal file might contain another universal file, or a non-binary file such as a plist, ELF binaries, or literally anything.

Because people can make universal files themselves (it's a very simple file format), we find sometimes people abuse the format and shove weird stuff in there. Sometimes it's something "wrong-but-benign" like shoving an macOS-compiled simulator framework alongside an iOS-compiled framework. Sometimes it's something "wrong-just-wrong".

ObjectForArch is an existing class meant to put the burden of checking types on MachOUniversal.cpp. Returning Binary would put the burden of checking types onto each caller. I agree its a weird type name to expose to callers.

A reasonable alternative would be to add "getArchiveForArch" alongside "getObjectForArch" and then provide a third "isObject / isArchive" accessor so callers knew which method to call. That's probably the same order of patch as this change.

alexshap marked an inline comment as done.Sep 18 2019, 1:05 PM
alexshap added inline comments.
include/llvm/Object/MachOUniversal.h
162 ↗(On Diff #220639)

@compnerd
Just wanted to add (I also mentioned this in the description), that I was looking at
the implementation of MachOUniversalBinary::ObjectForArch::getAsObjectFile / MachOUniversalBinary::ObjectForArch::getAsArchive. The thing is that the both methods create
MachOObjectFile / Archive instances on "the fly" using ObjectFile::createMachOObjectFile / Archive::create respectively. The both methods can return Error for multiple reasons.
Unpacking Expected<MachOObjectFile> and Expected<Archive> and picking the "right" error didn't look good to me, that's why preferred the current approach.

compnerd accepted this revision.Sep 18 2019, 2:55 PM

Okay, both @mtrent and @alexshap bring up good points. I *suppose* that is okay, but I shall string grumble. @mtrent, I think we could also simply make getObjectForArch be internal to the type, and force users to go down either getMachOArchiveForArch or getMachOObjectForArch.

This revision is now accepted and ready to land.Sep 18 2019, 2:55 PM
This revision was automatically updated to reflect the committed changes.