diff mbox series

[stage1] Add gcc_assert that &global_options are not dirty modified.

Message ID eb4571c7-ef8f-1808-dc79-0c93733070d8@suse.cz
State New
Headers show
Series [stage1] Add gcc_assert that &global_options are not dirty modified. | expand

Commit Message

Martin Liška March 19, 2020, 8:56 a.m. UTC
Hi.

As seen in the mentioned PR we do have issues related to modification
of global_options in context of #pragma GCC optimize/target and
the corresponding function attributes. The patch brings a sanity check
that these context related option modifications should not affect global
state. The patch lists known limitations (mentioned in the PR).

So far I bootstrapped and tested the patch on x86_64-linux-gnu and
ppc64le-linux-gnu.

I'm planning to work on the problematic options in next stage1 and this
patch will help me to catch another violations.

Ready to be installed in next stage1?
Thanks,
Martin

gcc/ChangeLog:

2020-03-17  Martin Liska  <mliska@suse.cz>

	PR tree-optimization/92860
	* opth-gen.awk: Generate new function gcc_options_check. Include
	known exceptions.

gcc/c-family/ChangeLog:

2020-03-17  Martin Liska  <mliska@suse.cz>

	PR tree-optimization/92860
	* c-attribs.c (handle_optimize_attribute):
	Save global options before parsing of an optimization attribute.
	* c-pragma.c (opt_stack): Add saved_global_options field.
	(handle_pragma_push_options): Save current global_options.
	(handle_pragma_pop_options): Check current options.
---
  gcc/c-family/c-attribs.c |  3 +++
  gcc/c-family/c-pragma.c  |  4 ++++
  gcc/opth-gen.awk         | 35 +++++++++++++++++++++++++++++++++++
  3 files changed, 42 insertions(+)

Comments

Jeff Law via Gcc-patches March 19, 2020, 9:09 a.m. UTC | #1
On Thu, Mar 19, 2020 at 09:56:00AM +0100, Martin Liška wrote:
> I'm planning to work on the problematic options in next stage1 and this
> patch will help me to catch another violations.
> 
> Ready to be installed in next stage1?

Isn't that costly even for the !flag_checking case?
struct gcc_options is over 5KB now.
So even just that
  gcc_options saved_global_options = global_options;
is a fair amount of work.
Can't you instead:
  gcc_options saved_global_options;
  if (flag_checking)
    saved_global_options = global_options;
?
Or for the opt_stack case have a pointer to the options and XNEW/XDELETE
it instead of having it directly in opt_stack?
I mean, optimize for the !flag_checking case...

	Jakub
Martin Liška March 19, 2020, 3:32 p.m. UTC | #2
On 3/19/20 10:09 AM, Jakub Jelinek wrote:
> I mean, optimize for the !flag_checking case...

Sure, I transformed both situations into heap memory allocation.
In gcc/c-family/c-attribs.c I faced maybe uninitialized warning when
I only assigned (and checked) the variable in flag_checking context.

Martin
Jeff Law via Gcc-patches March 19, 2020, 3:47 p.m. UTC | #3
On Thu, Mar 19, 2020 at 04:32:05PM +0100, Martin Liška wrote:
> +      gcc_options *saved_global_options = NULL;
> +      if (flag_checking)
> +	{
> +	  saved_global_options = (gcc_options *) xmalloc (sizeof (gcc_options));

XNEW (gcc_options) please.

> +      p->saved_global_options = (gcc_options *) xmalloc (sizeof (gcc_options));

Ditto.

> +      *p->saved_global_options = global_options;

	Jakub
Jeff Law via Gcc-patches March 19, 2020, 4:13 p.m. UTC | #4
On 3/19/20 9:32 AM, Martin Liška wrote:
> On 3/19/20 10:09 AM, Jakub Jelinek wrote:
>> I mean, optimize for the !flag_checking case...
> 
> Sure, I transformed both situations into heap memory allocation.
> In gcc/c-family/c-attribs.c I faced maybe uninitialized warning when
> I only assigned (and checked) the variable in flag_checking context.

I was mostly just curious about what was being checked and how so
I might be misunderstanding something but it looks to me like
the code generated by the loop below will be a very long series
(of hundreds?) of if statements, each doing the same thing but
each hardcoding different options names.  If that's correct, is
there an easy way to turn that repetitive series into a loop to
keep code (and string) size growth to a minimum?

Also, since the function prints output and the caller then aborts
on failure, would calling internal_error for the first mismatch
instead be more in keeping with how internal errors with additional
output are reported?

One last question/suggestion: if the efficiency of the checking is
at all a concern, would calling memcmp on the arrays first and only
looping to find the position of the mismatch, be viable? (I have no
idea if changes to some of the options are acceptable; if they are
this wouldn't work).

Martin

PS Since the function doesn't modify the option arrays it could
declare them const.

diff --git a/gcc/opth-gen.awk b/gcc/opth-gen.awk
index 856a69168a5..586213da3d3 100644
--- a/gcc/opth-gen.awk
+++ b/gcc/opth-gen.awk
@@ -119,6 +119,41 @@ print "#endif"
  print "#endif"
  print ""

+print "#if !defined(IN_LIBGCC2) && !defined(IN_TARGET_LIBS) && 
!defined(IN_RTS)"
+print "#ifndef GENERATOR_FILE"
+print "static inline bool gcc_options_check (gcc_options *ptr1, 
gcc_options *ptr2)"
+print "{"
+print "  bool result = true;"
+
+# all these options are mentioned in PR92860
+checked_options["flag_merge_constants"]++
+checked_options["param_max_fields_for_field_sensitive"]++
+checked_options["flag_omit_frame_pointer"]++
+checked_options["unroll_only_small_loops"]++
+
+for (i = 0; i < n_opts; i++) {
+	name = var_name(flags[i]);
+	if (name == "")
+		continue;
+
+	if (name in checked_options)
+		continue;
+	checked_options[name]++
+
+	print "  if (ptr1->x_" name " != ptr2->x_" name ")"
+	print "  {"
+	print "    if (result)"
+	print "      fprintf (stderr, \"Error: global_options are modified in 
local context:\\n\");"
+	print "    fprintf (stderr, \"  " name " (%ld/%ld)\\n\", (long 
int)ptr1->x_" name ", (long int)ptr2->x_" name ");"
+	print "    result = false;"
+	print "  }"
+}
+
+print "  return result;"
+print "}"
+print "#endif"
+print "#endif"
+
  # All of the optimization switches gathered together so they can be 
saved and restored.
  # This will allow attribute((cold)) to turn on space optimization.
Martin Liška March 20, 2020, 3:40 p.m. UTC | #5
On 3/19/20 5:13 PM, Martin Sebor wrote:
> On 3/19/20 9:32 AM, Martin Liška wrote:
>> On 3/19/20 10:09 AM, Jakub Jelinek wrote:
>>> I mean, optimize for the !flag_checking case...
>>>> Sure, I transformed both situations into heap memory allocation.
>> In gcc/c-family/c-attribs.c I faced maybe uninitialized warning when
>> I only assigned (and checked) the variable in flag_checking context.
> 

Hi Martin.

> I was mostly just curious about what was being checked and how so
> I might be misunderstanding something but it looks to me like
> the code generated by the loop below will be a very long series
> (of hundreds?) of if statements, each doing the same thing but
> each hardcoding different options names.  If that's correct, is
> there an easy way to turn that repetitive series into a loop to
> keep code (and string) size growth to a minimum?

Thank you very much for the feedback, it was useful. I realized that
the patch made a huge code bloat (mainly because of the string constants
and the fact that it didn't end quickly (with an internal_error).

The loop is here not possible because we compare struct fields.

> 
> Also, since the function prints output and the caller then aborts
> on failure, would calling internal_error for the first mismatch
> instead be more in keeping with how internal errors with additional
> output are reported?

I decided to call internal_error without string description of the error.
With that I have a reasonable code growth:

bloaty /tmp/cc1plus-after -- /tmp/cc1plus-before
      VM SIZE                     FILE SIZE
  --------------               --------------
   +0.1% +20.9Ki .text         +20.9Ki  +0.1%
   [ = ]       0 .debug_line   +2.38Ki  +0.0%
   [ = ]       0 .debug_info      +369  +0.0%
   [ = ]       0 .debug_str        +75  +0.0%
   +0.0%     +64 .rodata           +64  +0.0%
   [ = ]       0 .debug_ranges     +48  +0.0%
   +0.0%     +45 .dynstr           +45  +0.0%
   [ = ]       0 .strtab           +45  +0.0%
   +0.0%     +40 .eh_frame         +40  +0.0%
   +0.0%     +24 .dynsym           +24  +0.0%
   [ = ]       0 .symtab           +24  +0.0%
   +0.0%      +8 .eh_frame_hdr      +8  +0.0%
   +0.0%      +4 .gnu.hash          +4  +0.0%
   +0.0%      +4 .hash              +4  +0.0%
   +0.0%      +2 .gnu.version       +2  +0.0%
    +14%      +1 [LOAD [R]]         +1   +14%
   [ = ]       0 .debug_loc       -355  -0.0%
   [ = ]       0 [Unmapped]    -1.05Ki -36.5%
   +0.1% +21.0Ki TOTAL         +22.6Ki  +0.0%

So for debugging one will have to take a look at corresponding option-save.c line.
It does not seem to me a problem.

> 
> One last question/suggestion: if the efficiency of the checking is
> at all a concern, would calling memcmp on the arrays first and only
> looping to find the position of the mismatch, be viable? (I have no
> idea if changes to some of the options are acceptable; if they are
> this wouldn't work).

Well, I skip some fields in the struct and there can be string pointers, so I'm suggesting
not to use memcmp.

There's updated tested patch.
Martin

> 
> Martin
> 
> PS Since the function doesn't modify the option arrays it could
> declare them const.
> 
> diff --git a/gcc/opth-gen.awk b/gcc/opth-gen.awk
> index 856a69168a5..586213da3d3 100644
> --- a/gcc/opth-gen.awk
> +++ b/gcc/opth-gen.awk
> @@ -119,6 +119,41 @@ print "#endif"
>   print "#endif"
>   print ""
> 
> +print "#if !defined(IN_LIBGCC2) && !defined(IN_TARGET_LIBS) && !defined(IN_RTS)"
> +print "#ifndef GENERATOR_FILE"
> +print "static inline bool gcc_options_check (gcc_options *ptr1, gcc_options *ptr2)"
> +print "{"
> +print "  bool result = true;"
> +
> +# all these options are mentioned in PR92860
> +checked_options["flag_merge_constants"]++
> +checked_options["param_max_fields_for_field_sensitive"]++
> +checked_options["flag_omit_frame_pointer"]++
> +checked_options["unroll_only_small_loops"]++
> +
> +for (i = 0; i < n_opts; i++) {
> +    name = var_name(flags[i]);
> +    if (name == "")
> +        continue;
> +
> +    if (name in checked_options)
> +        continue;
> +    checked_options[name]++
> +
> +    print "  if (ptr1->x_" name " != ptr2->x_" name ")"
> +    print "  {"
> +    print "    if (result)"
> +    print "      fprintf (stderr, \"Error: global_options are modified in local context:\\n\");"
> +    print "    fprintf (stderr, \"  " name " (%ld/%ld)\\n\", (long int)ptr1->x_" name ", (long int)ptr2->x_" name ");"
> +    print "    result = false;"
> +    print "  }"
> +}
> +
> +print "  return result;"
> +print "}"
> +print "#endif"
> +print "#endif"
> +
>   # All of the optimization switches gathered together so they can be saved and restored.
>   # This will allow attribute((cold)) to turn on space optimization.
>
Jeff Law via Gcc-patches March 20, 2020, 3:43 p.m. UTC | #6
On Fri, Mar 20, 2020 at 04:40:17PM +0100, Martin Liška wrote:
> Thank you very much for the feedback, it was useful. I realized that
> the patch made a huge code bloat (mainly because of the string constants
> and the fact that it didn't end quickly (with an internal_error).
> 
> The loop is here not possible because we compare struct fields.

Are you sure?  I mean, you could have a loop over cl_options array which
contains the needed offsetof values and also the types.

	Jakub
Martin Liška March 20, 2020, 3:55 p.m. UTC | #7
On 3/20/20 4:43 PM, Jakub Jelinek wrote:
> On Fri, Mar 20, 2020 at 04:40:17PM +0100, Martin Liška wrote:
>> Thank you very much for the feedback, it was useful. I realized that
>> the patch made a huge code bloat (mainly because of the string constants
>> and the fact that it didn't end quickly (with an internal_error).
>>
>> The loop is here not possible because we compare struct fields.
> 
> Are you sure?  I mean, you could have a loop over cl_options array which
> contains the needed offsetof values and also the types.

Ok, it would be possible, but if you take a look at options-save.c there's no
function that will leverage that. It's a generated code so I guess we can
live with that?

Martin

> 
> 	Jakub
>
Martin Liška May 21, 2020, 12:04 p.m. UTC | #8
PING^1

On 3/20/20 4:55 PM, Martin Liška wrote:
> Ok, it would be possible, but if you take a look at options-save.c there's no
> function that will leverage that. It's a generated code so I guess we can
> live with that?
diff mbox series

Patch

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 9abf81d0248..c99b1256186 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -4448,6 +4448,7 @@  handle_optimize_attribute (tree *node, tree name, tree args,
 
       /* If we previously had some optimization options, use them as the
 	 default.  */
+      gcc_options saved_global_options = global_options;
       if (old_opts)
 	cl_optimization_restore (&global_options,
 				 TREE_OPTIMIZATION (old_opts));
@@ -4459,6 +4460,8 @@  handle_optimize_attribute (tree *node, tree name, tree args,
 
       /* Restore current options.  */
       cl_optimization_restore (&global_options, &cur_opts);
+      if (flag_checking)
+	gcc_assert (gcc_options_check (&saved_global_options, &global_options));
     }
 
   return NULL_TREE;
diff --git a/gcc/c-family/c-pragma.c b/gcc/c-family/c-pragma.c
index 7c35741745b..8b3b4f218ba 100644
--- a/gcc/c-family/c-pragma.c
+++ b/gcc/c-family/c-pragma.c
@@ -1003,6 +1003,7 @@  struct GTY(()) opt_stack {
   tree target_strings;
   tree optimize_binary;
   tree optimize_strings;
+  gcc_options saved_global_options;
 };
 
 static GTY(()) struct opt_stack * options_stack;
@@ -1028,6 +1029,7 @@  handle_pragma_push_options (cpp_reader *ARG_UNUSED(dummy))
   options_stack = p;
 
   /* Save optimization and target flags in binary format.  */
+  p->saved_global_options = global_options;
   p->optimize_binary = build_optimization_node (&global_options);
   p->target_binary = build_target_option_node (&global_options);
 
@@ -1079,6 +1081,8 @@  handle_pragma_pop_options (cpp_reader *ARG_UNUSED(dummy))
 				      p->optimize_binary);
       optimization_current_node = p->optimize_binary;
     }
+  if (flag_checking)
+    gcc_assert (gcc_options_check (&p->saved_global_options, &global_options));
 
   current_target_pragma = p->target_strings;
   current_optimize_pragma = p->optimize_strings;
diff --git a/gcc/opth-gen.awk b/gcc/opth-gen.awk
index 856a69168a5..586213da3d3 100644
--- a/gcc/opth-gen.awk
+++ b/gcc/opth-gen.awk
@@ -119,6 +119,41 @@  print "#endif"
 print "#endif"
 print ""
 
+print "#if !defined(IN_LIBGCC2) && !defined(IN_TARGET_LIBS) && !defined(IN_RTS)"
+print "#ifndef GENERATOR_FILE"
+print "static inline bool gcc_options_check (gcc_options *ptr1, gcc_options *ptr2)"
+print "{"
+print "  bool result = true;"
+
+# all these options are mentioned in PR92860
+checked_options["flag_merge_constants"]++
+checked_options["param_max_fields_for_field_sensitive"]++
+checked_options["flag_omit_frame_pointer"]++
+checked_options["unroll_only_small_loops"]++
+
+for (i = 0; i < n_opts; i++) {
+	name = var_name(flags[i]);
+	if (name == "")
+		continue;
+
+	if (name in checked_options)
+		continue;
+	checked_options[name]++
+
+	print "  if (ptr1->x_" name " != ptr2->x_" name ")"
+	print "  {"
+	print "    if (result)"
+	print "      fprintf (stderr, \"Error: global_options are modified in local context:\\n\");"
+	print "    fprintf (stderr, \"  " name " (%ld/%ld)\\n\", (long int)ptr1->x_" name ", (long int)ptr2->x_" name ");"
+	print "    result = false;"
+	print "  }"
+}
+
+print "  return result;"
+print "}"
+print "#endif"
+print "#endif"
+
 # All of the optimization switches gathered together so they can be saved and restored.
 # This will allow attribute((cold)) to turn on space optimization.