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.
>From 94f01ce8a662293dc6bc44a0e3047c5fe54fc15d Mon Sep 17 00:00:00 2001
From: Martin Liska <mli...@suse.cz>
Date: Tue, 10 Dec 2019 19:41:08 +0100
Subject: [PATCH] Add gcc_assert that &global_options are not dirty modified.
gcc/ChangeLog:
2020-03-20 Martin Liska <mli...@suse.cz>
PR tree-optimization/92860
* optc-save-gen.awk: Generate new function cl_optimization_compare.
* opth-gen.awk: Generate declaration of the function.
gcc/c-family/ChangeLog:
2020-03-20 Martin Liska <mli...@suse.cz>
PR tree-optimization/92860
* c-attribs.c (handle_optimize_attribute):
Save global options and compare it after parsing of function
attribute.
* c-pragma.c (opt_stack::saved_global_options): New field.
(handle_pragma_push_options): Save global_options.
(handle_pragma_pop_options): Compare them after pop.
---
gcc/c-family/c-attribs.c | 12 ++++++++++++
gcc/c-family/c-pragma.c | 11 +++++++++++
gcc/optc-save-gen.awk | 25 +++++++++++++++++++++++++
gcc/opth-gen.awk | 3 +++
4 files changed, 51 insertions(+)
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 9abf81d0248..095986dead4 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -4448,6 +4448,13 @@ 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 = NULL;
+ if (flag_checking)
+ {
+ saved_global_options = XNEW (gcc_options);
+ *saved_global_options = global_options;
+ }
+
if (old_opts)
cl_optimization_restore (&global_options,
TREE_OPTIMIZATION (old_opts));
@@ -4459,6 +4466,11 @@ handle_optimize_attribute (tree *node, tree name, tree args,
/* Restore current options. */
cl_optimization_restore (&global_options, &cur_opts);
+ if (saved_global_options != NULL)
+ {
+ cl_optimization_compare (saved_global_options, &global_options);
+ free (saved_global_options);
+ }
}
return NULL_TREE;
diff --git a/gcc/c-family/c-pragma.c b/gcc/c-family/c-pragma.c
index 7c35741745b..e3169e68fb6 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 * GTY ((skip)) saved_global_options;
};
static GTY(()) struct opt_stack * options_stack;
@@ -1028,6 +1029,11 @@ handle_pragma_push_options (cpp_reader *ARG_UNUSED(dummy))
options_stack = p;
/* Save optimization and target flags in binary format. */
+ if (flag_checking)
+ {
+ p->saved_global_options = XNEW (gcc_options);
+ *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 +1085,11 @@ handle_pragma_pop_options (cpp_reader *ARG_UNUSED(dummy))
p->optimize_binary);
optimization_current_node = p->optimize_binary;
}
+ if (flag_checking)
+ {
+ cl_optimization_compare (p->saved_global_options, &global_options);
+ free (p->saved_global_options);
+ }
current_target_pragma = p->target_strings;
current_optimize_pragma = p->optimize_strings;
diff --git a/gcc/optc-save-gen.awk b/gcc/optc-save-gen.awk
index 6033f519866..4a0e5ab64f3 100644
--- a/gcc/optc-save-gen.awk
+++ b/gcc/optc-save-gen.awk
@@ -945,5 +945,30 @@ for (i = 0; i < n_opt_val; i++) {
print " free (const_cast <char *>(ptr->" name"));";
}
}
+print "}";
+
+print "void";
+print "cl_optimization_compare (gcc_options *ptr1, gcc_options *ptr2)"
+print "{"
+
+# 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 " internal_error (\"Error: global_options are modified in local context\\n\");";
+}
+
print "}";
}
diff --git a/gcc/opth-gen.awk b/gcc/opth-gen.awk
index 856a69168a5..472fd591413 100644
--- a/gcc/opth-gen.awk
+++ b/gcc/opth-gen.awk
@@ -318,6 +318,9 @@ print "";
print "/* Free heap memory used by optimization options. */";
print "extern void cl_optimization_option_free (cl_optimization *ptr1);"
print "";
+print "/* Compare and report difference for a part of cl_optimization options. */";
+print "extern void cl_optimization_compare (gcc_options *ptr1, gcc_options *ptr2);";
+print "";
print "/* Generator files may not have access to location_t, and don't need these. */"
print "#if defined(UNKNOWN_LOCATION)"
print "bool "
--
2.25.1
