Page MenuHomePhabricator

Fix some posixy assumptions that were used during RunShellCommand
ClosedPublic

Authored by zturner on Dec 5 2014, 2:36 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 16999.Dec 5 2014, 2:36 PM
zturner updated this revision to Diff 17001.
zturner retitled this revision from to Fix some posixy assumptions that were used during RunShellCommand.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added a reviewer: clayborg.
zturner added a subscriber: Unknown Object (MLST).

Remove ClangExpressionParser changes from this patch, that was for an unrelated fix.

clayborg requested changes to this revision.Dec 5 2014, 2:47 PM
clayborg edited edge metadata.

The only thing I don't know about is the:

launch_info.SetArchitecture(HostInfo::GetArchitecture());

Line... You might specify a tool on MacOSX that is i386 like "/path/to/i386/a.out" and if you set this to the system architecture x86_64-apple-macosx, it won't execute the shell tool. This should probably be removed. Did you need to do this for windows or did you just add be because you thought it would always be correct?

This revision now requires changes to proceed.Dec 5 2014, 2:47 PM

If you take a look at ConvertArgumentsForLaunchingInShell, I had to make a
decision about whether to use -c or /C as an argument to the shell. I
could have string checked the shell name against "cmd.exe", but that seemed
more hackish than asking "Is this going to run in a shell on windows"?

Also, there's some code in ConvertArgumentsForLaunchingInShell that checks
the architecture. If you get into that function through
Host::RunShellCommand, the architecture would have been uninitialized, so
it looked like in that case the code that was checking for it would have
been buggy.

The architecture check is in case you have a universal file that contains both x86_64 and i386, you might set the architecture to specify you want to run (debug more like it) the i386 slice even though you are on a 64 bit system (MacOSX supports this). For running a shell command, just let the system do what it needs to do and launch it using the best slice it can and don't set the architecture in the launch info.

So please remove "launch_info.SetArchitecture(HostInfo::GetArchitecture());" from Host::RunShellCommand().

In ConvertArgumentsForLaunchingInShell(), do you suggest I change the
conditional to use /C if the shell name is "cmd.exe"? Seems a little
hackish, so if there's a better way then that would be preferable.

What about setting the Architecture as I'm currently doing, but modify the
triple so it only contains OS and Environment, but not architecture?

Seems like we should either get this from a call like the one that gets "cmd.exe" (HostInfoWindows::GetDefaultShell()). In Host::RunShellCommand() we already do:

launch_info.SetShell(HostInfo::GetDefaultShell());

We should be asking the HostInfo class to be setting the shell arguments for us. Then we HostInfoPosix and HostInfoWindows could "do the right thing".

So I vote to remove ProcessLaunchInfo::ConvertArgumentsForLaunchingInShell(), then create a function in the HostInfo class that can do this correctly for each system. This function is only used in Platform::LaunchProcess() when "IsHost()" is true and in Host::RunShellCommand().

Actually wait. Currently the architecture is completely uninitialized, so
anything that would break as a result of setting it in
Host::RunShellCommand would already break, because it would be making an
assumption that it could use an uninitialized ArchSpec. So this actually
seems a little better than before.

A more robust fix might involve breaking this function up into smaller
pieces.

In D6553#13, @clayborg wrote:

Seems like we should either get this from a call like the one that gets "cmd.exe" (HostInfoWindows::GetDefaultShell()). In Host::RunShellCommand() we already do:

launch_info.SetShell(HostInfo::GetDefaultShell());

We should be asking the HostInfo class to be setting the shell arguments for us. Then we HostInfoPosix and HostInfoWindows could "do the right thing".

So I vote to remove ProcessLaunchInfo::ConvertArgumentsForLaunchingInShell(), then create a function in the HostInfo class that can do this correctly for each system. This function is only used in Platform::LaunchProcess() when "IsHost()" is true and in Host::RunShellCommand().

Looks like we crossed paths, but we basically said the same thing, that the function should be reworked. Can I commit the fix as-is (since it doesn't break anything, due to the existing Architecture being uninitialized), and then follow up with the re-work into HostInfo?

The reason for comitting as-is first is because this patch contains some other stuff, like the changes in FileSpec, and the temp file paths, that would be unrelated to a refactor of this function.

Fine to commit as long as you don't set the architecture in the launch_info in Host::RunShellCommand() since that can break MacOSX.

In D6553#16, @clayborg wrote:

Fine to commit as long as you don't set the architecture in the launch_info in Host::RunShellCommand() since that can break MacOSX.

Ok, I will take out the checks I added against the triple in ConvertArgumentsForLaunchingInShell for now and commit, then re-add them once the logic is properly separated in HostInfo.

Just so I can understand the code better, can you point me to the code that would break if the Architecture were set? I see some in ConvertArgumentsForLaunchingInShell, but it's only exercised if will_debug == true, and RunShellCommand() uses will_debug == false. So I'm assuming its something deep inside of Host::LaunchProcess.

zturner closed this revision.Dec 5 2014, 4:15 PM
zturner updated this revision to Diff 17006.

Closed by commit rL223548 (authored by @zturner).

You are correct, since it is protected by "will_debug" it won't break MacOSX. Sorry about the noise.

Greg

This breaks the entire test suite (Jim Ingham informed me), "run" which is aliased to "process launch --shell" no longer works. Please revert this.

jingham added a subscriber: jingham.Dec 5 2014, 5:26 PM

Yes, as checked in it didn't even build (fixed in r223559.) But then "run" fails with:

/bin/sh: exec /usr/bin/arch -arch x86_64 /Volumes/ThePlayground/Users/jingham/Projects/Sketch/build/Debug/Sketch.app/Contents/MacOS/Sketch: No such file or directory
error: process exited with status 127

But "process launch" with the same binary works correctly.

Jim

Can you revert for me? sorry about the break. I will have to fix it Monday.

If its not urgent i can revert in a couple hours, but I don't have access
to a machine at the moment.

I did it (r223568.)

Jim

Thanks, before resubmitting I'll run the test suite on mac and linux.

I resubmitted this in r223695. My original patch was actually ok, the
issue happened when I removed the code for setting the Architecture in the
ProcessLaunchInfo, I inadvertently removed the code that added -c to the
command line of /bin/sh. Since toward the end of the thread we determined
that setting the architecture was actually ok (since it's checked inside of
a will_debug block, I added this code back in.

I built and ran on both Linux and Mac, and did not see any failures. On
the off chance it does fail again though, let me know.