Re: [PATCH][i386]Fix PR 57756

[Sriraman Tallam <tmsriram at google dot com>]

On Thu, Oct 17, 2013 at 9:52 AM, Michael Meissner
<meissner@linux.vnet.ibm.com> wrote:
> 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.

I would need the help of target maintainers to fix it this way since
it touches every target and it would take time for me to build and
test every target.

Michael, OTOH, I dont see any other targets other than i386 and rs6000
(grepping for "specific_save" and "specific_restore") using
function_specific save and restore functions. Would it be easier then
to just add back that line to "opth-gen.awk"?,the patch is below.
Since, they are not using function specific opts, they presumably
should not be validating target options a lot.

FWIW, I am testing this patch on i386 and powerpc right now.

Index: opth-gen.awk
===================================================================
--- opth-gen.awk (revision 203779)
+++ opth-gen.awk (working copy)
@@ -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_flags"
 print "#endif"
 print "#endif"
 print ""


>
> 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.

Lesson learnt, I clearly messed up.

  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
>