Re: [driver, LTO Patch]: Resurrect user specs support

[Christian Bruel <christian dot bruel at st dot com>]

> 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 %<flag rule in a internal GCC spec that is only
defined in a --spec file..., but that would be a forbidden use, caught
by an error.

So I tested the following semantics, with the ones that you pointed.

1) undefined switch
===================
 unchanged:
passing a switch that is not declared in a .opt internal file and not
defined in a --spec file is an error.

If it is not declared in a .opt but defined in a --spec file it is accepted

2) defined unchanged
============
  a) passing a switch that is declared in a .opt internal file, but is
not in a spec is an error

  b) passing a switch that is declared in a .opt internal file, and used
in any spec rule is accepted. even it it is just to ignore it %<

  such switch are marked with the .know field of the 'struct switchstr'
when it is not decoded as OPT_SPECIAL_unknown.

3) defined changed
==========
  a) passing a switch that is defined in a --spec file is accepted.
Because the driver will necessary process the spec definition rule at
some point (starting from EXTRA_SPECS). It there is no definition, then
it is an error

  b) if the switch is used in the <* case it is accepted

     * A switch handled in the <* case that is defined in a --spec file
doesn't not need to be  validated at this point, because it will be
validated when processing the spec option.

    * If there is no spec rule for this switch this will be an error.

This implementation should change the current semantics when --spec is
not used, or when it is used only new substitutes should be allowed.

I'm attaching here a revised version of the patch that contains the
removal of the 'validated' field in do_specs for non internal flags,

thanks for your feedback.

Christian

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, spec, user_p);
 	}
       else
 	{
@@ -2820,8 +2822,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;
@@ -3087,11 +3090,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;
@@ -3106,6 +3109,7 @@
 
   switches[n_switches].live_cond = 0;
   switches[n_switches].validated = validated;
+  switches[n_switches].known = known;
   switches[n_switches].ordering = 0;
   n_switches++;
 }
@@ -3124,9 +3128,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;
 }
@@ -3151,7 +3163,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;
@@ -3294,7 +3306,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_:
@@ -3379,12 +3391,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:
@@ -3427,7 +3439,7 @@
 	  user_specs_head = user;
 	user_specs_tail = user;
       }
-      do_save = false;
+      validated = true;
       break;
 
     case OPT__sysroot_:
@@ -3506,7 +3518,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:
@@ -3529,7 +3541,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;
 }
 
@@ -3956,7 +3968,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;
@@ -4331,7 +4343,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:
@@ -5203,8 +5215,12 @@
 		if (!strncmp (switches[i].part1, p, len - have_wildcard)
 		    && (have_wildcard || switches[i].part1[len] == '\0'))
 		  {
+		    /* User switch be validated from validate_all_switches.
+		       when the definition is seen from the spec file.
+		       If not defined anywhere, will be rejected.  */
+		    if (switches[i].known)
+		      switches[i].validated = true;
 		    switches[i].live_cond |= switch_option;
-		    switches[i].validated = 1;
 		  }
 
 	      p += len;
@@ -5831,7 +5847,9 @@
 	    if (switches[i].part1[0] == name[0]
 		&& ! strcmp (&switches[i].part1[1], &name[4]))
 	      {
-		switches[switchnum].validated = 1;
+		/* --specs are validated with the validate_switches mechanism.  */
+		if (switches[switchnum].known)
+		  switches[switchnum].validated = true;
 		switches[switchnum].live_cond = SWITCH_FALSE;
 		return 0;
 	      }
@@ -5846,7 +5864,9 @@
 		&& switches[i].part1[3] == '-'
 		&& !strcmp (&switches[i].part1[4], &name[1]))
 	      {
-		switches[switchnum].validated = 1;
+		/* --specs are validated with the validate_switches mechanism.  */
+		if (switches[switchnum].known)
+		  switches[switchnum].validated = true;
 		switches[switchnum].live_cond = SWITCH_FALSE;
 		return 0;
 	      }
@@ -6278,7 +6298,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 ();
 
@@ -6291,7 +6311,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.  */
@@ -6335,7 +6355,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
 
@@ -6411,7 +6431,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.  */
@@ -7050,14 +7070,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
@@ -7067,20 +7087,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;
@@ -7117,8 +7137,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++;
@@ -7133,9 +7154,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++;
@@ -8065,7 +8086,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/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 187927)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,20 @@
+<<<<<<< .mine
+2012-05-18  Christian Bruel  <christian.bruel@st.com>
+
+	* gcc.c (save_switch) Add user_p parameter. Set live_cond.
+	(read_specs): Likewise. Call record_file_spec.
+	(main): Call read_specs with user_p.
+	(record_file_spec): New function.
+	(file_spec_p): Likewise.
+	(SWITCH_USER): New flag.
+	(driver_unknown_option_callback): Test OPT_SPECIAL_unknown.
+	Add user_p parameter.
+	(set_collect_gcc_options): Don't ignore SWITCH_USER.
+	(check_live_switch): Likewise.
+	(validate_switches): Validate SWITCH_USER options.
+	(driver_handle_option): Propagate OPT_specs to collect2.
+
+=======
 2012-05-27  Nathan Sidwell  <nathan@acm.org>
 
 	* tree.c (build_constructor): Propagate TREE_SIDE_EFFECTS.
@@ -883,6 +900,7 @@
 	* tree-vrp.c (extract_range_from_binary_expr_1): Handle LSHIFT_EXPRs
 	by constants.
 
+>>>>>>> .r187927
 2012-05-15  Tristan Gingold  <gingold@adacore.com>
 
 	* tree-ssa-strlen.c (get_string_length): Convert lhs if needed.
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,2 @@
+*cc1runtime:
++ %{mfoo|tfoo: -DFOO} %<mfoo