This is an archive of the discontinued LLVM Phabricator instance.

Remove Platform references from the Host module
ClosedPublic

Authored by labath on Jan 10 2018, 4:56 AM.

Details

Summary

These were used by Host::LaunchProcess to "resolve" the executable it
was about to launch. The only parts of Platform::ResolveExecutable, which
seem to be relevant here are the FileSpec::ResolvePath and
ResolveExecutableLocation calls.

The rest (most) of that function deals with selecting an architecture
out of a fat binary and making sure we are able to create a Module with that
slice. These are reasonable actions when selecting a binary to debug,
but not for a generic process launching framework (it's technically even
wrong because we should be able to launch a binary with execute
permissions only, but trying to parse such file will obviously fail).

I remove the platform call by inlining the relevant FileSpec calls and
ignoring the rest of the Platform::ResolveExecutable code. The
architecture found by the slice-searching code is being ignored already
anyway, as we use the one specified in the LaunchInfo, so the only
effect of this should be a different error message in case the
executable does not contain the requested architecture -- before we
would get an error message from the Platform class, but now we will get
an error from the actual posix_spawn syscall (this is only relevant on
mac, as it's the only target supporting fat binaries).

Launching targets for debugging should not be affected as here the
executable is pre-resolved at the point when the Target is created.

Event Timeline

labath created this revision.Jan 10 2018, 4:56 AM
davide accepted this revision.Jan 10 2018, 7:31 AM
davide added a subscriber: davide.

This LGTM but please wait for a second opinion.

This revision is now accepted and ready to land.Jan 10 2018, 7:31 AM

As long as:

% lldb /path/to/Foo.app
(lldb) r

Still works, then I am fine with this. The resolve executable should find the executable down inside the app bundle (like "/path/to/Foo.app/Contents/MacOS/Foo" for desktop apps and "/path/to/Foo.app/Foo" for iOS apps).

As long as:

% lldb /path/to/Foo.app
(lldb) r

Still works, then I am fine with this. The resolve executable should find the executable down inside the app bundle (like "/path/to/Foo.app/Contents/MacOS/Foo" for desktop apps and "/path/to/Foo.app/Foo" for iOS apps).

This sounds like something that would be pretty easy to write a test for with lldb-test.

As long as:

% lldb /path/to/Foo.app
(lldb) r

Still works, then I am fine with this. The resolve executable should find the executable down inside the app bundle (like "/path/to/Foo.app/Contents/MacOS/Foo" for desktop apps and "/path/to/Foo.app/Foo" for iOS apps).

This sounds like something that would be pretty easy to write a test for with lldb-test.

Yes, please add a test.

labath updated this revision to Diff 129435.Jan 11 2018, 5:16 AM

Add back bundle resolution to the mac launcher

This revision was automatically updated to reflect the committed changes.