[driver,LTO] : Resurrect user specs support

Hello

On 05/22/2012 03:52 PM, Joseph S. Myers wrote:
> On Mon, 21 May 2012, Christian Bruel wrote:
> 
>> 1) Lazily check the flag validation until all command line spec files
>> are read. For this purpose, 'read_specs' records specs, to be analyzed
>> with 'file_spec_p'. Such flags have 'live_cond' = SWITCH_USER
> 
> I like the idea of allowing flags mentioned in user specs but not other 
> specs - but not the implementation using this new live_cond.  

OK, I have removed the SWITCH_USER flag and replaced it by a bool in the
struct spec_list. This field is now passed as parameter to
validate_switches. Maybe less central, but more flexible as you proposed.

> There are a 
> lot of places in gcc.c that set "validated", and I can't convince myself 
> that this implementation will ensure they all take correct account of 
> where the relevant spec (if any) came from without causing any other 
> change to how the driver behaves.  For example, I don't see any change to 
> the setting of "validated" for %< and %> in do_spec_1 to account for where 
> the spec came from.
> 
> Instead, I think that any function that sets "validated" based on a spec 
> should be passed the information about whether it's a user spec or not.  
> So validate_all_switches would need to pass that down to 
> validate_switches_from_spec, for example - and do_spec_1 would also need 
> to get that information.

I shared the same concern, however, after playing bits with spec toys, I
couldn't a find a way to get a %< switch recognition failure, since the
switches passed on the command line at this point are already validated
if necessary.

I put a simple example of this in attachment to illustrate this. But I
might lack imagination to make up a regression.

We indeed now have all the information to pass down to the do_specs
interfaces, but this would be very intrusive, I'm reluctant to do it if
not strictly necessary. Do you see a way an invalid option could be
accidentally validated ? I would have thought that with the current
implementation invalid flags are detected earlier.

Cheers

Christian

On Mon, 28 May 2012, Christian Bruel wrote:

> I shared the same concern, however, after playing bits with spec toys, I
> couldn't a find a way to get a %< switch recognition failure, since the
> switches passed on the command line at this point are already validated
> if necessary.

Suppose with the existing sources an option (in a .opt file) is matched by 
a $< spec, and not by any other spec.  Will it be rejected by the driver?  
It shouldn't be.  Are you saying there is some pre-existing bug here, or 
that %< validation happens in more than one place so some setting of 
"validated" is redundant but the code still works correctly?

On 05/28/2012 01:11 PM, Joseph S. Myers wrote:
> On Mon, 28 May 2012, Christian Bruel wrote:
> 
>> I shared the same concern, however, after playing bits with spec toys, I
>> couldn't a find a way to get a %< switch recognition failure, since the
>> switches passed on the command line at this point are already validated
>> if necessary.
> 
> Suppose with the existing sources an option (in a .opt file) is matched by 
> a $< spec, and not by any other spec.  Will it be rejected by the driver?  
> It shouldn't be. 

indeed, it's not rejected if it is present in a .opt file. I was
concerned that it will not be rejected even if not in any .opt (or now
in --specs). Which was what the validated setting seemed to imply.

Should it be rejected ? probably. But this is not implied by the --spec
changes.

 Are you saying there is some pre-existing bug here, or
> that %< validation happens in more than one place so some setting of 
> "validated" is redundant but the code still works correctly?
> 

I think the check

 if (! switches[i].validated)
      error

is already done when we process the do_spec for user specs.

It seems that there is no need to check for user option and set
'validated' in the cases '>:,'<', in do_spec_1 because if the switch
was not valid (not present in any .opt and not present in a user --spec)
it would already have been rejected.


Thanks

Christian

On Mon, 28 May 2012, Christian Bruel wrote:

> 
> 
> On 05/28/2012 01:11 PM, Joseph S. Myers wrote:
> > On Mon, 28 May 2012, Christian Bruel wrote:
> > 
> >> I shared the same concern, however, after playing bits with spec toys, I
> >> couldn't a find a way to get a %< switch recognition failure, since the
> >> switches passed on the command line at this point are already validated
> >> if necessary.
> > 
> > Suppose with the existing sources an option (in a .opt file) is matched by 
> > a $< spec, and not by any other spec.  Will it be rejected by the driver?  
> > It shouldn't be. 
> 
> indeed, it's not rejected if it is present in a .opt file. I was
> concerned that it will not be rejected even if not in any .opt (or now
> in --specs). Which was what the validated setting seemed to imply.
> 
> Should it be rejected ? probably. But this is not implied by the --spec
> changes.

The existing rule is supposed to be: options are only accepted if in 
*both* a .opt file *and* a spec.  If not in a .opt file, the common 
machinery will reject them; if in a .opt file but not a spec, the driver's 
own validation machinery will reject them.

If the driver's own validation machinery isn't rejecting them, that 
indicates that some spec has handled them.  It's possible there's more 
than one piece of code relating to accepting such options and some such 
code is redundant.

(This can't be tested with options starting -f or -m because of the specs 
passing all such options to cc1.)

The new semantics are supposed to be, I think: an option in a .opt file is 
accepted if any spec matches it (same as now), an option not in a .opt 
file is only accepted if a user spec matches it and not simply because of 
a match from a built-in spec (where built-in specs are considered to 
include those generated by some of GCC's own runtime libraries).

On 05/28/2012 06:27 PM, Joseph S. Myers wrote:
> On Mon, 28 May 2012, Christian Bruel wrote:
> 
>>
>>
>> On 05/28/2012 01:11 PM, Joseph S. Myers wrote:
>>> On Mon, 28 May 2012, Christian Bruel wrote:
>>>
>>>> I shared the same concern, however, after playing bits with spec toys, I
>>>> couldn't a find a way to get a %< switch recognition failure, since the
>>>> switches passed on the command line at this point are already validated
>>>> if necessary.
>>>
>>> Suppose with the existing sources an option (in a .opt file) is matched by 
>>> a $< spec, and not by any other spec.  Will it be rejected by the driver?  
>>> It shouldn't be. 
>>
>> indeed, it's not rejected if it is present in a .opt file. I was
>> concerned that it will not be rejected even if not in any .opt (or now
>> in --specs). Which was what the validated setting seemed to imply.
>>
>> Should it be rejected ? probably. But this is not implied by the --spec
>> changes.
> 
> The existing rule is supposed to be: options are only accepted if in 
> *both* a .opt file *and* a spec.  If not in a .opt file, the common 
> machinery will reject them; if in a .opt file but not a spec, the driver's 
> own validation machinery will reject them.

Thanks for confirming this, the question was more specifically for
<%options. Today, with the current implementation, I see two uses cases:

1) The flag appears in a %< spec but is not in a .opt file
  -> It is *not* rejected. It is just ignored.
2) The flag appears in a user switch and in a %< spec, and not in a .opt
file.
  -> It is rejected.

To refocus on the original question from the patch. I'm still not
convinced after our discussions and testings that the propagation of the
user flag to the do_spec functions is required to keep the same semantic.

If there is an issue with the current %< handling, could we handle this
separately ? my primary focus was in matching --spec user options
behavior with the .opt internal ones.

> 
> If the driver's own validation machinery isn't rejecting them, that 
> indicates that some spec has handled them.  It's possible there's more 
> than one piece of code relating to accepting such options and some such 
> code is redundant.
> 
> (This can't be tested with options starting -f or -m because of the specs 
> passing all such options to cc1.)
> 
> The new semantics are supposed to be, I think: an option in a .opt file is 
> accepted if any spec matches it (same as now), an option not in a .opt 
> file is only accepted if a user spec matches it and not simply because of 
> a match from a built-in spec (where built-in specs are considered to 
> include those generated by some of GCC's own runtime libraries).

I agree. I believe my patch implements this, my focus was on not
changing the current behavior for switches internally defined in a .opt
(or now in a --spec file). error are still generated for other cases.

Many thanks

Christian

On Tue, 29 May 2012, Christian Bruel wrote:

> > The existing rule is supposed to be: options are only accepted if in 
> > *both* a .opt file *and* a spec.  If not in a .opt file, the common 
> > machinery will reject them; if in a .opt file but not a spec, the driver's 
> > own validation machinery will reject them.
> 
> Thanks for confirming this, the question was more specifically for
> <%options. Today, with the current implementation, I see two uses cases:
> 
> 1) The flag appears in a %< spec but is not in a .opt file
>   -> It is *not* rejected. It is just ignored.

I don't really see that as a use case; it's more a matter of an internal 
consistency check that could be done but isn't.  I'm only concerned about 
cases where the option is actually passed on the command line to the 
driver.

> 2) The flag appears in a user switch and in a %< spec, and not in a .opt
> file.
>   -> It is rejected.

There are also cases:

* The option, passed by the user, is in a .opt file, a %< spec but no 
other spec.  (Should be accepted.)

* The option, passed by the user, is in a .opt file and no spec at all.  
(Should be rejected.)

> To refocus on the original question from the patch. I'm still not
> convinced after our discussions and testings that the propagation of the
> user flag to the do_spec functions is required to keep the same semantic.

With the proposed change to the rules for when a spec serves to validate 
an option, all settings of the "validated" field need to be reviewed to 
make sure that they are in accordance with the new rules - and that it is 
transparent to human readers of the code that they are in accordance with 
the new rules.  If you don't think propagation is needed to do_spec 
functions, then it should be possible to remove the "validated" setting in 
there, with a proper explanation of the order in which the various 
relevant functions are called and where "validated" will previously or 
subsequently be set.

On 05/29/2012 12:50 PM, Joseph S. Myers wrote:
> On Tue, 29 May 2012, Christian Bruel wrote:
> 
>>> The existing rule is supposed to be: options are only accepted if in 
>>> *both* a .opt file *and* a spec.  If not in a .opt file, the common 
>>> machinery will reject them; if in a .opt file but not a spec, the driver's 
>>> own validation machinery will reject them.
>>
>> Thanks for confirming this, the question was more specifically for
>> <%options. Today, with the current implementation, I see two uses cases:
>>
>> 1) The flag appears in a %< spec but is not in a .opt file
>>   -> It is *not* rejected. It is just ignored.
> 
> I don't really see that as a use case; it's more a matter of an internal 
> consistency check that could be done but isn't.  I'm only concerned about 
> cases where the option is actually passed on the command line to the 
> driver.
> 
>> 2) The flag appears in a user switch and in a %< spec, and not in a .opt
>> file.
>>   -> It is rejected.
> 
> There are also cases:
> 
> * The option, passed by the user, is in a .opt file, a %< spec but no 
> other spec.  (Should be accepted.)

yes, it is.

> 
> * The option, passed by the user, is in a .opt file and no spec at all.  
> (Should be rejected.)

yes, it is.

> 
>> To refocus on the original question from the patch. I'm still not
>> convinced after our discussions and testings that the propagation of the
>> user flag to the do_spec functions is required to keep the same semantic.
> 
> With the proposed change to the rules for when a spec serves to validate 
> an option, all settings of the "validated" field need to be reviewed to 
> make sure that they are in accordance with the new rules - and that it is 
> transparent to human readers of the code that they are in accordance with 
> the new rules. If you don't think propagation is needed to do_spec
> functions, then it should be possible to remove the "validated" setting in 
> there, with a proper explanation of the order in which the various 
> relevant functions are called and where "validated" will previously or 
> subsequently be set.
> 

I agree. I see two potential settings of the validated field. The %<
that we just reviewed, and the case : /* We have Xno-YYY, search for
XYYY.  */

It seems possible to remove those, I've just checked this during lunch
:-) with the scenarios described above (without use m* f* specs).

If it is a prerequisite before my --spec patch, I'd like to propose it
as an independent one, since this is a valid modification regardless of
the --spec implementation (although it makes it clearer).

Many thanks

Christian

On Tue, 29 May 2012, Christian Bruel wrote:

> I agree. I see two potential settings of the validated field. The %<
> that we just reviewed, and the case : /* We have Xno-YYY, search for
> XYYY.  */
> 
> It seems possible to remove those, I've just checked this during lunch
> :-) with the scenarios described above (without use m* f* specs).
> 
> If it is a prerequisite before my --spec patch, I'd like to propose it
> as an independent one, since this is a valid modification regardless of
> the --spec implementation (although it makes it clearer).

Please do send such a patch (with an explanation in each case of how 
"validated" still gets set with that redundant setting removed).