Re: [PATCH][i386]Fix PR 57756

[Michael Meissner <meissner at linux dot vnet dot ibm dot com>]

On Thu, Oct 17, 2013 at 09:28:26AM -0700, Steve Ellcey wrote:
> On Thu, 2013-10-17 at 06:03 -0700, Diego Novillo wrote:
> > On Wed, Oct 16, 2013 at 6:06 PM, David Edelsohn <dje.gcc@gmail.com> wrote:
> > 
> > > How is Google going to change its patch commit policies to ensure that
> > > this does not happen again?
> > 
> > There is nothing to change.  Google follows
> > http://gcc.gnu.org/contribute.html, like everyone else. Sri just fixed
> > the oversight and if there is any other fallout from his patch, he
> > will address it.
> > 
> > I don't see anything out of the ordinary here.
> > 
> > 
> > Diego.
> 
> FYI: I just want to make sure everyone is aware that there are still
> broken targets.  rs6000 may be fixed but mips still doesn't work and
> I presume other platforms like sparc are also still broken.
> 
> /local/home/sellcey/nightly_full/src/gcc/gcc/c-family/c-cppbuiltin.c: In
> function 'void cpp_atomic_builtins(cpp_reader*)':
> /local/home/sellcey/nightly_full/src/gcc/gcc/c-family/c-cppbuiltin.c:594:7: error: 'target_flags_explicit' was not declared in this scope
> /local/home/sellcey/nightly_full/src/gcc/gcc/c-family/c-cppbuiltin.c:606:7: error: 'target_flags_explicit' was not declared in this scope
> /local/home/sellcey/nightly_full/src/gcc/gcc/c-family/c-cppbuiltin.c:618:7: error: 'target_flags_explicit' was not declared in this scope
> /local/home/sellcey/nightly_full/src/gcc/gcc/c-family/c-cppbuiltin.c:630:7: error: 'target_flags_explicit' was not declared in this scope
> make[1]: *** [c-family/c-cppbuiltin.o] Error 1
> 
> Sriraman, are you looking at how to fix this globally or are the target
> maintainers expected to fix this?  Currently I am using this one line
> patch to get things building, but I presume this is not what we want
> long term.

You probably want to do something similar to what I did in the powerpc.

Provide a target_flags_explicit as a Variable in <machine>.opt.  This creates
x_target_flags_explicit in the gcc_options structure, and does the definition.
Here is the powerpc changes that does this for rs6000_isa_flags.

Index: gcc/config/rs6000/rs6000.opt
===================================================================
--- gcc/config/rs6000/rs6000.opt	(revision 203723)
+++ gcc/config/rs6000/rs6000.opt	(working copy)
@@ -30,6 +30,9 @@ TargetSave
 HOST_WIDE_INT x_rs6000_isa_flags
 
 ;; Miscellaneous flag bits that were set explicitly by the user
+Variable
+HOST_WIDE_INT rs6000_isa_flags_explicit
+
 TargetSave
 HOST_WIDE_INT x_rs6000_isa_flags_explicit
 

Then in your option override function, initialize it from the
global_set.target_flags:

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 203723)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -2796,6 +2796,10 @@ rs6000_option_override_internal (bool gl
     = ((global_init_p || target_option_default_node == NULL)
        ? NULL : TREE_TARGET_OPTION (target_option_default_node));
 
+  /* Remember the explicit arguments.  */
+  if (global_init_p)
+    rs6000_isa_flags_explicit = global_options_set.x_rs6000_isa_flags;
+
   /* On 64-bit Darwin, power alignment is ABI-incompatible with some C
      library functions, so warn about it. The flag may be useful for
      performance studies from time to time though, so don't disable it

If you have target specific save/restore functions, you need to deal with the
gcc_options argument.  I think it should use a field in the gcc_options
structure, which was the point of creating the variable in the .opt file
above.  That way, when the save/restore function are called in the future not
using the global switches, it will continue to work.  Here is the rs6000
changes:

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 203723)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -29995,19 +29999,22 @@ rs6000_set_current_function (tree fndecl
 /* Save the current options */
 
 static void
-rs6000_function_specific_save (struct cl_target_option *ptr)
+rs6000_function_specific_save (struct cl_target_option *ptr,
+			       struct gcc_options *opts)
 {
-  ptr->x_rs6000_isa_flags = rs6000_isa_flags;
-  ptr->x_rs6000_isa_flags_explicit = rs6000_isa_flags_explicit;
+  ptr->x_rs6000_isa_flags = opts->x_rs6000_isa_flags;
+  ptr->x_rs6000_isa_flags_explicit = opts->x_rs6000_isa_flags_explicit;
 }
 
 /* Restore the current options */
 
 static void
-rs6000_function_specific_restore (struct cl_target_option *ptr)
+rs6000_function_specific_restore (struct gcc_options *opts,
+				  struct cl_target_option *ptr)
+				  
 {
-  rs6000_isa_flags = ptr->x_rs6000_isa_flags;
-  rs6000_isa_flags_explicit = ptr->x_rs6000_isa_flags_explicit;
+  opts->x_rs6000_isa_flags = ptr->x_rs6000_isa_flags;
+  opts->x_rs6000_isa_flags_explicit = ptr->x_rs6000_isa_flags_explicit;
   (void) rs6000_option_override_internal (false);
 }
 
The last change was to remove the rs6000_isa_flags_explicit declaration in
rs6000.h.  If you are using target_flags_explicit, you probably don't need
this.

Index: gcc/config/rs6000/rs6000.h
===================================================================
--- gcc/config/rs6000/rs6000.h	(revision 203723)
+++ gcc/config/rs6000/rs6000.h	(working copy)
@@ -593,9 +593,6 @@ extern int rs6000_vector_align[];
 #define MASK_PROTOTYPE			OPTION_MASK_PROTOTYPE
 #endif
 
-/* Explicit ISA options that were set.  */
-#define rs6000_isa_flags_explicit	global_options_set.x_rs6000_isa_flags
-
 /* For power systems, we want to enable Altivec and VSX builtins even if the
    user did not use -maltivec or -mvsx to allow the builtins to be used inside
    of #pragma GCC target or the target attribute to change the code level for a

> % git diff opth-gen.awk
> diff --git a/gcc/opth-gen.awk b/gcc/opth-gen.awk
> index 01c5ab6..46bd570 100644
> --- a/gcc/opth-gen.awk
> +++ b/gcc/opth-gen.awk
> @@ -114,6 +114,7 @@ print "};"
>  print "extern struct gcc_options global_options;"
>  print "extern const struct gcc_options global_options_init;"
>  print "extern struct gcc_options global_options_set;"
> +print "#define target_flags_explicit global_options_set.x_target_flag
> s"
>  print "#endif"
>  print "#endif"
>  print ""

In terms of the whole saga, mistakes happen, and presumably Sriraman Tallam
will do better in the future.  I do think it should be a requirement that if
you touch a backend file in a patch, that you do at least a cross compiler
build of that target to make sure that the syntax is correct.  For the major
hosted targets that we have machines for in the compile farm (like the powerpc,
mips, etc.) you should do a full bootstrap build, and it would be nice for a
make check on at least C/C++.  I did get angry yesterday, when this change
wasn't even tested with a build (and of course it lost me the better part of a
day of work to fix it).

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460, USA
email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797