Re: [driver, LTO Patch]: Resurrect user specs support
On 05/29/2012 09:05 PM, Joseph S. Myers wrote: > On Tue, 29 May 2012, Christian Bruel wrote: > >> So I tested the following semantics, with the ones that you pointed. > > Thanks for the testing. The patch is OK with the ChangeLog conflict > markers removed (and the dates on log entries updated to the date of > commit). > Thanks, committed on trunk.
Re: [driver, LTO Patch]: Resurrect user specs support
On Tue, 29 May 2012, Christian Bruel wrote: > So I tested the following semantics, with the ones that you pointed. Thanks for the testing. The patch is OK with the ChangeLog conflict markers removed (and the dates on log entries updated to the date of commit). -- Joseph S. Myers jos...@codesourcery.com
Re: [driver, LTO Patch]: Resurrect user specs support
> Please do send such a patch (with an explanation in each case of how
> "validated" still gets set with that redundant setting removed).
>
'validated' can only be removed for user --specs switches, this is why I
think it doesn't need to be propagated to do_specs. My mistake from the
previous mail: It can't be split out of the whole patch.
For the explanation, We should see a difference in the way define a flag
as a .opt internal, and a option in a spec file.
%{S: as a substitute doesn't really define a new flag. But we want to
allow them.
This why it works. a --spec file option is validated as soon as a user
switch matches a substitute spec string. It can't possibly be an invalid
option and a user substitute rule in the same time, so there is no need
to check in do_specs. validate_all_switches is enough.
Of course for .opt flags it is different, and the current handling is
unchanged. But I need the .known guard, because we must make sure that
we are not caught by any * matching.
If the switch is defined by a substitute option in a user spec rule,
then the switch is validated when we see it. Since there is necessarily
a spec rule, it is necessarily used in a spec. So all other checks in
do_specs are not needed. The only risk is that we might mark a switch as
SWITCH_FALSE even if it's not valid. Which would be the case for
instance if we use a %Index: gcc/gcc.c
===
--- gcc/gcc.c (revision 187927)
+++ gcc/gcc.c (working copy)
@@ -190,8 +190,8 @@
static void store_arg (const char *, int, int);
static void insert_wrapper (const char *);
static char *load_specs (const char *);
-static void read_specs (const char *, int);
-static void set_spec (const char *, const char *);
+static void read_specs (const char *, bool, bool);
+static void set_spec (const char *, const char *, bool);
static struct compiler *lookup_compiler (const char *, size_t, const char *);
static char *build_search_list (const struct path_prefix *, const char *,
bool, bool);
@@ -227,9 +227,9 @@
static void do_self_spec (const char *);
static const char *find_file (const char *);
static int is_directory (const char *, bool);
-static const char *validate_switches (const char *);
+static const char *validate_switches (const char *, bool);
static void validate_all_switches (void);
-static inline void validate_switches_from_spec (const char *);
+static inline void validate_switches_from_spec (const char *, bool);
static void give_switch (int, int);
static int used_arg (const char *, int);
static int default_arg (const char *, int);
@@ -1171,11 +1171,12 @@
const char **ptr_spec; /* pointer to the spec itself. */
struct spec_list *next; /* Next spec in linked list. */
int name_len; /* length of the name */
- int alloc_p; /* whether string was allocated */
+ bool user_p; /* whether string come from file spec. */
+ bool alloc_p; /* whether string was allocated */
};
#define INIT_STATIC_SPEC(NAME,PTR) \
-{ NAME, NULL, PTR, (struct spec_list *) 0, sizeof (NAME) - 1, 0 }
+ { NAME, NULL, PTR, (struct spec_list *) 0, sizeof (NAME) - 1, false, false }
/* List of statically defined specs. */
static struct spec_list static_specs[] =
@@ -1479,7 +1480,7 @@
current spec. */
static void
-set_spec (const char *name, const char *spec)
+set_spec (const char *name, const char *spec, bool user_p)
{
struct spec_list *sl;
const char *old_spec;
@@ -1531,7 +1532,8 @@
if (old_spec && sl->alloc_p)
free (CONST_CAST(char *, old_spec));
- sl->alloc_p = 1;
+ sl->user_p = user_p;
+ sl->alloc_p = true;
}
/* Accumulate a command (program name and args), and run it. */
@@ -1687,7 +1689,7 @@
Anything invalid in the file is a fatal error. */
static void
-read_specs (const char *filename, int main_p)
+read_specs (const char *filename, bool main_p, bool user_p)
{
char *buffer;
char *p;
@@ -1736,7 +1738,7 @@
p[-2] = '\0';
new_filename = find_a_file (&startfile_prefixes, p1, R_OK, true);
- read_specs (new_filename ? new_filename : p1, FALSE);
+ read_specs (new_filename ? new_filename : p1, false, user_p);
continue;
}
else if (!strncmp (p1, "%include_noerr", sizeof "%include_noerr" - 1)
@@ -1757,7 +1759,7 @@
p[-2] = '\0';
new_filename = find_a_file (&startfile_prefixes, p1, R_OK, true);
if (new_filename)
- read_specs (new_filename, FALSE);
+ read_specs (new_filename, false, user_p);
else if (verbose_flag)
fnotice (stderr, "could not find specs file %s\n", p1);
continue;
@@ -1834,7 +1836,7 @@
#endif
}
- set_spec (p2, *(sl->ptr_spec));
+ set_spec (p2, *(sl->ptr_spec), user_p);
if (sl->alloc_p)
free (CONST_CAST (char *, *(sl->ptr_spec)));
@@ -1900,7 +1902,7 @@
if (! strcmp (suffix, "*link_command"))
link_command_spec = spec;
else
- set_spec (suffix + 1, spec);
+ set_spec (suffix + 1
Re: [driver, LTO Patch]: Resurrect user specs support
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). -- Joseph S. Myers jos...@codesourcery.com
Re: [driver, LTO Patch]: Resurrect user specs support
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
Re: [driver, LTO Patch]: Resurrect user specs support
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. -- Joseph S. Myers jos...@codesourcery.com
Re: [driver, LTO Patch]: Resurrect user specs support
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
Re: [driver, LTO Patch]: Resurrect user specs support
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). -- Joseph S. Myers jos...@codesourcery.com
Re: [driver, LTO Patch]: Resurrect user specs support
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
Re: [driver, LTO Patch]: Resurrect user specs support
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? -- Joseph S. Myers jos...@codesourcery.com
Re: [driver, LTO Patch]: 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
Index: gcc/gcc.c
===
--- gcc/gcc.c (revision 187500)
+++ gcc/gcc.c (working copy)
@@ -190,8 +190,8 @@
static void store_arg (const char *, int, int);
static void insert_wrapper (const char *);
static char *load_specs (const char *);
-static void read_specs (const char *, int);
-static void set_spec (const char *, const char *);
+static void read_specs (const char *, bool, bool);
+static void set_spec (const char *, const char *, bool);
static struct compiler *lookup_compiler (const char *, size_t, const char *);
static char *build_search_list (const struct path_prefix *, const char *,
bool, bool);
@@ -227,9 +227,9 @@
static void do_self_spec (const char *);
static const char *find_file (const char *);
static int is_directory (const char *, bool);
-static const char *validate_switches (const char *);
+static const char *validate_switches (const char *, bool);
static void validate_all_switches (void);
-static inline void validate_switches_from_spec (const char *);
+static inline void validate_switches_from_spec (const char *, bool);
static void give_switch (int, int);
static int used_arg (const char *, int);
static int default_arg (const char *, int);
@@ -1170,11 +1170,12 @@
const char **ptr_spec; /* pointer to the spec itself. */
struct spec_list *next; /* Next spec in linked list. */
int name_len; /* length of the name */
- int alloc_p; /* whether string was allocated */
+ bool user_p; /* whether string come from file spec. */
+ bool alloc_p; /* whether string was allocated */
};
#define INIT_STATIC_SPEC(NAME,PTR) \
-{ NAME, NULL, PTR, (struct spec_list *) 0, sizeof (NAME) - 1, 0 }
+ { NAME, NULL, PTR, (struct spec_list *) 0, sizeof (NAME) - 1, false, false }
/* List of statically defined specs. */
static struct spec_list static_specs[] =
@@ -1478,7 +1479,7 @@
current spec. */
static void
-set_spec (const char *name, const char *spec)
+set_spec (const char *name, const char *spec, bool user_p)
{
struct spec_list *sl;
const char *old_spec;
@@ -1530,7 +1531,8 @@
if (old_spec && sl->alloc_p)
free (CONST_CAST(char *, old_spec));
- sl->alloc_p = 1;
+ sl->user_p = user_p;
+ sl->alloc_p = true;
}
/* Accumulate a command (program name and args), and run it. */
@@ -1686,7 +1688,7 @@
Anything invalid in the file is a fatal error. */
static void
-read_specs (const char *filename, int main_p)
+read_specs (const char *filename, bool main_p, bool user_p)
{
char *buffer;
char *p;
@@ -1735,7 +1737,7 @@
p[-2] = '\0';
new_filename = find_a_file (&startfile_prefixes, p1, R_OK, true);
- read_specs (new_filename ? new_filename : p1, FALSE);
+ read_specs (new_filename ? new_filename : p1, false, user_p);
continue;
}
else if (!strncmp (p1, "%include_noerr", size
Re: [driver, LTO Patch]: Resurrect user specs support
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. 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. -- Joseph S. Myers jos...@codesourcery.com
