From: "Martin Liška" <mliska@suse.cz>
To: Martin Sebor <msebor@gmail.com>, Jakub Jelinek <jakub@redhat.com>
Cc: gcc-patches@gcc.gnu.org, Jan Hubicka <hubicka@ucw.cz>
Subject: Re: [stage1][PATCH] Add gcc_assert that &global_options are not dirty modified.
Date: Fri, 20 Mar 2020 16:40:17 +0100 [thread overview]
Message-ID: <63194d6e-cd33-8563-03fc-f2a86b4bf4ca@suse.cz> (raw)
In-Reply-To: <312fd3c4-cc7c-5389-4ae7-615aad0761f3@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4817 bytes --]
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.
>
[-- Attachment #2: 0001-Add-gcc_assert-that-global_options-are-not-dirty-mod.patch --]
[-- Type: text/x-patch, Size: 4962 bytes --]
From 94f01ce8a662293dc6bc44a0e3047c5fe54fc15d Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@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 <mliska@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 <mliska@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
next prev parent reply other threads:[~2020-03-20 15:40 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-19 8:56 Martin Liška
2020-03-19 9:09 ` Jakub Jelinek
2020-03-19 15:32 ` Martin Liška
2020-03-19 15:47 ` Jakub Jelinek
2020-03-19 16:13 ` Martin Sebor
2020-03-20 15:40 ` Martin Liška [this message]
2020-03-20 15:43 ` Jakub Jelinek
2020-03-20 15:55 ` Martin Liška
2020-05-21 12:04 ` Martin Liška
2020-06-09 21:01 ` Jeff Law
2020-06-18 11:32 ` Luis Machado
2020-06-18 12:21 ` Martin Liška
2020-06-18 15:02 ` Jeff Law
2020-06-18 15:36 ` Martin Liška
2020-06-24 7:44 ` Martin Liška
2020-06-18 15:40 ` Martin Liška
2020-06-18 15:47 ` Luis Machado
2020-06-18 16:02 ` Martin Liška
2020-06-18 17:18 ` Luis Machado
2020-06-18 18:34 ` Martin Liška
2020-06-18 21:24 ` Luis Machado
2020-06-24 7:25 ` Martin Liška
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=63194d6e-cd33-8563-03fc-f2a86b4bf4ca@suse.cz \
--to=mliska@suse.cz \
--cc=gcc-patches@gcc.gnu.org \
--cc=hubicka@ucw.cz \
--cc=jakub@redhat.com \
--cc=msebor@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).