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

[Daniel Dunbar]

2009/12/5 Rafael Espíndola <rafael.espindola at gmail.com>:
>> 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.

I view this as a symptom of it being a "bad idea". I really dislike
providing command line options for this kind of thing -- its better to
find a model where the compiler "just works".

>> 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 intend to have clang pass the "resources path" to clang-cc/clang
-cc1 fairly soon, we shouldn't need this option at the clang-cc level.

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

Yes, I understand the use case.

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

Is that actually a problem? I'm not convinced these parts of gcc are
well designed.

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

Is this important (the error)? Having a single consistent model is
much friendlier to users (in the long run) than having multiple models
and requiring an option. I think it is very unlikely that this lookup
will (a) succeed and (b) find the wrong compiler; do you have a use
case where that would happen?

> Also, it would be slower when a resource cannot be found.

This is not a use case that matters, though.

>> 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"?

My specific comment was just that the option shouldn't be in --help.
There is a new flag for making it hidden by default.

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