diff mbox

[driver,LTO] : Resurrect user specs support

Message ID 4FBA1623.8010703@st.com
State New
Headers show

Commit Message

Christian Bruel May 21, 2012, 10:17 a.m. UTC
Hello,

This patch restores the --specs flags functionality.

There are 2 parts
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
2) --specs options need to be propagated to COLLECT_GCC_OPTIONS. Without
this lto1 might be given user flags without the corresponding --spec
file. Note that --specs LTO is broken since 4.6 included.

Regression tests included. Bootstrapped and reg tested on [x86,sh4]
Linux and regression tested on sh-superh-elf with proprietary BSP
supports. Also tested with LTO.

Some references on the issue:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49858
http://gcc.gnu.org/ml/gcc/2012-05/msg00079.html

Having experienced with the implementation a bit, I prefer to implement
this lazy check, rather than explicitly expose the user flags thru a new
-fextension option, because I think the full previous behavior can be
restored without lost of missing diagnostic (as opposed with older
version (< 4.6) and without ambiguity : There is an error for
unrecognized flags either from internal .opt or explicitly provided in a
spec file.

Thanks a lot for your feedback. I'd like to candidate this change for
the 4.7 and trunk branches.

Christian

Comments

Joseph Myers May 22, 2012, 1:52 p.m. UTC | #1
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.
diff mbox

Patch

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-18  Christian Bruel  <christian.bruel@st.com>

	* gcc.dg/spec-options.c: Test recognized --specs option.
	* gcc.dg/spec-options-2.c: Test unknown --specs option.
	* gcc.dg/foo.specs: New file.

Index: gcc/gcc.c
===================================================================
--- gcc/gcc.c	(revision 187500)
+++ gcc/gcc.c	(working copy)
@@ -190,7 +190,7 @@ 
 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 read_specs (const char *, bool, bool);
 static void set_spec (const char *, const char *);
 static struct compiler *lookup_compiler (const char *, size_t, const char *);
 static char *build_search_list (const struct path_prefix *, const char *,
@@ -1472,6 +1472,47 @@ 
 
   specs = sl;
 }
+struct file_specs
+{
+  struct file_specs *next;
+  const char *spec;
+};
+
+static struct file_specs *file_specs_head;
+
+static void
+record_file_spec (const char *spec)
+{
+  struct file_specs *user = XNEW (struct file_specs);
+
+  user->next = file_specs_head;
+
+  if (spec[0] == '+' && ISSPACE ((unsigned char)spec[1]))
+    user->spec = spec + 1;
+  else
+    user->spec = spec;
+
+  file_specs_head = user;
+}
+
+static bool
+file_spec_p (const char *name)
+{
+  struct file_specs *user;
+
+  for (user = file_specs_head; user; user = user->next)
+    {
+      const char *p = user->spec;
+      char c;
+      while ((c = *p++))
+	if (c == '%' && (*p == '{' || *p == '<' || (*p == 'W' && *++p == '{')))
+	  if (!strcmp (name, p + 1))
+	    return true;
+    }
+
+  return false;
+}
+
 
 /* Change the value of spec NAME to SPEC.  If SPEC is empty, then the spec is
    removed; If the spec starts with a + then SPEC is added to the end of the
@@ -1686,7 +1727,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 +1776,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 +1797,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;
@@ -1899,7 +1940,12 @@ 
 	  if (! strcmp (suffix, "*link_command"))
 	    link_command_spec = spec;
 	  else
-	    set_spec (suffix + 1, spec);
+	    {
+	      set_spec (suffix + 1, spec);
+
+	      if (user_p)
+		record_file_spec (spec);
+	    }
 	}
       else
 	{
@@ -2806,20 +2852,22 @@ 
    SWITCH_IGNORE_PERMANENTLY to indicate this switch should be ignored
    in all do_spec calls afterwards.  Used for %<S from self specs.
    The `validated' field is nonzero if any spec has looked at this switch;
-   if it remains zero at the end of the run, it must be meaningless.  */
+   if it remains zero at the end of the run, it must be meaningless.
+   SWITCH_USER to indicate this switch could be defined in a user spec file.  */
 
 #define SWITCH_LIVE    			(1 << 0)
 #define SWITCH_FALSE   			(1 << 1)
 #define SWITCH_IGNORE			(1 << 2)
 #define SWITCH_IGNORE_PERMANENTLY	(1 << 3)
 #define SWITCH_KEEP_FOR_GCC		(1 << 4)
+#define SWITCH_USER                     (1 << 5)
 
 struct switchstr
 {
   const char *part1;
   const char **args;
   unsigned int live_cond;
-  unsigned char validated;
+  bool validated;
   unsigned char ordering;
 };
 
@@ -3086,11 +3134,11 @@ 
 }
 
 /* Save an option OPT with N_ARGS arguments in array ARGS, marking it
-   as validated if VALIDATED.  */
+   as validated if VALIDATED, and USER to check after user spec file are read. */
 
 static void
 save_switch (const char *opt, size_t n_args, const char *const *args,
-	     bool validated)
+	     bool validated, bool user)
 {
   alloc_switch ();
   switches[n_switches].part1 = opt + 1;
@@ -3103,7 +3151,7 @@ 
       switches[n_switches].args[n_args] = NULL;
     }
 
-  switches[n_switches].live_cond = 0;
+  switches[n_switches].live_cond = user ? SWITCH_USER : 0;
   switches[n_switches].validated = validated;
   switches[n_switches].ordering = 0;
   n_switches++;
@@ -3123,9 +3171,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, false);
       return false;
     }
+
+  if (decoded->opt_index == OPT_SPECIAL_unknown)
+    {
+      save_switch (decoded->canonical_option[0],
+		   decoded->canonical_option_num_elements - 1,
+		   &decoded->canonical_option[1], false, true);
+      return false;
+    }
   else
     return true;
 }
@@ -3150,7 +3206,7 @@ 
   else
     save_switch (decoded->canonical_option[0],
 		 decoded->canonical_option_num_elements - 1,
-		 &decoded->canonical_option[1], false);
+		 &decoded->canonical_option[1], false, false);
 }
 
 static const char *spec_lang = 0;
@@ -3293,7 +3349,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, false);
       return true;
 
     case OPT_Wa_:
@@ -3378,12 +3434,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, false);
       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, false);
       return true;
 
     case OPT_save_temps:
@@ -3426,7 +3482,7 @@ 
 	  user_specs_head = user;
 	user_specs_tail = user;
       }
-      do_save = false;
+      validated = true;
       break;
 
     case OPT__sysroot_:
@@ -3505,7 +3561,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, false);
       return true;
 
     case OPT_static_libgcc:
@@ -3528,7 +3584,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, false);
   return true;
 }
 
@@ -4003,8 +4059,8 @@ 
 
       /* Ignore elided switches.  */
       if ((switches[i].live_cond
-	   & (SWITCH_IGNORE | SWITCH_KEEP_FOR_GCC))
-	  == SWITCH_IGNORE)
+	   & (SWITCH_IGNORE | SWITCH_KEEP_FOR_GCC | SWITCH_USER))
+	  == (SWITCH_IGNORE | SWITCH_USER))
 	continue;
 
       obstack_grow (&collect_obstack, "'-", 2);
@@ -4330,7 +4386,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, false);
 	      break;
 
 	    default:
@@ -5789,7 +5845,7 @@ 
 
   /* If we already processed this switch and determined if it was
      live or not, return our past determination.  */
-  if (switches[switchnum].live_cond != 0)
+  if ((switches[switchnum].live_cond & ~SWITCH_USER) != 0)
     return ((switches[switchnum].live_cond & SWITCH_LIVE) != 0
 	    && (switches[switchnum].live_cond & SWITCH_FALSE) == 0
 	    && (switches[switchnum].live_cond & SWITCH_IGNORE_PERMANENTLY)
@@ -5861,7 +5917,7 @@ 
 static void
 give_switch (int switchnum, int omit_first_word)
 {
-  if ((switches[switchnum].live_cond & SWITCH_IGNORE) != 0)
+  if (switches[switchnum].live_cond & (SWITCH_IGNORE | SWITCH_USER))
     return;
 
   if (!omit_first_word)
@@ -6269,7 +6325,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 +6338,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.  */
@@ -6402,7 +6458,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.  */
@@ -6501,7 +6557,7 @@ 
 
   for (i = 0; (int) i < n_switches; i++)
     if (! switches[i].validated)
-      error ("unrecognized option %<-%s%>", switches[i].part1);
+      error ("unrecognized command line option %<-%s%>", switches[i].part1);
 
   /* Obey some of the options.  */
 
@@ -7107,9 +7163,16 @@ 
     {
       /* 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;
+	if (!switches[i].validated && !strncmp (switches[i].part1, atom, len))
+	  {
+	    if (switches[i].live_cond & SWITCH_USER)
+	      {
+		if (file_spec_p (start))
+		  switches[i].validated = true;
+	      }
+	    else if (starred || switches[i].part1[len] == 0)
+	      switches[i].validated = true;
+	  }
     }
 
   if (*p) p++;
@@ -8056,7 +8119,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,15 @@ 
+/* Check that -mfoo 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 -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,17 @@ 
+/* Check that -mbar is rejected if not defined in a user spec.  */
+/* { dg-do compile } */
+/* { dg-options "-B${srcdir}/gcc.dg --specs=foo.specs -mbar" } */
+/* { dg-error "unrecognized command line option" "" { target *-*-* } 0 } */
+
+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: -DFOO}
+