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", sizeof "%include_noerr" - 1) @@ -1756,7 +1758,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; @@ -1833,7 +1835,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))); @@ -1899,7 +1901,7 @@ if (! strcmp (suffix, "*link_command")) link_command_spec = spec; else - set_spec (suffix + 1, spec); + set_spec (suffix + 1, spec, user_p); } else { @@ -2819,8 +2821,9 @@ const char *part1; const char **args; unsigned int live_cond; - unsigned char validated; - unsigned char ordering; + bool known; + bool validated; + bool ordering; }; static struct switchstr *switches; @@ -3086,11 +3089,11 @@ } /* Save an option OPT with N_ARGS arguments in array ARGS, marking it - as validated if VALIDATED. */ + as validated if VALIDATED and KNOWN if it is an internal switch. */ static void save_switch (const char *opt, size_t n_args, const char *const *args, - bool validated) + bool validated, bool known) { alloc_switch (); switches[n_switches].part1 = opt + 1; @@ -3105,6 +3108,7 @@ switches[n_switches].live_cond = 0; switches[n_switches].validated = validated; + switches[n_switches].known = known; switches[n_switches].ordering = 0; n_switches++; } @@ -3123,9 +3127,17 @@ diagnosed only if there are warnings. */ save_switch (decoded->canonical_option[0], decoded->canonical_option_num_elements - 1, - &decoded->canonical_option[1], false); + &decoded->canonical_option[1], false, true); return false; } + if (decoded->opt_index == OPT_SPECIAL_unknown) + { + /* Give it a chance to define it a a spec file. */ + save_switch (decoded->canonical_option[0], + decoded->canonical_option_num_elements - 1, + &decoded->canonical_option[1], false, false); + return false; + } else return true; } @@ -3150,7 +3162,7 @@ else save_switch (decoded->canonical_option[0], decoded->canonical_option_num_elements - 1, - &decoded->canonical_option[1], false); + &decoded->canonical_option[1], false, true); } static const char *spec_lang = 0; @@ -3293,7 +3305,7 @@ compare_debug_opt = NULL; else compare_debug_opt = arg; - save_switch (compare_debug_replacement_opt, 0, NULL, validated); + save_switch (compare_debug_replacement_opt, 0, NULL, validated, true); return true; case OPT_Wa_: @@ -3378,12 +3390,12 @@ case OPT_L: /* Similarly, canonicalize -L for linkers that may not accept separate arguments. */ - save_switch (concat ("-L", arg, NULL), 0, NULL, validated); + save_switch (concat ("-L", arg, NULL), 0, NULL, validated, true); return true; case OPT_F: /* Likewise -F. */ - save_switch (concat ("-F", arg, NULL), 0, NULL, validated); + save_switch (concat ("-F", arg, NULL), 0, NULL, validated, true); return true; case OPT_save_temps: @@ -3426,7 +3438,7 @@ user_specs_head = user; user_specs_tail = user; } - do_save = false; + validated = true; break; case OPT__sysroot_: @@ -3505,7 +3517,7 @@ save_temps_prefix = xstrdup (arg); /* On some systems, ld cannot handle "-o" without a space. So split the option from its argument. */ - save_switch ("-o", 1, &arg, validated); + save_switch ("-o", 1, &arg, validated, true); return true; case OPT_static_libgcc: @@ -3528,7 +3540,7 @@ if (do_save) save_switch (decoded->canonical_option[0], decoded->canonical_option_num_elements - 1, - &decoded->canonical_option[1], validated); + &decoded->canonical_option[1], validated, true); return true; } @@ -3955,7 +3967,7 @@ NULL); switches[n_switches].args = 0; switches[n_switches].live_cond = 0; - switches[n_switches].validated = 0; + switches[n_switches].validated = false; switches[n_switches].ordering = 0; n_switches++; compare_debug = 1; @@ -4330,7 +4342,7 @@ save_switch (decoded_options[j].canonical_option[0], (decoded_options[j].canonical_option_num_elements - 1), - &decoded_options[j].canonical_option[1], false); + &decoded_options[j].canonical_option[1], false, true); break; default: @@ -5195,7 +5207,7 @@ && (have_wildcard || switches[i].part1[len] == '\0')) { switches[i].live_cond |= switch_option; - switches[i].validated = 1; + switches[i].validated = true; } p += len; @@ -5808,7 +5820,7 @@ for (i = switchnum + 1; i < n_switches; i++) if (switches[i].part1[0] == 'O') { - switches[switchnum].validated = 1; + switches[switchnum].validated = true; switches[switchnum].live_cond = SWITCH_FALSE; return 0; } @@ -5822,7 +5834,7 @@ if (switches[i].part1[0] == name[0] && ! strcmp (&switches[i].part1[1], &name[4])) { - switches[switchnum].validated = 1; + switches[switchnum].validated = true; switches[switchnum].live_cond = SWITCH_FALSE; return 0; } @@ -5837,7 +5849,7 @@ && switches[i].part1[3] == '-' && !strcmp (&switches[i].part1[4], &name[1])) { - switches[switchnum].validated = 1; + switches[switchnum].validated = true; switches[switchnum].live_cond = SWITCH_FALSE; return 0; } @@ -5901,7 +5913,7 @@ } do_spec_1 (" ", 0, NULL); - switches[switchnum].validated = 1; + switches[switchnum].validated = true; } /* Search for a file named NAME trying various prefixes including the @@ -6269,7 +6281,7 @@ specs_file = find_a_file (&startfile_prefixes, "specs", R_OK, true); /* Read the specs file unless it is a default one. */ if (specs_file != 0 && strcmp (specs_file, "specs")) - read_specs (specs_file, TRUE); + read_specs (specs_file, true, false); else init_spec (); @@ -6282,7 +6294,7 @@ strcat (specs_file, just_machine_suffix); strcat (specs_file, "specs"); if (access (specs_file, R_OK) == 0) - read_specs (specs_file, TRUE); + read_specs (specs_file, true, false); /* Process any configure-time defaults specified for the command line options, via OPTION_DEFAULT_SPECS. */ @@ -6326,7 +6338,7 @@ { obstack_grow (&obstack, "%(sysroot_spec) ", strlen ("%(sysroot_spec) ")); obstack_grow0 (&obstack, link_spec, strlen (link_spec)); - set_spec ("link", XOBFINISH (&obstack, const char *)); + set_spec ("link", XOBFINISH (&obstack, const char *), false); } #endif @@ -6402,7 +6414,7 @@ { char *filename = find_a_file (&startfile_prefixes, uptr->filename, R_OK, true); - read_specs (filename ? filename : uptr->filename, FALSE); + read_specs (filename ? filename : uptr->filename, false, true); } /* Process any user self specs. */ @@ -7041,14 +7053,14 @@ } static inline void -validate_switches_from_spec (const char *spec) +validate_switches_from_spec (const char *spec, bool user) { const char *p = spec; char c; while ((c = *p++)) if (c == '%' && (*p == '{' || *p == '<' || (*p == 'W' && *++p == '{'))) /* We have a switch spec. */ - p = validate_switches (p + 1); + p = validate_switches (p + 1, user); } static void @@ -7058,20 +7070,20 @@ struct spec_list *spec; for (comp = compilers; comp->spec; comp++) - validate_switches_from_spec (comp->spec); + validate_switches_from_spec (comp->spec, false); /* Look through the linked list of specs read from the specs file. */ for (spec = specs; spec; spec = spec->next) - validate_switches_from_spec (*spec->ptr_spec); + validate_switches_from_spec (*spec->ptr_spec, spec->user_p); - validate_switches_from_spec (link_command_spec); + validate_switches_from_spec (link_command_spec, false); } /* Look at the switch-name that comes after START and mark as valid all supplied switches that match it. */ static const char * -validate_switches (const char *start) +validate_switches (const char *start, bool user_spec) { const char *p = start; const char *atom; @@ -7108,8 +7120,9 @@ /* Mark all matching switches as valid. */ for (i = 0; i < n_switches; i++) if (!strncmp (switches[i].part1, atom, len) - && (starred || switches[i].part1[len] == 0)) - switches[i].validated = 1; + && (starred || switches[i].part1[len] == '\0') + && (switches[i].known || user_spec)) + switches[i].validated = true; } if (*p) p++; @@ -7124,9 +7137,9 @@ { p++; if (*p == '{' || *p == '<') - p = validate_switches (p+1); + p = validate_switches (p+1, user_spec); else if (p[0] == 'W' && p[1] == '{') - p = validate_switches (p+2); + p = validate_switches (p+2, user_spec); } else p++; @@ -8056,7 +8069,7 @@ abort (); file = find_a_file (&startfile_prefixes, argv[0], R_OK, true); - read_specs (file ? file : argv[0], FALSE); + read_specs (file ? file : argv[0], false, false); return NULL; }
Index: gcc/testsuite/gcc.dg/spec-options.c =================================================================== --- gcc/testsuite/gcc.dg/spec-options.c (revision 0) +++ gcc/testsuite/gcc.dg/spec-options.c (revision 0) @@ -0,0 +1,16 @@ +/* Check that -mfoo is accepted if defined in a user spec + and that it is not passed on the command line. */ +/* { dg-do compile } */ +/* { dg-do run { target sh*-*-* } } */ +/* { dg-options "-B${srcdir}/gcc.dg --specs=foo.specs -mfoo" } */ + +extern void abort(void); + +int main(void) +{ +#ifdef FOO + return 0; +#else + abort(); +#endif +} Index: gcc/testsuite/gcc.dg/spec-options-2.c =================================================================== --- gcc/testsuite/gcc.dg/spec-options-2.c (revision 0) +++ gcc/testsuite/gcc.dg/spec-options-2.c (revision 0) @@ -0,0 +1,15 @@ +/* Check that -tfoo is accepted if defined in a user spec. */ +/* { dg-do compile } */ +/* { dg-do run { target sh*-*-* } } */ +/* { dg-options "-B${srcdir}/gcc.dg --specs=foo.specs -tfoo" } */ + +extern void abort(void); + +int main(void) +{ +#ifdef FOO + return 0; +#else + abort(); +#endif +} Index: gcc/testsuite/gcc.dg/foo.specs =================================================================== --- gcc/testsuite/gcc.dg/foo.specs (revision 0) +++ gcc/testsuite/gcc.dg/foo.specs (revision 0) @@ -0,0 +1,3 @@ +*cc1runtime: ++ %{mfoo|tfoo: -DFOO} %<mfoo +