| Message ID | da0c9072-3ba7-8a38-7206-4acc62d0482b@suse.cz |
|---|---|
| State | New |
| Headers | show |
On Fri, Jul 28, 2017 at 01:11:51PM +0200, Martin Liška wrote: > Hello. > > Following patch fixed race where we first need to properly build > all gimple_build_debug_bind (arg, var, NULL) and then DECL_HAS_VALUE_EXPR_P > can be registered. > > Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. > > Ready to be installed? > Martin > > gcc/ChangeLog: > > 2017-07-10 Martin Liska <mliska@suse.cz> > > PR sanitizer/81340 > * sanopt.c (sanitize_rewrite_addressable_params): Missing description of what you've changed. Also, it isn't immediately clear why this is needed. For force_gimple* etc. I'd understand that setting DECL_VALUE_EXPR prematurely will result in gimplification of that into the DECL_VALUE_EXPR, but gimple_build_assign or gimple_build_debug_bind shouldn't do that. Or does the former invoke match.pd and that cares about it? Jakub
On Fri, Jul 28, 2017 at 1:19 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Jul 28, 2017 at 01:11:51PM +0200, Martin Liška wrote: >> Hello. >> >> Following patch fixed race where we first need to properly build >> all gimple_build_debug_bind (arg, var, NULL) and then DECL_HAS_VALUE_EXPR_P >> can be registered. >> >> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >> >> Ready to be installed? >> Martin >> >> gcc/ChangeLog: >> >> 2017-07-10 Martin Liska <mliska@suse.cz> >> >> PR sanitizer/81340 >> * sanopt.c (sanitize_rewrite_addressable_params): > > Missing description of what you've changed. Also, it isn't immediately > clear why this is needed. For force_gimple* etc. I'd understand that > setting DECL_VALUE_EXPR prematurely will result in gimplification of that > into the DECL_VALUE_EXPR, but gimple_build_assign or gimple_build_debug_bind > shouldn't do that. Or does the former invoke match.pd and that cares about > it? I think target_for_debug_bind () gets confused. > Jakub
On 07/28/2017 01:32 PM, Richard Biener wrote: > On Fri, Jul 28, 2017 at 1:19 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Fri, Jul 28, 2017 at 01:11:51PM +0200, Martin Liška wrote: >>> Hello. >>> >>> Following patch fixed race where we first need to properly build >>> all gimple_build_debug_bind (arg, var, NULL) and then DECL_HAS_VALUE_EXPR_P >>> can be registered. >>> >>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >>> >>> Ready to be installed? >>> Martin >>> >>> gcc/ChangeLog: >>> >>> 2017-07-10 Martin Liska <mliska@suse.cz> >>> >>> PR sanitizer/81340 >>> * sanopt.c (sanitize_rewrite_addressable_params): >> >> Missing description of what you've changed. Also, it isn't immediately >> clear why this is needed. For force_gimple* etc. I'd understand that >> setting DECL_VALUE_EXPR prematurely will result in gimplification of that >> into the DECL_VALUE_EXPR, but gimple_build_assign or gimple_build_debug_bind >> shouldn't do that. Or does the former invoke match.pd and that cares about >> it? > > I think target_for_debug_bind () gets confused. Exactly: tree target_for_debug_bind (tree var) { ... if (DECL_HAS_VALUE_EXPR_P (var)) return target_for_debug_bind (DECL_VALUE_EXPR (var)); ... There's fixed changelog entry: Set VALUE_EXPR after a debug stmt is generated (PR sanitizer/81340). gcc/ChangeLog: 2017-07-10 Martin Liska <mliska@suse.cz> PR sanitizer/81340 * sanopt.c (sanitize_rewrite_addressable_params): Set VALUE_EXPR after gimple_build_debug_bind because it calls target_for_debug_bind and would confuse the function. gcc/testsuite/ChangeLog: 2017-07-10 Martin Liska <mliska@suse.cz> PR sanitizer/81340 * g++.dg/asan/pr81340.C: New test. Martin > >> Jakub
On Fri, Jul 28, 2017 at 01:36:54PM +0200, Martin Liška wrote: > > I think target_for_debug_bind () gets confused. > > Exactly: > > tree > target_for_debug_bind (tree var) > { > ... > > if (DECL_HAS_VALUE_EXPR_P (var)) > return target_for_debug_bind (DECL_VALUE_EXPR (var)); > ... > > There's fixed changelog entry: > > Set VALUE_EXPR after a debug stmt is generated (PR sanitizer/81340). > > gcc/ChangeLog: > > 2017-07-10 Martin Liska <mliska@suse.cz> > > PR sanitizer/81340 > * sanopt.c (sanitize_rewrite_addressable_params): > Set VALUE_EXPR after gimple_build_debug_bind because > it calls target_for_debug_bind and would confuse the function. s/VALUE_EXPR/DECL_VALUE_EXPR/ and at least the first two words should fit on the line just fine: * sanopt.c (sanitize_rewrite_addressable_params): Set DECL_VALUE_EXPR after gimple_build_debug_bind. The because doesn't belong to ChangeLog, if you want, you can put it into a comment in the source. > gcc/testsuite/ChangeLog: > > 2017-07-10 Martin Liska <mliska@suse.cz> > > PR sanitizer/81340 > * g++.dg/asan/pr81340.C: New test. Ok. Jakub
diff --git a/gcc/sanopt.c b/gcc/sanopt.c index b7740741d43..227f72fcdd9 100644 --- a/gcc/sanopt.c +++ b/gcc/sanopt.c @@ -915,8 +915,6 @@ sanitize_rewrite_addressable_params (function *fun) IDENTIFIER_POINTER (DECL_NAME (arg))); gcc_assert (!DECL_HAS_VALUE_EXPR_P (arg)); - DECL_HAS_VALUE_EXPR_P (arg) = 1; - SET_DECL_VALUE_EXPR (arg, var); SET_DECL_PT_UID (var, DECL_PT_UID (arg)); @@ -945,6 +943,9 @@ sanitize_rewrite_addressable_params (function *fun) gimple_seq_add_stmt (&stmts, g); clear_value_expr_list.safe_push (arg); } + + DECL_HAS_VALUE_EXPR_P (arg) = 1; + SET_DECL_VALUE_EXPR (arg, var); } } diff --git a/gcc/testsuite/g++.dg/asan/pr81340.C b/gcc/testsuite/g++.dg/asan/pr81340.C new file mode 100644 index 00000000000..76ac08a9a56 --- /dev/null +++ b/gcc/testsuite/g++.dg/asan/pr81340.C @@ -0,0 +1,22 @@ +// { dg-options "-fsanitize=address -O2 -g -Wno-write-strings" } + +class a { + struct b { + b(int, int); + } c; + +public: + int d; + a(char *) : c(0, d) {} +}; +class e { + int f(const int &, const int &, const int &, bool, bool, bool, int, bool); +}; +class g { +public: + static g *h(); + void i(a, void *); +}; +int e::f(const int &, const int &, const int &, bool j, bool, bool, int, bool) { + g::h()->i("", &j); +}