Some fixes for the GetMainExecutable function

Hi,

The attached patch ensures that GetMainExecutable function in llvm-trunk/lib/System/Unix/Path.inc also works on Minix. I'm not sure it should be applied unmodified, because there may be controversial changes in it.

In particular, one of the things it changes is this:

- snprintf(buf, PATH_MAX, "%s//%s", dir, bin);
+ snprintf(buf, PATH_MAX, "%s/%s", dir, bin);

in the function test_dir(), because Minix doesn't like multiple slashes to as path separator. Since it is not immediately obvious to use double slashes here, I'm wondering if this was a deliberate choice. Is there a reason for these two slashes?

Also, the if-all-else-fails return value of GetMainExecutable is an empty path, but since the users of this function don't test for that, this choice leads to rather obscure errors. Wouldn't it be better to give an explicit error in such cases, or at the very least to return a path just containing Arg0 rather than the empty string?

It would be nice if these fixes could be part of llvm 2.8, since we're planning to use that as our base version for Minix, once it's released.

minix-patch.diff (1005 Bytes)

- snprintf(buf, PATH_MAX, "%s//%s", dir, bin);

There's no comment, and no mention in the commit message.
It seems pretty safe to fix.

Also, the if-all-else-fails return value of GetMainExecutable is an empty path, but since the users of this function don't test for that, this choice leads to rather obscure errors. Wouldn't it be better to give an explicit error in such cases,

Issuing an error is hard to do right here, since it's in such a low-level
library. However, it doesn't have very many users, and at least some of
them already do handle this error gracefully, so can any users which
don't be fixed?

or at the very least to return a path just containing Arg0 rather than the empty string?

If GetMainExecutable ever were to actually fail, it would mean that
something is seriously wrong with the system, so it's quite acceptable
to just fail, rather than cast about.

Dan

- snprintf(buf, PATH_MAX, "%s//%s", dir, bin);

There's no comment, and no mention in the commit message.
It seems pretty safe to fix.

Also, the if-all-else-fails return value of GetMainExecutable is an empty path, but since the users of this function don't test for that, this choice leads to rather obscure errors. Wouldn't it be better to give an explicit error in such cases,

Issuing an error is hard to do right here, since it's in such a low-level
library. However, it doesn't have very many users, and at least some of
them already do handle this error gracefully, so can any users which
don't be fixed?

It depends a bit on the nature of the error state. If it were my own program, and if this is never supposed to happen (which seems to be the case here) I would print an error message starting with "Internal error:", and probably do an exit(1).

or at the very least to return a path just containing Arg0 rather than the empty string?

If GetMainExecutable ever were to actually fail, it would mean that
something is seriously wrong with the system, so it's quite acceptable
to just fail, rather than cast about.

Ok, but then it would be entirely reasonable (I would even argue mandatory) to emit an error message in GetMainExecutable, and probably make the error fatal.

And to take it one step further: the current code silently just returns an empty path on platforms that do not match any of the #ifdefs in this function. I think people porting llvm to a new platform would very much appreciate a

#else
#error ...

so that they don't get bitten by this.

But perhaps I'm misunderstanding the 'contract' of this function, and is it allowed to fail after all. (In which case the clang driver should test the result of this function.)

Also, the if-all-else-fails return value of GetMainExecutable is an empty path, but since the users of this function don't test for that, this choice leads to rather obscure errors. Wouldn't it be better to give an explicit error in such cases,

Issuing an error is hard to do right here, since it's in such a low-level
library. However, it doesn't have very many users, and at least some of
them already do handle this error gracefully, so can any users which
don't be fixed?

It depends a bit on the nature of the error state. If it were my own program, and if this is never supposed to happen (which seems
to be the case here) I would print an error message starting with "Internal error:", and probably do an exit(1).

Maybe a way to solve this would be to split the code so that the
main API entrypoint lives in lib/Support instead of lib/System.
That way it could call llvm::report_fatal_error. I'll look into
this.

or at the very least to return a path just containing Arg0 rather than the empty string?

If GetMainExecutable ever were to actually fail, it would mean that
something is seriously wrong with the system, so it's quite acceptable
to just fail, rather than cast about.

Ok, but then it would be entirely reasonable (I would even argue mandatory) to emit an error message in GetMainExecutable, and
probably make the error fatal.

And to take it one step further: the current code silently just returns an empty path on platforms that do not match any of the
#ifdefs in this function. I think people porting llvm to a new platform would very much appreciate a

#else
#error ...

so that they don't get bitten by this.

But perhaps I'm misunderstanding the 'contract' of this function, and is it allowed to fail after all. (In which case the clang
driver should test the result of this function.)

Sounds good to me. I've now added this.

Dan