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

[Mikhail Glushenkov]

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;
  }

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.

> >
> >> 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.

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

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

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

--
()  ascii ribbon campaign - against html e-mail
/\  www.asciiribbon.org   - against proprietary attachments
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Path-Add-GetEXESuffix-to-complement-GetDLLSuffix.patch
Type: text/x-diff
Size: 1987 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101102/b95177cc/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-80-col-violations.patch
Type: text/x-diff
Size: 1401 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101102/b95177cc/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Make-FindProgramByName-return-absolute-paths-unmodif.patch
Type: text/x-diff
Size: 2685 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101102/b95177cc/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Make-FindExecutable-always-return-an-executable-path.patch
Type: text/x-diff
Size: 3100 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20101102/b95177cc/attachment-0003.patch>