llvm dev - Sep 2010 - [LLVMdev] 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.

-- 
Dr. ir. Kees van Reeuwijk, Vrije Universiteit Amsterdam



-------------- next part --------------
A non-text attachment was scrubbed...
Name: minix-patch.diff
Type: application/octet-stream
Size: 1005 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20100902/9902780e/attachment.obj>
-------------- next part --------------

On Sep 2, 2010, at 10:03 AM, Kees van Reeuwijk wrote:
> -  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

On 09/02/2010 07:51 PM, Dan Gohman wrote:
>
> On Sep 2, 2010, at 10:03 AM, Kees van Reeuwijk wrote:
>
>> -  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.)

-- 
Dr. Ir. Kees van Reeuwijk, Vrije Universiteit Amsterdam