| Message ID | 1518192504-49084-1-git-send-email-bonzini@gnu.org |
|---|---|
| State | New |
| Headers | show |
| Series | Improve dead code elimination with -fsanitize=address (PR84307) | expand |
On February 9, 2018 5:08:24 PM GMT+01:00, Paolo Bonzini <bonzini@gnu.org> wrote: >Hi all, > >in this PR, a dead reference to a function pointer cannot be optimized >out by the compiler because some ASAN_MARK UNPOISON calls, which are >placed before the store, cause the containing struct to escape. >(Without -fsanitize=address, the dead code is eliminated by the first >DSE pass). > >The fix, which works at least for this testcase, is to copy part of the >sanopt dead code elimination pass early, so that the compiler can see >fewer UNPOISON calls. I am not sure this is general enough, due to >the very limited data flow analysis done by >sanitize_asan_mark_unpoison. >Another possibility which I considered but did not implement is to mark >the UNPOISON calls so that they do not cause the parameter to escape. I'd do this, thus assign proper fnspec attributes to the asan functions. Richard. >Any suggestions on how to proceed here (or approval is welcome too :))? > >Paolo > >2018-02-09 Paolo Bonzini <bonzini@gnu.org> > > * passes.def: add pass_sandce before first DSE pass. > * sanopt.c (pass_data_sandce, pass_sandce, make_pass_sandce): New. > * tree-pass.h (make_pass_sandce): Declare it. > >testsuite: >2018-02-09 Paolo Bonzini <bonzini@gnu.org> > > PR sanitizer/84307 > * gcc.dg/asan/pr84307.c: New test. > >diff --git a/gcc/passes.def b/gcc/passes.def >index 9802f08..180df50 100644 >--- a/gcc/passes.def >+++ b/gcc/passes.def >@@ -244,6 +244,7 @@ along with GCC; see the file COPYING3. If not see > only examines PHIs to discover const/copy propagation > opportunities. */ > NEXT_PASS (pass_phi_only_cprop); >+ NEXT_PASS (pass_sandce); > NEXT_PASS (pass_dse); > NEXT_PASS (pass_reassoc, true /* insert_powi_p */); > NEXT_PASS (pass_dce); >diff --git a/gcc/sanopt.c b/gcc/sanopt.c >index cd94638..23b8e6e 100644 >--- a/gcc/sanopt.c >+++ b/gcc/sanopt.c >@@ -906,6 +906,32 @@ sanopt_optimize (function *fun, bool >*contains_asan_mark) > > namespace { > >+const pass_data pass_data_sandce = >+{ >+ GIMPLE_PASS, /* type */ >+ "sandce", /* name */ >+ OPTGROUP_NONE, /* optinfo_flags */ >+ TV_NONE, /* tv_id */ >+ ( PROP_ssa | PROP_cfg | PROP_gimple_leh ), /* properties_required */ >+ 0, /* properties_provided */ >+ 0, /* properties_destroyed */ >+ 0, /* todo_flags_start */ >+ TODO_update_ssa, /* todo_flags_finish */ >+}; >+ >+class pass_sandce : public gimple_opt_pass >+{ >+public: >+ pass_sandce (gcc::context *ctxt) >+ : gimple_opt_pass (pass_data_sandce, ctxt) >+ {} >+ >+ /* opt_pass methods: */ >+ virtual bool gate (function *) { return flag_sanitize & >SANITIZE_ADDRESS; } >+ virtual unsigned int execute (function *); >+ >+}; // class pass_sanopt >+ > const pass_data pass_data_sanopt = > { > GIMPLE_PASS, /* type */ >@@ -1244,6 +1270,31 @@ sanitize_rewrite_addressable_params (function >*fun) > } > > unsigned int >+pass_sandce::execute (function *fun) >+{ >+ basic_block bb; >+ bool contains_asan_mark = false; >+ >+ /* Try to remove redundant unpoisoning. */ >+ gimple_stmt_iterator gsi; >+ FOR_EACH_BB_FN (bb, fun) >+ for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) >+ { >+ gimple *stmt = gsi_stmt (gsi); >+ if (gimple_call_internal_p (stmt, IFN_ASAN_MARK)) >+ { >+ contains_asan_mark = true; >+ break; >+ } >+ } >+ >+ if (contains_asan_mark) >+ sanitize_asan_mark_unpoison (); >+ >+ return 0; >+} >+ >+unsigned int > pass_sanopt::execute (function *fun) > { > basic_block bb; >@@ -1367,6 +1418,12 @@ pass_sanopt::execute (function *fun) > } // anon namespace > > gimple_opt_pass * >+make_pass_sandce (gcc::context *ctxt) >+{ >+ return new pass_sandce (ctxt); >+} >+ >+gimple_opt_pass * > make_pass_sanopt (gcc::context *ctxt) > { > return new pass_sanopt (ctxt); >diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h >index 93a6a99..d5eb055 100644 >--- a/gcc/tree-pass.h >+++ b/gcc/tree-pass.h >@@ -350,6 +350,7 @@ extern gimple_opt_pass *make_pass_tsan >(gcc::context *ctxt); > extern gimple_opt_pass *make_pass_tsan_O0 (gcc::context *ctxt); > extern gimple_opt_pass *make_pass_sancov (gcc::context *ctxt); > extern gimple_opt_pass *make_pass_sancov_O0 (gcc::context *ctxt); >+extern gimple_opt_pass *make_pass_sandce (gcc::context *ctxt); > extern gimple_opt_pass *make_pass_lower_cf (gcc::context *ctxt); > extern gimple_opt_pass *make_pass_refactor_eh (gcc::context *ctxt); > extern gimple_opt_pass *make_pass_lower_eh (gcc::context *ctxt); >diff --git a/gcc/testsuite/gcc.dg/asan/pr84307.c >b/gcc/testsuite/gcc.dg/asan/pr84307.c >new file mode 100644 >index 0000000..6e1a197 >--- /dev/null >+++ b/gcc/testsuite/gcc.dg/asan/pr84307.c >@@ -0,0 +1,21 @@ >+/* PR middle-end/83185 */ >+/* { dg-do link } */ >+/* { dg-options "-O1" } */ >+ >+struct f { >+ void (*func)(void); >+}; >+ >+extern void link_error(void); >+extern int printf(const char *f, ...); >+ >+static inline struct f *gimme_null(struct f *result) >+{ >+ return 0; >+} >+ >+int main(int argc, char **argv) >+{ >+ struct f *x = gimme_null(&(struct f) { .func = link_error }); >+ printf("%p", x); >+}
On Fri, Feb 09, 2018 at 05:40:09PM +0100, Richard Biener wrote: > On February 9, 2018 5:08:24 PM GMT+01:00, Paolo Bonzini <bonzini@gnu.org> wrote: > >Hi all, > > > >in this PR, a dead reference to a function pointer cannot be optimized > >out by the compiler because some ASAN_MARK UNPOISON calls, which are > >placed before the store, cause the containing struct to escape. > >(Without -fsanitize=address, the dead code is eliminated by the first > >DSE pass). > > > >The fix, which works at least for this testcase, is to copy part of the > >sanopt dead code elimination pass early, so that the compiler can see > >fewer UNPOISON calls. I am not sure this is general enough, due to > >the very limited data flow analysis done by > >sanitize_asan_mark_unpoison. > >Another possibility which I considered but did not implement is to mark > >the UNPOISON calls so that they do not cause the parameter to escape. > > I'd do this, thus assign proper fnspec attributes to the asan functions. It already uses ".R.." "fn spec". Jakub
On 09/02/2018 17:40, Richard Biener wrote: > On February 9, 2018 5:08:24 PM GMT+01:00, Paolo Bonzini <bonzini@gnu.org> wrote: >> Another possibility which I considered but did not implement is to mark >> the UNPOISON calls so that they do not cause the parameter to escape. > > I'd do this, thus assign proper fnspec attributes to the asan functions. Hmm, actually that might be as simple as fixing a typo: diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def index 5970d0e..15d6151 100644 --- a/gcc/internal-fn.def +++ b/gcc/internal-fn.def @@ -255,8 +255,8 @@ DEF_INTERNAL_FN (UBSAN_PTR, ECF_LEAF | ECF_NOTHROW, ".R.") DEF_INTERNAL_FN (UBSAN_OBJECT_SIZE, ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, ECF_NORETURN, NULL) DEF_INTERNAL_FN (BUILTIN_EXPECT, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) -DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, ".R...") -DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, ".R..") +DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, "..R..") +DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, "..W.") DEF_INTERNAL_FN (ASAN_POISON, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL) DEF_INTERNAL_FN (ASAN_POISON_USE, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL) DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) which indeed fixes the testcase and seems not to break asan.exp. 'W' is needed to avoid breaking the pr78541.c testcase, and I think it makes sense since ASAN_MARK is "writing" the state of the object (in the test case FRE moves a dereference across a poisoning). I'll look at it next week. Someone maybe should take a look at ubsan fnspecs too. Paolo
On February 9, 2018 6:23:37 PM GMT+01:00, Paolo Bonzini <bonzini@gnu.org> wrote: >On 09/02/2018 17:40, Richard Biener wrote: >> On February 9, 2018 5:08:24 PM GMT+01:00, Paolo Bonzini ><bonzini@gnu.org> wrote: >>> Another possibility which I considered but did not implement is to >mark >>> the UNPOISON calls so that they do not cause the parameter to >escape. >> >> I'd do this, thus assign proper fnspec attributes to the asan >functions. > >Hmm, actually that might be as simple as fixing a typo: > >diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def >index 5970d0e..15d6151 100644 >--- a/gcc/internal-fn.def >+++ b/gcc/internal-fn.def >@@ -255,8 +255,8 @@ DEF_INTERNAL_FN (UBSAN_PTR, ECF_LEAF | ECF_NOTHROW, >".R.") > DEF_INTERNAL_FN (UBSAN_OBJECT_SIZE, ECF_LEAF | ECF_NOTHROW, NULL) > DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, ECF_NORETURN, NULL) >DEF_INTERNAL_FN (BUILTIN_EXPECT, ECF_CONST | ECF_LEAF | ECF_NOTHROW, >NULL) >-DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, >".R...") >-DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, ".R..") >+DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, >"..R..") >+DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, "..W.") >DEF_INTERNAL_FN (ASAN_POISON, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, >NULL) >DEF_INTERNAL_FN (ASAN_POISON_USE, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, >NULL) >DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, >NULL) > >which indeed fixes the testcase and seems not to break asan.exp. Huh. Need to double check why that makes sense ;) >'W' is needed to avoid breaking the pr78541.c testcase, and I think it >makes sense since ASAN_MARK is "writing" the state of the object >(in the test case FRE moves a dereference across a poisoning). > >I'll look at it next week. Someone maybe should take a look at ubsan >fnspecs too. > >Paolo
On Fri, Feb 09, 2018 at 07:01:08PM +0100, Richard Biener wrote: > >which indeed fixes the testcase and seems not to break asan.exp. > > Huh. Need to double check why that makes sense ;) I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument is the second one, the first one is an integer argument with flags. And ASAN_MARK, both poison and unpoison, works kind like a clobber on the referenced variable, before unpoison it is generally inaccessible and after poison too. Jakub
On February 9, 2018 7:07:45 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote: >On Fri, Feb 09, 2018 at 07:01:08PM +0100, Richard Biener wrote: >> >which indeed fixes the testcase and seems not to break asan.exp. >> >> Huh. Need to double check why that makes sense ;) > >I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument >is the second one, the first one is an integer argument with flags. >And ASAN_MARK, both poison and unpoison, works kind like a clobber on >the >referenced variable, before unpoison it is generally inaccessible and >after >poison too. Ah, indeed. Richard. > Jakub
On Fri, Feb 9, 2018 at 9:10 PM, Richard Biener <richard.guenther@gmail.com> wrote: > On February 9, 2018 7:07:45 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote: >>On Fri, Feb 09, 2018 at 07:01:08PM +0100, Richard Biener wrote: >>> >which indeed fixes the testcase and seems not to break asan.exp. >>> >>> Huh. Need to double check why that makes sense ;) >> >>I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument >>is the second one, the first one is an integer argument with flags. >>And ASAN_MARK, both poison and unpoison, works kind like a clobber on >>the >>referenced variable, before unpoison it is generally inaccessible and >>after >>poison too. > > Ah, indeed. Which was an approval as well, in case you want to push this right now. Richard. > Richard. > >> Jakub >
On 12/02/2018 09:56, Richard Biener wrote: >>> I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument >>> is the second one, the first one is an integer argument with flags. >>> And ASAN_MARK, both poison and unpoison, works kind like a clobber on >>> the >>> referenced variable, before unpoison it is generally inaccessible and >>> after >>> poison too. >> Ah, indeed. > Which was an approval as well, in case you want to push this right now. Oh cool. I was going to look at ubsan builtins too, I'll post that separately. Ok for GCC 7 too? Thanks, Paolo
On Mon, Feb 12, 2018 at 01:02:20PM +0100, Paolo Bonzini wrote: > On 12/02/2018 09:56, Richard Biener wrote: > >>> I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument > >>> is the second one, the first one is an integer argument with flags. > >>> And ASAN_MARK, both poison and unpoison, works kind like a clobber on > >>> the > >>> referenced variable, before unpoison it is generally inaccessible and > >>> after > >>> poison too. > >> Ah, indeed. > > Which was an approval as well, in case you want to push this right now. > > Oh cool. I was going to look at ubsan builtins too, I'll post that > separately. Ok for GCC 7 too? Please wait with GCC 7 backport at least 2 weeks after it is committed to trunk. Jakub
On 09/02/2018 19:07, Jakub Jelinek wrote: > On Fri, Feb 09, 2018 at 07:01:08PM +0100, Richard Biener wrote: >>> which indeed fixes the testcase and seems not to break asan.exp. >> >> Huh. Need to double check why that makes sense ;) > > I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument > is the second one, the first one is an integer argument with flags. > And ASAN_MARK, both poison and unpoison, works kind like a clobber on the > referenced variable, before unpoison it is generally inaccessible and after > poison too. This was too optimistic. :( In use-after-scope-types-1.C, after the patch FRE+DSE are able to optimize away the problematic read. In general it seems to me that the sanitizer passes should be before DSE if we want ASAN builtins to have precise info, otherwise some reads or stores might not be instrumented---GCC was being lucky here. The obvious change here is: Index: passes.def =================================================================== --- passes.def (revision 257584) +++ passes.def (working copy) @@ -95,6 +95,9 @@ NEXT_PASS (pass_fre); NEXT_PASS (pass_early_vrp); NEXT_PASS (pass_merge_phi); + NEXT_PASS (pass_sancov); + NEXT_PASS (pass_asan); + NEXT_PASS (pass_tsan); NEXT_PASS (pass_dse); NEXT_PASS (pass_cd_dce); NEXT_PASS (pass_early_ipa_sra); @@ -259,9 +262,6 @@ NEXT_PASS (pass_walloca, false); NEXT_PASS (pass_pre); NEXT_PASS (pass_sink_code); - NEXT_PASS (pass_sancov); - NEXT_PASS (pass_asan); - NEXT_PASS (pass_tsan); NEXT_PASS (pass_dce); /* Pass group that runs when 1) enabled, 2) there are loops in the function. Make sure to run pass_fix_loops before which seems to work (this time for real... not sure what went wrong in my previous testing) but it's a pretty large change that I'd like to run by you guys before posting it. Paolo
On Tue, Feb 13, 2018 at 12:41:28AM +0100, Paolo Bonzini wrote: > On 09/02/2018 19:07, Jakub Jelinek wrote: > > On Fri, Feb 09, 2018 at 07:01:08PM +0100, Richard Biener wrote: > >>> which indeed fixes the testcase and seems not to break asan.exp. > >> > >> Huh. Need to double check why that makes sense ;) > > > > I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument > > is the second one, the first one is an integer argument with flags. > > And ASAN_MARK, both poison and unpoison, works kind like a clobber on the > > referenced variable, before unpoison it is generally inaccessible and after > > poison too. > > This was too optimistic. :( > > In use-after-scope-types-1.C, after the patch FRE+DSE are able to > optimize away the problematic read. In general it seems to me that the > sanitizer passes should be before DSE if we want ASAN builtins to have > precise info, otherwise some reads or stores might not be > instrumented---GCC was being lucky here. > > The obvious change here is: That is way too early, I don't think this is a good idea. Certainly not in stage4. Jakub
Hello. It caused PR84340, I'm suggesting following fix: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84340#c3 Does it make sense? Martin
On 13/02/2018 10:32, Martin Liška wrote: > Hello. > > It caused PR84340, I'm suggesting following fix: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84340#c3 I don't think EAF_DIRECT is the issue. You could think of ASAN_MARK as writing a global variable, which it can do because it's not const. The issue is that the ASAN_CHECK doesn't exist at early DSE time, and thus causes the store to disappear. Paolo
On Tue, Feb 13, 2018 at 12:15:36PM +0100, Paolo Bonzini wrote: > On 13/02/2018 10:32, Martin Liška wrote: > > Hello. > > > > It caused PR84340, I'm suggesting following fix: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84340#c3 > > I don't think EAF_DIRECT is the issue. You could think of ASAN_MARK as > writing a global variable, which it can do because it's not const. > > The issue is that the ASAN_CHECK doesn't exist at early DSE time, and > thus causes the store to disappear. That doesn't make sense to me, because the testcases regressed with the change of "fn spec" attribute on ASAN_{CHECK,MARK}. If it was DSE removing the stores before asan pass, then it would FAIL before as well. Jakub
On 13/02/2018 00:49, Jakub Jelinek wrote: > On Tue, Feb 13, 2018 at 12:41:28AM +0100, Paolo Bonzini wrote: >> On 09/02/2018 19:07, Jakub Jelinek wrote: >>> On Fri, Feb 09, 2018 at 07:01:08PM +0100, Richard Biener wrote: >>>>> which indeed fixes the testcase and seems not to break asan.exp. >>>> >>>> Huh. Need to double check why that makes sense ;) >>> >>> I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument >>> is the second one, the first one is an integer argument with flags. >>> And ASAN_MARK, both poison and unpoison, works kind like a clobber on the >>> referenced variable, before unpoison it is generally inaccessible and after >>> poison too. >> >> This was too optimistic. :( >> >> In use-after-scope-types-1.C, after the patch FRE+DSE are able to >> optimize away the problematic read. In general it seems to me that the >> sanitizer passes should be before DSE if we want ASAN builtins to have >> precise info, otherwise some reads or stores might not be >> instrumented---GCC was being lucky here. >> >> The obvious change here is: > > That is way too early, I don't think this is a good idea. > Certainly not in stage4. Certainly, for now I'll revert. But can you expand on why it's too early? Indeed I suppose it may affect inlining decisions, on the other hand it seems dangerous to apply instrumentation after pretty much any optimization pass. Thanks, Paolo
On Tue, Feb 13, 2018 at 01:55:23PM +0100, Paolo Bonzini wrote: > On 13/02/2018 00:49, Jakub Jelinek wrote: > > On Tue, Feb 13, 2018 at 12:41:28AM +0100, Paolo Bonzini wrote: > >> On 09/02/2018 19:07, Jakub Jelinek wrote: > >>> On Fri, Feb 09, 2018 at 07:01:08PM +0100, Richard Biener wrote: > >>>>> which indeed fixes the testcase and seems not to break asan.exp. > >>>> > >>>> Huh. Need to double check why that makes sense ;) > >>> > >>> I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument > >>> is the second one, the first one is an integer argument with flags. > >>> And ASAN_MARK, both poison and unpoison, works kind like a clobber on the > >>> referenced variable, before unpoison it is generally inaccessible and after > >>> poison too. > >> > >> This was too optimistic. :( > >> > >> In use-after-scope-types-1.C, after the patch FRE+DSE are able to > >> optimize away the problematic read. In general it seems to me that the > >> sanitizer passes should be before DSE if we want ASAN builtins to have > >> precise info, otherwise some reads or stores might not be > >> instrumented---GCC was being lucky here. > >> > >> The obvious change here is: > > > > That is way too early, I don't think this is a good idea. > > Certainly not in stage4. > > Certainly, for now I'll revert. Reversion is not the right thing, the "fn spec" attributes were clearly incorrect. So, we should change them to something more conservative that will work. > But can you expand on why it's too early? Indeed I suppose it may > affect inlining decisions, on the other hand it seems dangerous to apply > instrumentation after pretty much any optimization pass. It will prevent pretty much all optimizations. We don't want -O2 -fsanitize=address to be unusably slow, if people want to catch everything, they can always use -O0 -fsanitize=address. The current placement of the passes has been a result of long discussions if I remember well. Jakub
On Tue, Feb 13, 2018 at 12:21:55PM +0100, Jakub Jelinek wrote: > On Tue, Feb 13, 2018 at 12:15:36PM +0100, Paolo Bonzini wrote: > > On 13/02/2018 10:32, Martin Liška wrote: > > > Hello. > > > > > > It caused PR84340, I'm suggesting following fix: > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84340#c3 > > > > I don't think EAF_DIRECT is the issue. You could think of ASAN_MARK as > > writing a global variable, which it can do because it's not const. > > > > The issue is that the ASAN_CHECK doesn't exist at early DSE time, and > > thus causes the store to disappear. > > That doesn't make sense to me, because the testcases regressed with the change > of "fn spec" attribute on ASAN_{CHECK,MARK}. > If it was DSE removing the stores before asan pass, then it would FAIL > before as well. Sorry, while ASAN_CHECK is introduced late, ASAN_MARK is present there already from the gimplification. For use after scope, I guess a lot of the stores after end of scope (rather than reads) are something DSE could consider removing. So, shall we just disable DSE on vars where their address "escapes" through ASAN_MARK when -fsanitize-address-use-after-scope? Generally, dead stores could be eliminable when stored before the corresponding ASAN_MARK poison (but even ASAN_MARK with "..W.." will prevent those) and uneliminable when stored after ASAN_MARK poison. For the "fn spec" for now, I'd just go with "..R.." for ASAN_CHECK and NULL for ASAN_MARK for now. Jakub
On 13/02/2018 14:00, Jakub Jelinek wrote: >> Certainly, for now I'll revert. > Reversion is not the right thing, the "fn spec" attributes were clearly > incorrect. So, we should change them to something more conservative that > will work. That would only be "all dots", that is no fnspec at all. Martin suggested removing EAF_DIRECT, but I don't think I agree with his reasoning. Besides, aliasing doesn't see the shadow memory at all (see call_may_clobber_ref_p_1), so it's okay to ignore it for the sake of fnspecs. >> But can you expand on why it's too early? Indeed I suppose it may >> affect inlining decisions, on the other hand it seems dangerous to apply >> instrumentation after pretty much any optimization pass. > > It will prevent pretty much all optimizations. We don't want -O2 > -fsanitize=address to be unusably slow, if people want to catch everything, > they can always use -O0 -fsanitize=address. The current placement of the > passes has been a result of long discussions if I remember well. I'm not sure it will be that bad, together with the fnspec. Consider that PR84340 is latent in current GCC; the testcases work because GCC thinks that the &x pointer escaped, and thus treated the stores as not dead. In other words, -fsanitize=address -O2 _currently_ lacks an awful lot of aliasing-based optimizations such as DSE, because all variables are marked as escaping after the initial ASAN_MARK(UNPOISON, &var, sz). With some luck (that we can ascertain between now and stage 1) the negative effects of pass placement balance with the positive effects of the fnspec. But I agree that it requires some discussion and benchmarking. Paolo
On Fri, Feb 09, 2018 at 05:08:24PM +0100, Paolo Bonzini wrote: > PR sanitizer/84307 > * gcc.dg/asan/pr84307.c: New test. BTW, your testcase shows a more severe problem, that we actually don't handle compound literals correctly. C99 says that: "If the compound literal occurs outside the body of a function, the object has static storage duration; otherwise, it has automatic storage duration associated with the enclosing block." but if we create an object with automatic storage duration, we don't actually put that object into the scope of the enclosing block, but of the enclosing function, which explains the weird ASAN_MARK UNPOISON present, but corresponding ASAN_MARK POISON not present. The following testcase should IMHO FAIL with -fsanitize=address on the second bar call, but doesn't, even at -O0 without any DSE. When optimizing we because of this don't emit CLOBBER stmts when the compound literal object goes out of scope, and with -fsanitize=address -fsanitize-address-use-after-scope we don't emit the POISON. struct S { int s; } *p; static inline void foo (struct S *x) { p = x; } static inline void bar (void) { p->s = 5; } int main () { { foo (&(struct S) { 1 }); bar (); } { foo (&(struct S) { 2 }); } bar (); return 0; } The following untested patch seems to cure thatm will see how much it will break. 2018-02-13 Jakub Jelinek <jakub@redhat.com> PR sanitizer/84340 * c-decl.c (build_compound_literal): Call pushdecl (decl) even when it is not TREE_STATIC. --- gcc/c/c-decl.c.jj 2018-01-03 10:20:20.114537949 +0100 +++ gcc/c/c-decl.c 2018-02-13 15:17:47.091186077 +0100 @@ -5348,6 +5348,8 @@ build_compound_literal (location_t loc, pushdecl (decl); rest_of_decl_compilation (decl, 1, 0); } + else + pushdecl (decl); if (non_const) { Jakub
On 13/02/2018 14:35, Jakub Jelinek wrote: > On Tue, Feb 13, 2018 at 12:21:55PM +0100, Jakub Jelinek wrote: >> On Tue, Feb 13, 2018 at 12:15:36PM +0100, Paolo Bonzini wrote: >>> The issue is that the ASAN_CHECK doesn't exist at early DSE time, and >>> thus causes the store to disappear. >> >> If it was DSE removing the stores before asan pass, then it would FAIL >> before as well. > > Sorry, while ASAN_CHECK is introduced late, ASAN_MARK is present there > already from the gimplification. Yeah, and it keeps everything alive. > For use after scope, I guess a lot of the stores after end of scope > (rather than reads) are something DSE could consider removing. > So, shall we just disable DSE on vars where their address "escapes" > through ASAN_MARK when -fsanitize-address-use-after-scope? But the stores _are_ dead; it's only the ASAN_CHECK that isn't. Hence the idea of doing the entire instrumentation very early. > Generally, dead stores could be eliminable when stored before the > corresponding ASAN_MARK poison (but even ASAN_MARK with "..W.." will > prevent those) and uneliminable when stored after ASAN_MARK poison. > > For the "fn spec" for now, I'd just go with "..R.." for ASAN_CHECK and > NULL for ASAN_MARK for now. I'm a bit scared of that even, :) especially in stage4. If you think it's safe enough, I can give it a shot, but honestly I wouldn't have much time to deal with the fallout now (hence the quick revert). Paolo
On 02/13/2018 04:22 PM, Paolo Bonzini wrote: > On 13/02/2018 14:35, Jakub Jelinek wrote: >> On Tue, Feb 13, 2018 at 12:21:55PM +0100, Jakub Jelinek wrote: >>> On Tue, Feb 13, 2018 at 12:15:36PM +0100, Paolo Bonzini wrote: >>>> The issue is that the ASAN_CHECK doesn't exist at early DSE time, and >>>> thus causes the store to disappear. >>> >>> If it was DSE removing the stores before asan pass, then it would FAIL >>> before as well. >> >> Sorry, while ASAN_CHECK is introduced late, ASAN_MARK is present there >> already from the gimplification. > > Yeah, and it keeps everything alive. > >> For use after scope, I guess a lot of the stores after end of scope >> (rather than reads) are something DSE could consider removing. >> So, shall we just disable DSE on vars where their address "escapes" >> through ASAN_MARK when -fsanitize-address-use-after-scope? > > But the stores _are_ dead; it's only the ASAN_CHECK that isn't. Hence > the idea of doing the entire instrumentation very early. > >> Generally, dead stores could be eliminable when stored before the >> corresponding ASAN_MARK poison (but even ASAN_MARK with "..W.." will >> prevent those) and uneliminable when stored after ASAN_MARK poison. >> >> For the "fn spec" for now, I'd just go with "..R.." for ASAN_CHECK and >> NULL for ASAN_MARK for now. > > I'm a bit scared of that even, :) especially in stage4. If you think > it's safe enough, I can give it a shot, but honestly I wouldn't have > much time to deal with the fallout now (hence the quick revert). Let me do it tomorrow. Martin > > Paolo >
Hi.
I'm sending patch that does what Jakub suggested.
Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
I consider the previous email as approval and I'm going to install the patch.
Thanks,
Martin
From b306ad522ed0f8a010647d0eb1d9c36b102e92c3 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 15 Feb 2018 14:41:46 +0100
Subject: [PATCH] Set proper internal functions fnspec (PR sanitizer/84307).
gcc/ChangeLog:
2018-02-16 Martin Liska <mliska@suse.cz>
PR sanitizer/84307
* internal-fn.def (ASAN_CHECK): Set proper flags.
(ASAN_MARK): Likewise.
---
gcc/internal-fn.def | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def
index 5970d0e472c..4080e1698ea 100644
--- a/gcc/internal-fn.def
+++ b/gcc/internal-fn.def
@@ -255,8 +255,8 @@ DEF_INTERNAL_FN (UBSAN_PTR, ECF_LEAF | ECF_NOTHROW, ".R.")
DEF_INTERNAL_FN (UBSAN_OBJECT_SIZE, ECF_LEAF | ECF_NOTHROW, NULL)
DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, ECF_NORETURN, NULL)
DEF_INTERNAL_FN (BUILTIN_EXPECT, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
-DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, ".R...")
-DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, ".R..")
+DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, "..R..")
+DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, NULL)
DEF_INTERNAL_FN (ASAN_POISON, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL)
DEF_INTERNAL_FN (ASAN_POISON_USE, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL)
DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
On Fri, Feb 16, 2018 at 10:53:58AM +0100, Martin Liška wrote: > Hi. > > I'm sending patch that does what Jakub suggested. > Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. > I consider the previous email as approval and I'm going to install the patch. Ok, thanks. > >From b306ad522ed0f8a010647d0eb1d9c36b102e92c3 Mon Sep 17 00:00:00 2001 > From: marxin <mliska@suse.cz> > Date: Thu, 15 Feb 2018 14:41:46 +0100 > Subject: [PATCH] Set proper internal functions fnspec (PR sanitizer/84307). > > gcc/ChangeLog: > > 2018-02-16 Martin Liska <mliska@suse.cz> > > PR sanitizer/84307 > * internal-fn.def (ASAN_CHECK): Set proper flags. > (ASAN_MARK): Likewise. > --- > gcc/internal-fn.def | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def > index 5970d0e472c..4080e1698ea 100644 > --- a/gcc/internal-fn.def > +++ b/gcc/internal-fn.def > @@ -255,8 +255,8 @@ DEF_INTERNAL_FN (UBSAN_PTR, ECF_LEAF | ECF_NOTHROW, ".R.") > DEF_INTERNAL_FN (UBSAN_OBJECT_SIZE, ECF_LEAF | ECF_NOTHROW, NULL) > DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, ECF_NORETURN, NULL) > DEF_INTERNAL_FN (BUILTIN_EXPECT, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > -DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, ".R...") > -DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, ".R..") > +DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, "..R..") > +DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, NULL) > DEF_INTERNAL_FN (ASAN_POISON, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL) > DEF_INTERNAL_FN (ASAN_POISON_USE, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL) > DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > -- > 2.16.1 > Jakub
On 02/16/2018 10:55 AM, Jakub Jelinek wrote: > On Fri, Feb 16, 2018 at 10:53:58AM +0100, Martin Liška wrote: >> Hi. >> >> I'm sending patch that does what Jakub suggested. >> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >> I consider the previous email as approval and I'm going to install the patch. > > Ok, thanks. > >> >From b306ad522ed0f8a010647d0eb1d9c36b102e92c3 Mon Sep 17 00:00:00 2001 >> From: marxin <mliska@suse.cz> >> Date: Thu, 15 Feb 2018 14:41:46 +0100 >> Subject: [PATCH] Set proper internal functions fnspec (PR sanitizer/84307). >> >> gcc/ChangeLog: >> >> 2018-02-16 Martin Liska <mliska@suse.cz> >> >> PR sanitizer/84307 >> * internal-fn.def (ASAN_CHECK): Set proper flags. >> (ASAN_MARK): Likewise. >> --- >> gcc/internal-fn.def | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def >> index 5970d0e472c..4080e1698ea 100644 >> --- a/gcc/internal-fn.def >> +++ b/gcc/internal-fn.def >> @@ -255,8 +255,8 @@ DEF_INTERNAL_FN (UBSAN_PTR, ECF_LEAF | ECF_NOTHROW, ".R.") >> DEF_INTERNAL_FN (UBSAN_OBJECT_SIZE, ECF_LEAF | ECF_NOTHROW, NULL) >> DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, ECF_NORETURN, NULL) >> DEF_INTERNAL_FN (BUILTIN_EXPECT, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) >> -DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, ".R...") >> -DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, ".R..") >> +DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, "..R..") >> +DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, NULL) >> DEF_INTERNAL_FN (ASAN_POISON, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL) >> DEF_INTERNAL_FN (ASAN_POISON_USE, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL) >> DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) >> -- >> 2.16.1 >> > > > Jakub > Btw. do we want such patch to be backported? Martin
On Fri, Feb 16, 2018 at 11:58:07AM +0100, Martin Liška wrote:
> Btw. do we want such patch to be backported?
No.
Jakub
diff --git a/gcc/passes.def b/gcc/passes.def index 9802f08..180df50 100644 --- a/gcc/passes.def +++ b/gcc/passes.def @@ -244,6 +244,7 @@ along with GCC; see the file COPYING3. If not see only examines PHIs to discover const/copy propagation opportunities. */ NEXT_PASS (pass_phi_only_cprop); + NEXT_PASS (pass_sandce); NEXT_PASS (pass_dse); NEXT_PASS (pass_reassoc, true /* insert_powi_p */); NEXT_PASS (pass_dce); diff --git a/gcc/sanopt.c b/gcc/sanopt.c index cd94638..23b8e6e 100644 --- a/gcc/sanopt.c +++ b/gcc/sanopt.c @@ -906,6 +906,32 @@ sanopt_optimize (function *fun, bool *contains_asan_mark) namespace { +const pass_data pass_data_sandce = +{ + GIMPLE_PASS, /* type */ + "sandce", /* name */ + OPTGROUP_NONE, /* optinfo_flags */ + TV_NONE, /* tv_id */ + ( PROP_ssa | PROP_cfg | PROP_gimple_leh ), /* properties_required */ + 0, /* properties_provided */ + 0, /* properties_destroyed */ + 0, /* todo_flags_start */ + TODO_update_ssa, /* todo_flags_finish */ +}; + +class pass_sandce : public gimple_opt_pass +{ +public: + pass_sandce (gcc::context *ctxt) + : gimple_opt_pass (pass_data_sandce, ctxt) + {} + + /* opt_pass methods: */ + virtual bool gate (function *) { return flag_sanitize & SANITIZE_ADDRESS; } + virtual unsigned int execute (function *); + +}; // class pass_sanopt + const pass_data pass_data_sanopt = { GIMPLE_PASS, /* type */ @@ -1244,6 +1270,31 @@ sanitize_rewrite_addressable_params (function *fun) } unsigned int +pass_sandce::execute (function *fun) +{ + basic_block bb; + bool contains_asan_mark = false; + + /* Try to remove redundant unpoisoning. */ + gimple_stmt_iterator gsi; + FOR_EACH_BB_FN (bb, fun) + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) + { + gimple *stmt = gsi_stmt (gsi); + if (gimple_call_internal_p (stmt, IFN_ASAN_MARK)) + { + contains_asan_mark = true; + break; + } + } + + if (contains_asan_mark) + sanitize_asan_mark_unpoison (); + + return 0; +} + +unsigned int pass_sanopt::execute (function *fun) { basic_block bb; @@ -1367,6 +1418,12 @@ pass_sanopt::execute (function *fun) } // anon namespace gimple_opt_pass * +make_pass_sandce (gcc::context *ctxt) +{ + return new pass_sandce (ctxt); +} + +gimple_opt_pass * make_pass_sanopt (gcc::context *ctxt) { return new pass_sanopt (ctxt); diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h index 93a6a99..d5eb055 100644 --- a/gcc/tree-pass.h +++ b/gcc/tree-pass.h @@ -350,6 +350,7 @@ extern gimple_opt_pass *make_pass_tsan (gcc::context *ctxt); extern gimple_opt_pass *make_pass_tsan_O0 (gcc::context *ctxt); extern gimple_opt_pass *make_pass_sancov (gcc::context *ctxt); extern gimple_opt_pass *make_pass_sancov_O0 (gcc::context *ctxt); +extern gimple_opt_pass *make_pass_sandce (gcc::context *ctxt); extern gimple_opt_pass *make_pass_lower_cf (gcc::context *ctxt); extern gimple_opt_pass *make_pass_refactor_eh (gcc::context *ctxt); extern gimple_opt_pass *make_pass_lower_eh (gcc::context *ctxt); diff --git a/gcc/testsuite/gcc.dg/asan/pr84307.c b/gcc/testsuite/gcc.dg/asan/pr84307.c new file mode 100644 index 0000000..6e1a197 --- /dev/null +++ b/gcc/testsuite/gcc.dg/asan/pr84307.c @@ -0,0 +1,21 @@ +/* PR middle-end/83185 */ +/* { dg-do link } */ +/* { dg-options "-O1" } */ + +struct f { + void (*func)(void); +}; + +extern void link_error(void); +extern int printf(const char *f, ...); + +static inline struct f *gimme_null(struct f *result) +{ + return 0; +} + +int main(int argc, char **argv) +{ + struct f *x = gimme_null(&(struct f) { .func = link_error }); + printf("%p", x); +}