[llvm-commits] [llvm] r117596 - /llvm/trunk/lib/System/Unix/Program.inc

[Dan Gohman]

On Nov 1, 2010, at 6:22 PM, Mikhail Glushenkov wrote:

> Hi,
> 
> On Fri, Oct 29, 2010 at 11:12:01AM -0700, Dan Gohman wrote:
>> 
>>>>> Alternatively, we can change FindExecutable to take into account that
>>>>> canExecute can be false for the path returned by FindProgramByName.
>>>> 
>>>> 
>>>> My assumption is that it's perfectly acceptable for FindExecutable
>>>> to return a non-executable path here; its caller will attempt to
>>>> execute the path, get an error, and report the error to the user,
>>> 
>>> I think it's not. Currently, FindExecutable always succeeds (on unix-likes),
>>> even if given a non-absolute path. FindExecutable("something") =
>>> Path("$EXE_DIR").appendComponent("something"), which was not the original
>>> intent.
>> 
>> The code to call FindProgramByName there was only added as a
>> hack for Windows to add a ".exe" suffix.  FindExecutable ideally should
>> always succeed, because it is primarily just a path manipulation.
> 
> Originally, FindExecutable looked like this:
> 
>  sys::Path Result = sys::Path::GetMainExecutable(Argv0, MainAddr);
>  Result.eraseComponent();
>  if (!Result.isEmpty()) {
>    Result.appendComponent(ExeName);
>    if (Result.canExecute())
>      return Result;
>  }

This was an obsolete artifact. Going back in time further, this code was
followed by a PATH search, and in those days the canExecute() check had a
purpose. In r78240, I removed the PATH search, and it appears I failed to
remove the canExecute() check, even though it was no longer needed.

> 
> Then the call to FindProgramByName was added to append the '.exe' suffix:
> 
>    if (Result.isAbsolute()) {
>      Result = sys::Program::FindProgramByName(Result.str());
>      return Result;
>    }
> 
> On Windows, the behaviour is still the same (because FindProgramByName always
> checks for executability), but on unix-likes FindExecutable now always succeeds
> (because of the special treatment of absolute paths). IMO, this is a bug.

The current usage of FindProgramByName in FindExecutable is admittedly
awkward.  It solved a problem for someone working on Windows, and the
isAbsolute thing was a way to stay pretty close to the desired behavior
on Unix.

> 
>>> 
>>>> What problem are you trying to solve here?
>>> 
>>> I find the current behaviour confusing and I want it to be consistent across
>>> platforms. If we decide to standardise on the Unix behaviour, then we should
>>> also make the Win32 version of FindProgramByName return absolute paths
>>> untouched, and mention this issue in the comments.
>> 
>> 
>> Currently my understanding is that it modifies absolute paths by appending a
>> ".exe" suffix.  If there's another way to achieve this, that would probably be
>> better.
> 
> I propose the following changes (patches attached):
> 
>  1. Make FindProgramByName() return absolute paths untouched on all platforms
>  and mention this in the comments.

This is fine by me, though I don't know what FindProgramByName's callers
expectations are with respect to ".exe" suffixes on Windows.

Also, note that on Unix it's not just absolute paths, it's any path
containing slashes. I don't know how that translates on Windows.

>  2. Add a GetEXESuffix() function to the Path class (there is already
>  GetDLLSuffix()).

Ok.

>  3. Make FindExecutable() use GetEXESuffix() instead of FindProgramByName().

Ok.

>  4. Make FindExecutable() always return an executable path, as it used to.


FindExecutable shouldn't do this. If LLVM tools are installed with .exe
suffixes on Windows, FindExecutable should unconditionally append the
GetEXESuffix string and return the resulting path.

FindExecutable is used when one LLVM tool wants to invoke another LLVM
tool. These tools are installed together, so they shouldn't have to do
any checking to figure out how to locate each other. Also, it's rare
for these tools to be missing, much less present but non-executable.
Client code is expected to check for errors after an execute anyway, so
doing extra checking up front is redundant and makes the code harder
to follow.

Dan