[Bug target/67129] New: x86: erratic parsing of "#pragma GCC target ("...")"

[Bug target/67129] New: x86: erratic parsing of "#pragma GCC target ("...")"

[vogt at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67129

            Bug ID: 67129
           Summary: x86: erratic parsing of "#pragma GCC target ("...")"
           Product: gcc
           Version: 6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: vogt at linux dot vnet.ibm.com
  Target Milestone: ---

There's a bug in the way the "GCC target" pragma is translated into the
"target" attribute on x86, and probably also on (all?) other platforms that
base their implementation of the "GCC target" pragma and the "target" attribute
on the code in i386.c and i386-c.c:

According to the documentation, the last occurence of "#pragma GCC target
("...")" should always replace all previous occurences, and this is indeed what
happens.  On a machine with sse support, this code compiles just fine:

-- snip --
#pragma GCC target ("fpmath=sse")
#pragma GCC target ("no-sse")
void t1(void) { }
-- snip --

$ gcc -S t1.c
(ok)

The "no-sse" option wins, "fpmath=sse" is not used at all.

However, *sometimes* during processing of the #pragma, the value of the old
#pragma is used for checking validity of the new #pragma:

-- snip --
#pragma GCC target ("fpmath=sse")
#pragma GCC target ("no-sse")
void t2(void) { }
-- snip --

$ gcc -S t2.c
/t2.c:2:9: warning: SSE instruction set disabled, using 387 arithmetics
[enabled by default]
t2.c:3:1: warning: SSE instruction set disabled, using 387 arithmetics [enabled
by default]
t2.c:3:1: warning: SSE instruction set disabled, using 387 arithmetics [enabled
by default]

Since this is only a warning, compilation continues, and eventually the old
value gets discarded without causing any harm.  However, if there is a
combination of options that causes an error, gcc will fail to compile valid
code.

--

There is a bug in the implementation of i386-c.c:ix86_pragma_target_parse(). 
As far as I understand, when a "#pragma GCC target ("...")" is processed:

1. Use global_options as the target options structure.
2. Add the options defined in the string.
3. Check if the resulting options are valid.

The last step fails when the secons #pragma is parsed.  Then, when a function
definition begins:

4. Look up the most recent target pragma definition and the function's target
attribute and, if present, parse their strings in order.

So, for functions everything is fine, but any definitions between the last
#pragma and the next function definition is affected by the union of all
#pragmas (after the last reset).

Now, don't ask me why the warning in the example above is affected by the order
of the two pragmas.  I think to fix this, the function in i386-c.c should:

1. Make a working copy of the original global_options and global_options_set
(the values that were affected only by command line options).  No idea how to
get the original values at that point in the code.
2. Call ix86_valid_target_attribute_tree() with that copy.
3. If the result is calid, overwrite global_options with the modified values in
the copy.

[Bug target/67129] x86: erratic parsing of "#pragma GCC target ("...")"

[vogt at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67129

--- Comment #1 from Dominik Vogt <vogt at linux dot vnet.ibm.com> ---
Created attachment 36135
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36135&action=edit
no warning

Code that does not produce the warning.

[Bug target/67129] x86: erratic parsing of "#pragma GCC target ("...")"

[vogt at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67129

--- Comment #2 from Dominik Vogt <vogt at linux dot vnet.ibm.com> ---
Created attachment 36136
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36136&action=edit
warning

Code that produces the warning.

[Bug target/67129] x86: erratic parsing of "#pragma GCC target ("...")"

[vogt at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67129

--- Comment #3 from Dominik Vogt <vogt at linux dot vnet.ibm.com> ---
Sorry, the expample with t2() in the code snippet above should have the pragmas
in the opposite order:

-- snip --
#pragma GCC target ("no-sse")
#pragma GCC target ("fpmath=sse")
void t2(void) { }
-- snip --

The attached files ar all right, though.

[Bug target/67129] x86: erratic parsing of "#pragma GCC target ("...")"

[vogt at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67129

--- Comment #4 from Dominik Vogt <vogt at linux dot vnet.ibm.com> ---
Also, it is a bad idea to pass global_options_set in this call

  ix86_valid_target_attribute_tree (args, &global_options,
&global_options_set);

because it may be modified (via ix86_handle_option()).  Unlike global_options,
the structure global_options_set is never saved or restored, so any target
pragma or attribute may change the structure for the rest of the compilation
unit.  This may or may not cause weird things to happen.

[Bug target/67129] x86: erratic parsing of "#pragma GCC target ("...")"

[vogt at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67129

--- Comment #5 from Dominik Vogt <vogt at linux dot vnet.ibm.com> ---
Created attachment 36139
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36139&action=edit
experimental fix

The attached patch fixes the problem for me (on x86), but it really needs
reviewing.

[Bug target/67129] x86: erratic parsing of "#pragma GCC target ("...")"

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67129

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
So starting with GCC 7 we get a warning with the no-warning case:
<source>:3:13: warning: SSE instruction set disabled, using 387 arithmetics
 void t1(void) { }
             ^
<source>:3:13: warning: SSE instruction set disabled, using 387 arithmetics

Starting with GCC 11 we get a warning on the pragma itself:
<source>:2:29: warning: SSE instruction set disabled, using 387 arithmetics
    2 | #pragma GCC target ("no-sse")
      |                             ^
<source>:3:13: warning: SSE instruction set disabled, using 387 arithmetics
    3 | void t1(void) { }
      |             ^
<source>:3:13: warning: SSE instruction set disabled, using 387 arithmetics

[Bug target/67129] x86: erratic parsing of "#pragma GCC target ("...")"

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67129

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
   Target Milestone|---                         |11.0
         Resolution|---                         |FIXED

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Fixed by r11-3183 in GCC 11.
I have not checked what caused the change in GCC 7 though.

[Bug target/67129] x86: erratic parsing of "#pragma GCC target ("...")"

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67129

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Dominik Vogt from comment #4)
> Unlike
> global_options, the structure global_options_set is never saved or restored,
> so any target pragma or attribute may change the structure for the rest of
> the compilation unit.  This may or may not cause weird things to happen.

This is what r11-3183 solves exactly.