[LLVMdev] Some fixes for the GetMainExecutable function

[Dan Gohman]

On Sep 6, 2010, at 3:02 AM, Kees van Reeuwijk wrote:

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