[cfe-commits] r90577 - in /cfe/trunk: include/clang/Driver/Options.td lib/Driver/Tools.cpp tools/driver/driver.cpp

[Rafael Espíndola]

> I'm not really happy about this patch (adding argument parsing logic
> in tools/driver in particular).

I am not happy with where it is parsed too. I just couldn't find a
better place without a lot additional refactoring.

> Is it also really true that this is
> all this option does for gcc?

Gcc does path canonization in a central location, but in practical
term it looks like the only difference is where gcc looks for cc1.
Note that gcc's cc1 never tries  to find its own canonical path. In
clang I will probably have to pass -no-canonical-prefixes to clang-cc
(or clang -cc1) too.

> I assume this is fixing something in a google symlink madness
> environment, but I think there is a better solution.

It makes clang work on what Simon calls a "symlink farm". Consider

farm/bin/clang       -> /foo/clang
farm/bin/clang-cc   ->  /bar/clang-cc

> The main thing we
> are using this for is to find our "resources" (lib files and includes)
> -- what if we searched the following locations in order:
>  a. relative to argv[0]
>  b. relative to argv[0], following top-level links (NOT realpath)
>  c. relative to GetMainExecutable()
>
> I suspect this would solve your problem, and would also find the
> library directory much faster (than dladdr + realpath and friends) in
> the common case.

I would not match gcc's behavior. I am not sure if it might really be
a problem, but consider case where a resource can be found with "b"
but not with "a". I would like clang and clang-cc to produce an error.
Also, it would be slower when a resource cannot be found.

> I have a few other nits re the patch, inline, for posterity.
>
>> Modified:
>>    cfe/trunk/include/clang/Driver/Options.td
>>    cfe/trunk/lib/Driver/Tools.cpp
>>    cfe/trunk/tools/driver/driver.cpp
>>
>> Modified: cfe/trunk/include/clang/Driver/Options.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=90577&r1=90576&r2=90577&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/include/clang/Driver/Options.td (original)
>> +++ cfe/trunk/include/clang/Driver/Options.td Fri Dec  4 13:31:58 2009
>> @@ -375,6 +375,8 @@
>>  def mwarn_nonportable_cfstrings : Flag<"-mwarn-nonportable-cfstrings">, Group<m_Group>;
>>  def m_Separate : Separate<"-m">, Group<m_Group>;
>>  def m_Joined : Joined<"-m">, Group<m_Group>;
>> +def no_canonical_prefixes : Flag<"-no-canonical-prefixes">, Flags<[DriverOption]>,
>> +  HelpText<"Do not resolve symbolic links, turn relative paths into absolute ones, or do anything else to identify the executable">;
>
> This should probably be under help hidden, or perhaps just in the man
> page, its certainly not something to make the "short" help list.

How about "Use relative instead of canonical paths"?

>>  def no_cpp_precomp : Flag<"-no-cpp-precomp">;
>>  def no_integrated_cpp : Flag<"-no-integrated-cpp">, Flags<[DriverOption]>;
>>  def no__dead__strip__inits__and__terms : Flag<"-no_dead_strip_inits_and_terms">;
>>
>> Modified: cfe/trunk/lib/Driver/Tools.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=90577&r1=90576&r2=90577&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Driver/Tools.cpp (original)
>> +++ cfe/trunk/lib/Driver/Tools.cpp Fri Dec  4 13:31:58 2009
>> @@ -1104,6 +1104,9 @@
>>   // care to warn the user about.
>>   Args.ClaimAllArgs(options::OPT_clang_ignored_f_Group);
>>   Args.ClaimAllArgs(options::OPT_clang_ignored_m_Group);
>> +
>> +  // -no-canonical-prefixes is used very early in main.
>> +  Args.ClaimAllArgs(options::OPT_no_canonical_prefixes);
>
> This isn't the right place to claim, the option was handled in the
> main() so it should be claimed in the driver. But the only reason this
> is needed at all is because the argument parsing was done by hand.

I can try to change where we claim it, but I think I have to parse
this one by hand (as -cc1 is) since it has to be parsed before the
option parsing infrastructure is up and running.

Cheers,
Rafael