| Message ID | 4ec48432-9df6-154a-1b13-065b9772cbbf@suse.cz |
|---|---|
| State | New |
| Headers | show |
On Thu, Dec 22, 2016 at 06:03:50PM +0100, Martin Liška wrote: > Done by hash_map. Ok. > > 3) I think you just want to do copy_node, plus roughly what > > copy_decl_for_dup_finish does (and set DECL_ARTIFICIAL and > > DECL_IGNORED_P) - except that you don't have copy_body_data > > so you can't use it directly (well, you could create copy_body_data > > just for that purpose and set src_fn and dst_fn to current_function_decl > > and the rest to NULL) > > I decided to use the function with prepared copy_body_data ;) Ok. > > I'd really like to see the storing to poisoned var becoming non-addressable > > in action (if it can ever happen, so it isn't just theoretical) to look at > > what it does. > > Well, having following sample: > > int > main (int argc, char **argv) > { > int *ptr = 0; > > { > int a; > ptr = &a; > *ptr = 12345; > } > > *ptr = 12345; > return *ptr; > } > > Right after rewriting into SSA it looks as follows: > > main (int argc, char * * argv) > { > int a; > int * ptr; > int _8; > > <bb 2> [0.00%]: > a_9 = 12345; > a_10 = ASAN_POISON (); > a_11 = 12345; > _8 = a_11; > return _8; > > } But we do not want to rewrite into SSA that way, but instead as main (int argc, char * * argv) { int a; int * ptr; int _8; <bb 2> [0.00%]: a_9 = 12345; a_10 = ASAN_POISON (); ASAN_POISON (a_10); a_11 = 12345; _8 = a_11; return _8; } or something similar, so that you can 1) emit a diagnostics at the spot where the out of scope store happens 2) differentiate between reads from out of scope var and stores to out of scope var What we need is to hook into tree-into-ssa.c for this, where a_11 is created, find out that there is a store to a var that has ASAN_POISON result as currently active definition. Something like if we emit ASAN_POISON for some var, during tree-into-ssa.c if we see a store to that var that we need to rewrite into SSA pretend there is a read from that var first at that location and if it is result of ASAN_POISON, emit the additional stmt. Jakub
On 12/22/2016 06:21 PM, Jakub Jelinek wrote: > On Thu, Dec 22, 2016 at 06:03:50PM +0100, Martin Liška wrote: >> Done by hash_map. > > Ok. > >>> 3) I think you just want to do copy_node, plus roughly what >>> copy_decl_for_dup_finish does (and set DECL_ARTIFICIAL and >>> DECL_IGNORED_P) - except that you don't have copy_body_data >>> so you can't use it directly (well, you could create copy_body_data >>> just for that purpose and set src_fn and dst_fn to current_function_decl >>> and the rest to NULL) >> >> I decided to use the function with prepared copy_body_data ;) > > Ok. > >>> I'd really like to see the storing to poisoned var becoming non-addressable >>> in action (if it can ever happen, so it isn't just theoretical) to look at >>> what it does. >> >> Well, having following sample: >> >> int >> main (int argc, char **argv) >> { >> int *ptr = 0; >> >> { >> int a; >> ptr = &a; >> *ptr = 12345; >> } >> >> *ptr = 12345; >> return *ptr; >> } >> >> Right after rewriting into SSA it looks as follows: >> >> main (int argc, char * * argv) >> { >> int a; >> int * ptr; >> int _8; >> >> <bb 2> [0.00%]: >> a_9 = 12345; >> a_10 = ASAN_POISON (); >> a_11 = 12345; >> _8 = a_11; >> return _8; >> >> } > > But we do not want to rewrite into SSA that way, but instead as > > main (int argc, char * * argv) > { > int a; > int * ptr; > int _8; > > <bb 2> [0.00%]: > a_9 = 12345; > a_10 = ASAN_POISON (); > ASAN_POISON (a_10); > a_11 = 12345; > _8 = a_11; > return _8; > > } I'm still not sure how to do that. Problem is that transformation from: ASAN_MARK (UNPOISON, &a, 4); a = 5; ASAN_MARK (POISON, &a, 4); to a_8 = 5; a_9 = ASAN_POISON (); happens in tree-ssa.c, after SSA is created, in situation where we prove the 'a' does not need to live in memory. Thus said, question is how to identify that we need to transform into SSA in a different way: a_10 = ASAN_POISON (); ASAN_POISON (a_10); Thanks for help, Martin > > or something similar, so that you can 1) emit a diagnostics at the spot > where the out of scope store happens 2) differentiate between reads from > out of scope var and stores to out of scope var > > What we need is to hook into tree-into-ssa.c for this, where a_11 is > created, find out that there is a store to a var that has ASAN_POISON result > as currently active definition. Something like if we emit ASAN_POISON > for some var, during tree-into-ssa.c if we see a store to that var that we > need to rewrite into SSA pretend there is a read from that var first at > that location and if it is result of ASAN_POISON, emit the additional > stmt. > > Jakub >
On Mon, Jan 09, 2017 at 03:58:04PM +0100, Martin Liška wrote: > >> Well, having following sample: > >> > >> int > >> main (int argc, char **argv) > >> { > >> int *ptr = 0; > >> > >> { > >> int a; > >> ptr = &a; > >> *ptr = 12345; > >> } > >> > >> *ptr = 12345; > >> return *ptr; > >> } > >> > I'm still not sure how to do that. Problem is that transformation from: > > ASAN_MARK (UNPOISON, &a, 4); > a = 5; > ASAN_MARK (POISON, &a, 4); > > to > > a_8 = 5; > a_9 = ASAN_POISON (); > > happens in tree-ssa.c, after SSA is created, in situation where we prove the 'a' > does not need to live in memory. Thus said, question is how to identify that we > need to transform into SSA in a different way: > > a_10 = ASAN_POISON (); > ASAN_POISON (a_10); I meant something like this (completely untested, and without the testcase added to the testsuite). The incremental patch as is relies on the ASAN_POISON_USE call having the argument the result of ASAN_POISON, it would ICE if that is not the case (especially if -fsanitize-recover=address). Dunno if some optimization might decide to create a PHI in between, say merge two unrelated vars for if (something) { x_1 = ASAN_POISON (); ... ASAN_POISON_USE (x_1); } else { y_2 = ASAN_POISON (); ... ASAN_POISON_USE (y_2); } to turn that into: if (something) x_1 = ASAN_POISON (); else y_2 = ASAN_POISON (); _3 = PHI <x_1, y_2>; ... ASAN_POISON_USE (_3); If it did, we would ICE because ASAN_POISON_USE would survive this way until expansion. A quick fix for the ICE (if it can ever happen) would be easy, in sanopt remove ASAN_POISON_USE calls which have argument that is not lhs of ASAN_POISON (all other ASAN_POISON_USE calls will be handled by my incremental patch). Of course that would also mean in that case we'd report a read rather than write. But if it can't happen or is very unlikely to happen, then it is a non-issue. Something missing from the patch is some change in DCE to remove ASAN_POISON calls without lhs earlier. I think we can't make ASAN_POISON ECF_CONST, we don't want it to be merged for different variables. Jakub--- gcc/internal-fn.def.jj 2017-01-16 13:19:49.000000000 +0100 +++ gcc/internal-fn.def 2017-01-16 14:25:37.427962196 +0100 @@ -167,6 +167,7 @@ DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, EC 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_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) DEF_INTERNAL_FN (SUB_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (MUL_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) --- gcc/asan.c.jj 2017-01-16 13:19:49.000000000 +0100 +++ gcc/asan.c 2017-01-16 14:52:34.022044223 +0100 @@ -3094,6 +3094,8 @@ create_asan_shadow_var (tree var_decl, return *slot; } +/* Expand ASAN_POISON ifn. */ + bool asan_expand_poison_ifn (gimple_stmt_iterator *iter, bool *need_commit_edge_insert, @@ -3107,8 +3109,8 @@ asan_expand_poison_ifn (gimple_stmt_iter return true; } - tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var), - shadow_vars_mapping); + tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var), + shadow_vars_mapping); bool recover_p; if (flag_sanitize & SANITIZE_USER_ADDRESS) @@ -3122,16 +3124,16 @@ asan_expand_poison_ifn (gimple_stmt_iter ASAN_MARK_POISON), build_fold_addr_expr (shadow_var), size); - use_operand_p use_p; + gimple *use; imm_use_iterator imm_iter; - FOR_EACH_IMM_USE_FAST (use_p, imm_iter, poisoned_var) + FOR_EACH_IMM_USE_STMT (use, imm_iter, poisoned_var) { - gimple *use = USE_STMT (use_p); if (is_gimple_debug (use)) continue; int nargs; - tree fun = report_error_func (false, recover_p, tree_to_uhwi (size), + bool store_p = gimple_call_internal_p (use, IFN_ASAN_POISON_USE); + tree fun = report_error_func (store_p, recover_p, tree_to_uhwi (size), &nargs); gcall *call = gimple_build_call (fun, 1, @@ -3160,7 +3162,10 @@ asan_expand_poison_ifn (gimple_stmt_iter else { gimple_stmt_iterator gsi = gsi_for_stmt (use); - gsi_insert_before (&gsi, call, GSI_NEW_STMT); + if (store_p) + gsi_replace (&gsi, call, true); + else + gsi_insert_before (&gsi, call, GSI_NEW_STMT); } } --- gcc/tree-into-ssa.c.jj 2017-01-01 12:45:35.000000000 +0100 +++ gcc/tree-into-ssa.c 2017-01-16 14:32:14.853808726 +0100 @@ -38,6 +38,7 @@ along with GCC; see the file COPYING3. #include "tree-ssa.h" #include "domwalk.h" #include "statistics.h" +#include "asan.h" #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y)) @@ -1807,6 +1808,26 @@ maybe_replace_use_in_debug_stmt (use_ope } +/* If DEF has x_5 = ASAN_POISON () as its current def, add + ASAN_POISON_USE (x_5) stmt before GSI to denote the stmt writes into + a poisoned (out of scope) variable. */ + +static void +maybe_add_asan_poison_write (tree def, gimple_stmt_iterator *gsi) +{ + tree cdef = get_current_def (def); + if (cdef != NULL + && TREE_CODE (cdef) == SSA_NAME + && gimple_call_internal_p (SSA_NAME_DEF_STMT (cdef), IFN_ASAN_POISON)) + { + gcall *call + = gimple_build_call_internal (IFN_ASAN_POISON_USE, 1, cdef); + gimple_set_location (call, gimple_location (gsi_stmt (*gsi))); + gsi_insert_before (gsi, call, GSI_SAME_STMT); + } +} + + /* If the operand pointed to by DEF_P is an SSA name in NEW_SSA_NAMES or OLD_SSA_NAMES, or if it is a symbol marked for renaming, register it as the current definition for the names replaced by @@ -1837,7 +1858,11 @@ maybe_register_def (def_operand_p def_p, def = get_or_create_ssa_default_def (cfun, sym); } else - def = make_ssa_name (def, stmt); + { + if (asan_sanitize_use_after_scope ()) + maybe_add_asan_poison_write (def, &gsi); + def = make_ssa_name (def, stmt); + } SET_DEF (def_p, def); tree tracked_var = target_for_debug_bind (sym); --- gcc/internal-fn.c.jj 2017-01-16 13:19:49.000000000 +0100 +++ gcc/internal-fn.c 2017-01-16 14:26:10.828529039 +0100 @@ -388,6 +388,14 @@ expand_ASAN_POISON (internal_fn, gcall * gcc_unreachable (); } +/* This should get expanded in the sanopt pass. */ + +static void +expand_ASAN_POISON_USE (internal_fn, gcall *) +{ + gcc_unreachable (); +} + /* This should get expanded in the tsan pass. */ static void
On 01/16/2017 03:20 PM, Jakub Jelinek wrote: > On Mon, Jan 09, 2017 at 03:58:04PM +0100, Martin Liška wrote: >>>> Well, having following sample: >>>> >>>> int >>>> main (int argc, char **argv) >>>> { >>>> int *ptr = 0; >>>> >>>> { >>>> int a; >>>> ptr = &a; >>>> *ptr = 12345; >>>> } >>>> >>>> *ptr = 12345; >>>> return *ptr; >>>> } >>>> > >> I'm still not sure how to do that. Problem is that transformation from: >> >> ASAN_MARK (UNPOISON, &a, 4); >> a = 5; >> ASAN_MARK (POISON, &a, 4); >> >> to >> >> a_8 = 5; >> a_9 = ASAN_POISON (); >> >> happens in tree-ssa.c, after SSA is created, in situation where we prove the 'a' >> does not need to live in memory. Thus said, question is how to identify that we >> need to transform into SSA in a different way: >> >> a_10 = ASAN_POISON (); >> ASAN_POISON (a_10); > > I meant something like this (completely untested, and without the testcase > added to the testsuite). > The incremental patch as is relies on the ASAN_POISON_USE call having the > argument the result of ASAN_POISON, it would ICE if that is not the case > (especially if -fsanitize-recover=address). Dunno if some optimization > might decide to create a PHI in between, say merge two unrelated vars for > if (something) > { > x_1 = ASAN_POISON (); > ... > ASAN_POISON_USE (x_1); > } > else > { > y_2 = ASAN_POISON (); > ... > ASAN_POISON_USE (y_2); > } > to turn that into: > if (something) > x_1 = ASAN_POISON (); > else > y_2 = ASAN_POISON (); > _3 = PHI <x_1, y_2>; > ... > ASAN_POISON_USE (_3); > > If it did, we would ICE because ASAN_POISON_USE would survive this way until > expansion. A quick fix for the ICE (if it can ever happen) would be easy, > in sanopt remove ASAN_POISON_USE calls which have argument that is not lhs > of ASAN_POISON (all other ASAN_POISON_USE calls will be handled by my > incremental patch). Of course that would also mean in that case we'd report > a read rather than write. But if it can't happen or is very unlikely to > happen, then it is a non-issue. Thank you Jakub for working on that. The patch is fine, I added DCE support and a test-case. Please see attached patch. asan.exp regression tests look fine and I've been building linux kernel with KASAN enabled. I'll also do asan-boostrap. I would like to commit the patch soon, should I squash both patches together, or would it be preferred to separate basic optimization and support for stores? Thanks, Martin > Something missing from the patch is some change in DCE to remove ASAN_POISON > calls without lhs earlier. I think we can't make ASAN_POISON ECF_CONST, we > don't want it to be merged for different variables. > > --- gcc/internal-fn.def.jj 2017-01-16 13:19:49.000000000 +0100 > +++ gcc/internal-fn.def 2017-01-16 14:25:37.427962196 +0100 > @@ -167,6 +167,7 @@ DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, EC > 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_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) > DEF_INTERNAL_FN (SUB_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > DEF_INTERNAL_FN (MUL_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > --- gcc/asan.c.jj 2017-01-16 13:19:49.000000000 +0100 > +++ gcc/asan.c 2017-01-16 14:52:34.022044223 +0100 > @@ -3094,6 +3094,8 @@ create_asan_shadow_var (tree var_decl, > return *slot; > } > > +/* Expand ASAN_POISON ifn. */ > + > bool > asan_expand_poison_ifn (gimple_stmt_iterator *iter, > bool *need_commit_edge_insert, > @@ -3107,8 +3109,8 @@ asan_expand_poison_ifn (gimple_stmt_iter > return true; > } > > - tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var), > - shadow_vars_mapping); > + tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var), > + shadow_vars_mapping); > > bool recover_p; > if (flag_sanitize & SANITIZE_USER_ADDRESS) > @@ -3122,16 +3124,16 @@ asan_expand_poison_ifn (gimple_stmt_iter > ASAN_MARK_POISON), > build_fold_addr_expr (shadow_var), size); > > - use_operand_p use_p; > + gimple *use; > imm_use_iterator imm_iter; > - FOR_EACH_IMM_USE_FAST (use_p, imm_iter, poisoned_var) > + FOR_EACH_IMM_USE_STMT (use, imm_iter, poisoned_var) > { > - gimple *use = USE_STMT (use_p); > if (is_gimple_debug (use)) > continue; > > int nargs; > - tree fun = report_error_func (false, recover_p, tree_to_uhwi (size), > + bool store_p = gimple_call_internal_p (use, IFN_ASAN_POISON_USE); > + tree fun = report_error_func (store_p, recover_p, tree_to_uhwi (size), > &nargs); > > gcall *call = gimple_build_call (fun, 1, > @@ -3160,7 +3162,10 @@ asan_expand_poison_ifn (gimple_stmt_iter > else > { > gimple_stmt_iterator gsi = gsi_for_stmt (use); > - gsi_insert_before (&gsi, call, GSI_NEW_STMT); > + if (store_p) > + gsi_replace (&gsi, call, true); > + else > + gsi_insert_before (&gsi, call, GSI_NEW_STMT); > } > } > > --- gcc/tree-into-ssa.c.jj 2017-01-01 12:45:35.000000000 +0100 > +++ gcc/tree-into-ssa.c 2017-01-16 14:32:14.853808726 +0100 > @@ -38,6 +38,7 @@ along with GCC; see the file COPYING3. > #include "tree-ssa.h" > #include "domwalk.h" > #include "statistics.h" > +#include "asan.h" > > #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y)) > > @@ -1807,6 +1808,26 @@ maybe_replace_use_in_debug_stmt (use_ope > } > > > +/* If DEF has x_5 = ASAN_POISON () as its current def, add > + ASAN_POISON_USE (x_5) stmt before GSI to denote the stmt writes into > + a poisoned (out of scope) variable. */ > + > +static void > +maybe_add_asan_poison_write (tree def, gimple_stmt_iterator *gsi) > +{ > + tree cdef = get_current_def (def); > + if (cdef != NULL > + && TREE_CODE (cdef) == SSA_NAME > + && gimple_call_internal_p (SSA_NAME_DEF_STMT (cdef), IFN_ASAN_POISON)) > + { > + gcall *call > + = gimple_build_call_internal (IFN_ASAN_POISON_USE, 1, cdef); > + gimple_set_location (call, gimple_location (gsi_stmt (*gsi))); > + gsi_insert_before (gsi, call, GSI_SAME_STMT); > + } > +} > + > + > /* If the operand pointed to by DEF_P is an SSA name in NEW_SSA_NAMES > or OLD_SSA_NAMES, or if it is a symbol marked for renaming, > register it as the current definition for the names replaced by > @@ -1837,7 +1858,11 @@ maybe_register_def (def_operand_p def_p, > def = get_or_create_ssa_default_def (cfun, sym); > } > else > - def = make_ssa_name (def, stmt); > + { > + if (asan_sanitize_use_after_scope ()) > + maybe_add_asan_poison_write (def, &gsi); > + def = make_ssa_name (def, stmt); > + } > SET_DEF (def_p, def); > > tree tracked_var = target_for_debug_bind (sym); > --- gcc/internal-fn.c.jj 2017-01-16 13:19:49.000000000 +0100 > +++ gcc/internal-fn.c 2017-01-16 14:26:10.828529039 +0100 > @@ -388,6 +388,14 @@ expand_ASAN_POISON (internal_fn, gcall * > gcc_unreachable (); > } > > +/* This should get expanded in the sanopt pass. */ > + > +static void > +expand_ASAN_POISON_USE (internal_fn, gcall *) > +{ > + gcc_unreachable (); > +} > + > /* This should get expanded in the tsan pass. */ > > static void > > > Jakub >From c30802f6a29390a83208bfdb1090a6378ed42691 Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Tue, 17 Jan 2017 16:49:29 +0100 Subject: [PATCH] use-after-scope: handle writes to a poisoned variable gcc/testsuite/ChangeLog: 2017-01-17 Martin Liska <mliska@suse.cz> * gcc.dg/asan/use-after-scope-10.c: New test. gcc/ChangeLog: 2017-01-16 Jakub Jelinek <jakub@redhat.com> * asan.c (asan_expand_poison_ifn): Support stores and use appropriate ASAN report function. * internal-fn.c (expand_ASAN_POISON_USE): New function. * internal-fn.def (ASAN_POISON_USE): Declare. * tree-into-ssa.c (maybe_add_asan_poison_write): New function. (maybe_register_def): Create ASAN_POISON_USE when sanitizing. * tree-ssa-dce.c (eliminate_unnecessary_stmts): Remove ASAN_POISON calls w/o LHS. --- gcc/asan.c | 19 +++++++++++------- gcc/internal-fn.c | 8 ++++++++ gcc/internal-fn.def | 1 + gcc/testsuite/gcc.dg/asan/use-after-scope-10.c | 22 +++++++++++++++++++++ gcc/tree-into-ssa.c | 27 +++++++++++++++++++++++++- gcc/tree-ssa-dce.c | 4 ++++ 6 files changed, 73 insertions(+), 8 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/asan/use-after-scope-10.c diff --git a/gcc/asan.c b/gcc/asan.c index fe117a6951a..486ebfdb6af 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -3094,6 +3094,8 @@ create_asan_shadow_var (tree var_decl, return *slot; } +/* Expand ASAN_POISON ifn. */ + bool asan_expand_poison_ifn (gimple_stmt_iterator *iter, bool *need_commit_edge_insert, @@ -3107,8 +3109,8 @@ asan_expand_poison_ifn (gimple_stmt_iterator *iter, return true; } - tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var), - shadow_vars_mapping); + tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var), + shadow_vars_mapping); bool recover_p; if (flag_sanitize & SANITIZE_USER_ADDRESS) @@ -3122,16 +3124,16 @@ asan_expand_poison_ifn (gimple_stmt_iterator *iter, ASAN_MARK_POISON), build_fold_addr_expr (shadow_var), size); - use_operand_p use_p; + gimple *use; imm_use_iterator imm_iter; - FOR_EACH_IMM_USE_FAST (use_p, imm_iter, poisoned_var) + FOR_EACH_IMM_USE_STMT (use, imm_iter, poisoned_var) { - gimple *use = USE_STMT (use_p); if (is_gimple_debug (use)) continue; int nargs; - tree fun = report_error_func (false, recover_p, tree_to_uhwi (size), + bool store_p = gimple_call_internal_p (use, IFN_ASAN_POISON_USE); + tree fun = report_error_func (store_p, recover_p, tree_to_uhwi (size), &nargs); gcall *call = gimple_build_call (fun, 1, @@ -3160,7 +3162,10 @@ asan_expand_poison_ifn (gimple_stmt_iterator *iter, else { gimple_stmt_iterator gsi = gsi_for_stmt (use); - gsi_insert_before (&gsi, call, GSI_NEW_STMT); + if (store_p) + gsi_replace (&gsi, call, true); + else + gsi_insert_before (&gsi, call, GSI_NEW_STMT); } } diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c index 71be382ab8b..a4a2995f58b 100644 --- a/gcc/internal-fn.c +++ b/gcc/internal-fn.c @@ -388,6 +388,14 @@ expand_ASAN_POISON (internal_fn, gcall *) gcc_unreachable (); } +/* This should get expanded in the sanopt pass. */ + +static void +expand_ASAN_POISON_USE (internal_fn, gcall *) +{ + gcc_unreachable (); +} + /* This should get expanded in the tsan pass. */ static void diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def index 7b28b6722ff..fd25a952299 100644 --- a/gcc/internal-fn.def +++ b/gcc/internal-fn.def @@ -168,6 +168,7 @@ 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_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) DEF_INTERNAL_FN (SUB_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (MUL_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) diff --git a/gcc/testsuite/gcc.dg/asan/use-after-scope-10.c b/gcc/testsuite/gcc.dg/asan/use-after-scope-10.c new file mode 100644 index 00000000000..24de8cec1ff --- /dev/null +++ b/gcc/testsuite/gcc.dg/asan/use-after-scope-10.c @@ -0,0 +1,22 @@ +// { dg-do run } +// { dg-shouldfail "asan" } +// { dg-additional-options "-O2 -fdump-tree-asan1" } + +int +main (int argc, char **argv) +{ + int *ptr = 0; + + { + int a; + ptr = &a; + *ptr = 12345; + } + + *ptr = 12345; + return *ptr; +} + +// { dg-output "ERROR: AddressSanitizer: stack-use-after-scope on address.*(\n|\r\n|\r)" } +// { dg-output "WRITE of size .*" } +// { dg-output ".*'a' <== Memory access at offset \[0-9\]* is inside this variable.*" } diff --git a/gcc/tree-into-ssa.c b/gcc/tree-into-ssa.c index c7df237d57f..22261c15dc2 100644 --- a/gcc/tree-into-ssa.c +++ b/gcc/tree-into-ssa.c @@ -38,6 +38,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-ssa.h" #include "domwalk.h" #include "statistics.h" +#include "asan.h" #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y)) @@ -1807,6 +1808,26 @@ maybe_replace_use_in_debug_stmt (use_operand_p use_p) } +/* If DEF has x_5 = ASAN_POISON () as its current def, add + ASAN_POISON_USE (x_5) stmt before GSI to denote the stmt writes into + a poisoned (out of scope) variable. */ + +static void +maybe_add_asan_poison_write (tree def, gimple_stmt_iterator *gsi) +{ + tree cdef = get_current_def (def); + if (cdef != NULL + && TREE_CODE (cdef) == SSA_NAME + && gimple_call_internal_p (SSA_NAME_DEF_STMT (cdef), IFN_ASAN_POISON)) + { + gcall *call + = gimple_build_call_internal (IFN_ASAN_POISON_USE, 1, cdef); + gimple_set_location (call, gimple_location (gsi_stmt (*gsi))); + gsi_insert_before (gsi, call, GSI_SAME_STMT); + } +} + + /* If the operand pointed to by DEF_P is an SSA name in NEW_SSA_NAMES or OLD_SSA_NAMES, or if it is a symbol marked for renaming, register it as the current definition for the names replaced by @@ -1837,7 +1858,11 @@ maybe_register_def (def_operand_p def_p, gimple *stmt, def = get_or_create_ssa_default_def (cfun, sym); } else - def = make_ssa_name (def, stmt); + { + if (asan_sanitize_use_after_scope ()) + maybe_add_asan_poison_write (def, &gsi); + def = make_ssa_name (def, stmt); + } SET_DEF (def_p, def); tree tracked_var = target_for_debug_bind (sym); diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c index 4e51e699d49..7fd2478adec 100644 --- a/gcc/tree-ssa-dce.c +++ b/gcc/tree-ssa-dce.c @@ -1384,6 +1384,10 @@ eliminate_unnecessary_stmts (void) case IFN_MUL_OVERFLOW: maybe_optimize_arith_overflow (&gsi, MULT_EXPR); break; + case IFN_ASAN_POISON: + if (!gimple_has_lhs (stmt)) + remove_dead_stmt (&gsi, bb); + break; default: break; } -- 2.11.0
On Tue, Jan 17, 2017 at 05:16:44PM +0100, Martin Liška wrote: > > If it did, we would ICE because ASAN_POISON_USE would survive this way until > > expansion. A quick fix for the ICE (if it can ever happen) would be easy, > > in sanopt remove ASAN_POISON_USE calls which have argument that is not lhs > > of ASAN_POISON (all other ASAN_POISON_USE calls will be handled by my > > incremental patch). Of course that would also mean in that case we'd report > > a read rather than write. But if it can't happen or is very unlikely to > > happen, then it is a non-issue. > > Thank you Jakub for working on that. > > The patch is fine, I added DCE support and a test-case. Please see attached patch. > asan.exp regression tests look fine and I've been building linux kernel with KASAN > enabled. I'll also do asan-boostrap. > > I would like to commit the patch soon, should I squash both patches together, or would it > be preferred to separate basic optimization and support for stores? Your choice, either is fine. If the two patches pass bootstrap/regtest (ideally also asan-bootstrap), they are ok for trunk. Just one nit: > --- a/gcc/tree-ssa-dce.c > +++ b/gcc/tree-ssa-dce.c > @@ -1384,6 +1384,10 @@ eliminate_unnecessary_stmts (void) > case IFN_MUL_OVERFLOW: > maybe_optimize_arith_overflow (&gsi, MULT_EXPR); > break; > + case IFN_ASAN_POISON: > + if (!gimple_has_lhs (stmt)) > + remove_dead_stmt (&gsi, bb); > + break; > default: > break; > } This doesn't seem to be the best spot for it. At least when looking at say: int foo (int x) { int *ptr = 0; if (x < 127) return 5; { int a; ptr = &a; *ptr = 12345; } if (x == 34) return *ptr; return 7; } where the ASAN_POISON is initially used and only after evrp becomes dead, then cddce1 calls eliminate_unnecessary_stmts and removes the lhs of the ASAN_POISON only (and not the whole stmt, unlike how e.g. GOMP_SIMD_LANE is handled), and only next dce pass tons of passes later removes the ASAN_POISON call. So IMHO you need one of these (untested) patches. The former assumes that the DCE pass is the only one that can drop the lhs of ASAN_POISON. If that is not the case, then perhaps the second patch is better, by removing the stmt regardless if we've removed the lhs in the current dce pass or in whatever earlier pass. I think it shouldn't break IFN_*_OVERFLOW, because maybe_optimize_arith_overflow starts with tree lhs = gimple_call_lhs (stmt); if (lhs == NULL_TREE || ...) return; Jakub --- gcc/tree-ssa-dce.c.jj 2017-01-01 12:45:38.380670110 +0100 +++ gcc/tree-ssa-dce.c 2017-01-17 17:37:38.639427099 +0100 @@ -1366,13 +1366,8 @@ eliminate_unnecessary_stmts (void) maybe_clean_or_replace_eh_stmt (stmt, stmt); update_stmt (stmt); release_ssa_name (name); - - /* GOMP_SIMD_LANE without lhs is not needed. */ - if (gimple_call_internal_p (stmt) - && gimple_call_internal_fn (stmt) == IFN_GOMP_SIMD_LANE) - remove_dead_stmt (&gsi, bb); } - else if (gimple_call_internal_p (stmt)) + if (gimple_call_internal_p (stmt)) switch (gimple_call_internal_fn (stmt)) { case IFN_ADD_OVERFLOW: @@ -1384,6 +1379,13 @@ eliminate_unnecessary_stmts (void) case IFN_MUL_OVERFLOW: maybe_optimize_arith_overflow (&gsi, MULT_EXPR); break; + /* GOMP_SIMD_LANE or ASAN_POISON without lhs is not + needed. */ + case IFN_GOMP_SIMD_LANE: + case IFN_ASAN_POISON: + if (gimple_call_lhs (stmt) == NULL_TREE) + remove_dead_stmt (&gsi, bb); + break; default: break; }--- gcc/tree-ssa-dce.c.jj 2017-01-01 12:45:38.380670110 +0100 +++ gcc/tree-ssa-dce.c 2017-01-17 17:35:43.650902141 +0100 @@ -1367,10 +1367,18 @@ eliminate_unnecessary_stmts (void) update_stmt (stmt); release_ssa_name (name); - /* GOMP_SIMD_LANE without lhs is not needed. */ - if (gimple_call_internal_p (stmt) - && gimple_call_internal_fn (stmt) == IFN_GOMP_SIMD_LANE) - remove_dead_stmt (&gsi, bb); + /* GOMP_SIMD_LANE or ASAN_POISON without lhs is not + needed. */ + if (gimple_call_internal_p (stmt)) + switch (gimple_call_internal_fn (stmt)) + { + case IFN_GOMP_SIMD_LANE: + case IFN_ASAN_POISON: + remove_dead_stmt (&gsi, bb); + break; + default: + break; + } } else if (gimple_call_internal_p (stmt)) switch (gimple_call_internal_fn (stmt))
Hello.
During bootstrap, I came to following test-case:
struct A
{
int regno;
};
struct
{
A base;
} typedef *df_ref;
int *a;
void
fn1 (int N)
{
for (int i = 0; i < N; i++)
{
df_ref b;
a[(b)->base.regno]++;
}
}
As we expand all usages of an LHS of a ASAN_POISON to all uses, we propagate that to
a PHI node that originally contained ASAN_MARK (UNPOISON):
<bb 4> [0.00%]:
ASAN_MARK (UNPOISON, &b, 8);
a.0_1 = a;
b.1_2 = b;
_3 = b.1_2->base.regno;
_4 = (long unsigned int) _3;
_5 = _4 * 4;
_6 = a.0_1 + _5;
_7 = *_6;
_8 = _7 + 1;
*_6 = _8;
ASAN_MARK (POISON, &b, 8);
i_17 = i_9 + 1;
goto <bb 3>; [0.00%]
Is transformed to:
<bb 3> [0.00%]:
# i_9 = PHI <0(2), i_17(4)>
# b_18 = PHI <b_19(D)(2), b_20(4)>
if (i_9 >= N_13(D))
goto <bb 5>; [0.00%]
else
goto <bb 4>; [0.00%]
<bb 4> [0.00%]:
a.0_1 = a;
b.1_2 = b_18;
_3 = b.1_2->base.regno;
_4 = (long unsigned int) _3;
_5 = _4 * 4;
_6 = a.0_1 + _5;
_7 = *_6;
_8 = _7 + 1;
*_6 = _8;
b_20 = ASAN_POISON ();
i_17 = i_9 + 1;
goto <bb 3>; [0.00%]
Motivation for propagation over PHI nodes was:
cat use.c
int
main (int argc, char **argv)
{
int *ptr = 0;
if (argc == 1)
{
int my_char;
ptr = &my_char;
}
if (ptr)
return *ptr;
return 0;
}
I'm thinking whether the selected approach is fundamentally wrong, our we'll have to stop the PHI propagation
and we still be able to catch some cases with -O2?
Thanks,
Martin
On Wed, Jan 18, 2017 at 04:34:48PM +0100, Martin Liška wrote: > Hello. > > During bootstrap, I came to following test-case: > > struct A > { > int regno; > }; > struct > { > A base; > } typedef *df_ref; > int *a; > void > fn1 (int N) > { > for (int i = 0; i < N; i++) > { > df_ref b; > a[(b)->base.regno]++; > } > } Well, in this case it is UB too, just not actually out of bounds access, but use of uninitialized variable. Perhaps what we should do, in addition to turning ASAN_MARK (POISON, &b, ...) into b = ASAN_POISON (); turn ASAN_MARK (UNPOISON, &b, ...) into b = b_YYY(D); The following seems to do the job: Jakub--- gcc/tree-ssa.c.jj 2017-01-19 17:20:15.000000000 +0100 +++ gcc/tree-ssa.c 2017-01-19 17:29:58.015356370 +0100 @@ -1911,7 +1911,16 @@ execute_update_addresses_taken (void) gsi_replace (&gsi, call, GSI_SAME_STMT); } else - gsi_remove (&gsi, true); + { + /* In ASAN_MARK (UNPOISON, &b, ...) the variable + is uninitialized. Avoid dependencies on + previous out of scope value. */ + tree clobber + = build_constructor (TREE_TYPE (var), NULL); + TREE_THIS_VOLATILE (clobber) = 1; + gimple *g = gimple_build_assign (var, clobber); + gsi_replace (&gsi, g, GSI_SAME_STMT); + } continue; } }
On 01/19/2017 05:33 PM, Jakub Jelinek wrote: > On Wed, Jan 18, 2017 at 04:34:48PM +0100, Martin Liška wrote: >> Hello. >> >> During bootstrap, I came to following test-case: >> >> struct A >> { >> int regno; >> }; >> struct >> { >> A base; >> } typedef *df_ref; >> int *a; >> void >> fn1 (int N) >> { >> for (int i = 0; i < N; i++) >> { >> df_ref b; >> a[(b)->base.regno]++; >> } >> } > > Well, in this case it is UB too, just not actually out of bounds access, > but use of uninitialized variable. > Perhaps what we should do, in addition to turning ASAN_MARK (POISON, &b, ...) > into b = ASAN_POISON (); turn ASAN_MARK (UNPOISON, &b, ...) into > b = b_YYY(D); Great, thanks a lot. I'm going to re-trigger asan-bootstrap with your patch. I'm also adding gcc/testsuite/gcc.dg/asan/use-after-scope-10.c that is a valid test-case for this issue. Hopefully it will survive both regression tests and asan-bootstrap. Thanks, Martin > The following seems to do the job: > --- gcc/tree-ssa.c.jj 2017-01-19 17:20:15.000000000 +0100 > +++ gcc/tree-ssa.c 2017-01-19 17:29:58.015356370 +0100 > @@ -1911,7 +1911,16 @@ execute_update_addresses_taken (void) > gsi_replace (&gsi, call, GSI_SAME_STMT); > } > else > - gsi_remove (&gsi, true); > + { > + /* In ASAN_MARK (UNPOISON, &b, ...) the variable > + is uninitialized. Avoid dependencies on > + previous out of scope value. */ > + tree clobber > + = build_constructor (TREE_TYPE (var), NULL); > + TREE_THIS_VOLATILE (clobber) = 1; > + gimple *g = gimple_build_assign (var, clobber); > + gsi_replace (&gsi, g, GSI_SAME_STMT); > + } > continue; > } > } > > Jakub >From fa8a7fa81df7cf775dcf9018911044e5a331570d Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Tue, 17 Jan 2017 16:49:29 +0100 Subject: [PATCH] use-after-scope: handle writes to a poisoned variable gcc/testsuite/ChangeLog: 2017-01-17 Martin Liska <mliska@suse.cz> * gcc.dg/asan/use-after-scope-10.c: New test. * g++.dg/asan/use-after-scope-5.C: New test. gcc/ChangeLog: 2017-01-16 Jakub Jelinek <jakub@redhat.com> * asan.c (asan_expand_poison_ifn): Support stores and use appropriate ASAN report function. * internal-fn.c (expand_ASAN_POISON_USE): New function. * internal-fn.def (ASAN_POISON_USE): Declare. * tree-into-ssa.c (maybe_add_asan_poison_write): New function. (maybe_register_def): Create ASAN_POISON_USE when sanitizing. * tree-ssa-dce.c (eliminate_unnecessary_stmts): Remove ASAN_POISON calls w/o LHS. * tree-ssa.c (execute_update_addresses_taken): Create clobber for ASAN_MARK (UNPOISON, &x, ...) in order to prevent usage of a LHS from ASAN_MARK (POISON, &x, ...) coming to a PHI node. --- gcc/asan.c | 19 +++++++++++------- gcc/internal-fn.c | 8 ++++++++ gcc/internal-fn.def | 1 + gcc/testsuite/g++.dg/asan/use-after-scope-5.C | 23 ++++++++++++++++++++++ gcc/testsuite/gcc.dg/asan/use-after-scope-10.c | 22 +++++++++++++++++++++ gcc/tree-into-ssa.c | 27 +++++++++++++++++++++++++- gcc/tree-ssa-dce.c | 16 +++++++++++---- gcc/tree-ssa.c | 11 ++++++++++- 8 files changed, 114 insertions(+), 13 deletions(-) create mode 100644 gcc/testsuite/g++.dg/asan/use-after-scope-5.C create mode 100644 gcc/testsuite/gcc.dg/asan/use-after-scope-10.c diff --git a/gcc/asan.c b/gcc/asan.c index fe117a6951a..486ebfdb6af 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -3094,6 +3094,8 @@ create_asan_shadow_var (tree var_decl, return *slot; } +/* Expand ASAN_POISON ifn. */ + bool asan_expand_poison_ifn (gimple_stmt_iterator *iter, bool *need_commit_edge_insert, @@ -3107,8 +3109,8 @@ asan_expand_poison_ifn (gimple_stmt_iterator *iter, return true; } - tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var), - shadow_vars_mapping); + tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var), + shadow_vars_mapping); bool recover_p; if (flag_sanitize & SANITIZE_USER_ADDRESS) @@ -3122,16 +3124,16 @@ asan_expand_poison_ifn (gimple_stmt_iterator *iter, ASAN_MARK_POISON), build_fold_addr_expr (shadow_var), size); - use_operand_p use_p; + gimple *use; imm_use_iterator imm_iter; - FOR_EACH_IMM_USE_FAST (use_p, imm_iter, poisoned_var) + FOR_EACH_IMM_USE_STMT (use, imm_iter, poisoned_var) { - gimple *use = USE_STMT (use_p); if (is_gimple_debug (use)) continue; int nargs; - tree fun = report_error_func (false, recover_p, tree_to_uhwi (size), + bool store_p = gimple_call_internal_p (use, IFN_ASAN_POISON_USE); + tree fun = report_error_func (store_p, recover_p, tree_to_uhwi (size), &nargs); gcall *call = gimple_build_call (fun, 1, @@ -3160,7 +3162,10 @@ asan_expand_poison_ifn (gimple_stmt_iterator *iter, else { gimple_stmt_iterator gsi = gsi_for_stmt (use); - gsi_insert_before (&gsi, call, GSI_NEW_STMT); + if (store_p) + gsi_replace (&gsi, call, true); + else + gsi_insert_before (&gsi, call, GSI_NEW_STMT); } } diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c index 71be382ab8b..a4a2995f58b 100644 --- a/gcc/internal-fn.c +++ b/gcc/internal-fn.c @@ -388,6 +388,14 @@ expand_ASAN_POISON (internal_fn, gcall *) gcc_unreachable (); } +/* This should get expanded in the sanopt pass. */ + +static void +expand_ASAN_POISON_USE (internal_fn, gcall *) +{ + gcc_unreachable (); +} + /* This should get expanded in the tsan pass. */ static void diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def index 7b28b6722ff..fd25a952299 100644 --- a/gcc/internal-fn.def +++ b/gcc/internal-fn.def @@ -168,6 +168,7 @@ 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_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) DEF_INTERNAL_FN (SUB_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (MUL_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) diff --git a/gcc/testsuite/g++.dg/asan/use-after-scope-5.C b/gcc/testsuite/g++.dg/asan/use-after-scope-5.C new file mode 100644 index 00000000000..7e28fc35e6d --- /dev/null +++ b/gcc/testsuite/g++.dg/asan/use-after-scope-5.C @@ -0,0 +1,23 @@ +// { dg-do run } + +int * +__attribute__((optimize(("-O0")))) +fn1 (int *a) +{ + return a; +} + +void +fn2 () +{ + for (int i = 0; i < 10; i++) + { + int *a; + (a) = fn1 (a); + } +} + +int main() +{ + fn2(); +} diff --git a/gcc/testsuite/gcc.dg/asan/use-after-scope-10.c b/gcc/testsuite/gcc.dg/asan/use-after-scope-10.c new file mode 100644 index 00000000000..24de8cec1ff --- /dev/null +++ b/gcc/testsuite/gcc.dg/asan/use-after-scope-10.c @@ -0,0 +1,22 @@ +// { dg-do run } +// { dg-shouldfail "asan" } +// { dg-additional-options "-O2 -fdump-tree-asan1" } + +int +main (int argc, char **argv) +{ + int *ptr = 0; + + { + int a; + ptr = &a; + *ptr = 12345; + } + + *ptr = 12345; + return *ptr; +} + +// { dg-output "ERROR: AddressSanitizer: stack-use-after-scope on address.*(\n|\r\n|\r)" } +// { dg-output "WRITE of size .*" } +// { dg-output ".*'a' <== Memory access at offset \[0-9\]* is inside this variable.*" } diff --git a/gcc/tree-into-ssa.c b/gcc/tree-into-ssa.c index c7df237d57f..22261c15dc2 100644 --- a/gcc/tree-into-ssa.c +++ b/gcc/tree-into-ssa.c @@ -38,6 +38,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-ssa.h" #include "domwalk.h" #include "statistics.h" +#include "asan.h" #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y)) @@ -1807,6 +1808,26 @@ maybe_replace_use_in_debug_stmt (use_operand_p use_p) } +/* If DEF has x_5 = ASAN_POISON () as its current def, add + ASAN_POISON_USE (x_5) stmt before GSI to denote the stmt writes into + a poisoned (out of scope) variable. */ + +static void +maybe_add_asan_poison_write (tree def, gimple_stmt_iterator *gsi) +{ + tree cdef = get_current_def (def); + if (cdef != NULL + && TREE_CODE (cdef) == SSA_NAME + && gimple_call_internal_p (SSA_NAME_DEF_STMT (cdef), IFN_ASAN_POISON)) + { + gcall *call + = gimple_build_call_internal (IFN_ASAN_POISON_USE, 1, cdef); + gimple_set_location (call, gimple_location (gsi_stmt (*gsi))); + gsi_insert_before (gsi, call, GSI_SAME_STMT); + } +} + + /* If the operand pointed to by DEF_P is an SSA name in NEW_SSA_NAMES or OLD_SSA_NAMES, or if it is a symbol marked for renaming, register it as the current definition for the names replaced by @@ -1837,7 +1858,11 @@ maybe_register_def (def_operand_p def_p, gimple *stmt, def = get_or_create_ssa_default_def (cfun, sym); } else - def = make_ssa_name (def, stmt); + { + if (asan_sanitize_use_after_scope ()) + maybe_add_asan_poison_write (def, &gsi); + def = make_ssa_name (def, stmt); + } SET_DEF (def_p, def); tree tracked_var = target_for_debug_bind (sym); diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c index 4e51e699d49..5ebe57b0983 100644 --- a/gcc/tree-ssa-dce.c +++ b/gcc/tree-ssa-dce.c @@ -1367,10 +1367,18 @@ eliminate_unnecessary_stmts (void) update_stmt (stmt); release_ssa_name (name); - /* GOMP_SIMD_LANE without lhs is not needed. */ - if (gimple_call_internal_p (stmt) - && gimple_call_internal_fn (stmt) == IFN_GOMP_SIMD_LANE) - remove_dead_stmt (&gsi, bb); + /* GOMP_SIMD_LANE or ASAN_POISON without lhs is not + needed. */ + if (gimple_call_internal_p (stmt)) + switch (gimple_call_internal_fn (stmt)) + { + case IFN_GOMP_SIMD_LANE: + case IFN_ASAN_POISON: + remove_dead_stmt (&gsi, bb); + break; + default: + break; + } } else if (gimple_call_internal_p (stmt)) switch (gimple_call_internal_fn (stmt)) diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c index cc920950bab..5bd9004e715 100644 --- a/gcc/tree-ssa.c +++ b/gcc/tree-ssa.c @@ -1886,7 +1886,16 @@ execute_update_addresses_taken (void) gsi_replace (&gsi, call, GSI_SAME_STMT); } else - gsi_remove (&gsi, true); + { + /* In ASAN_MARK (UNPOISON, &b, ...) the variable + is uninitialized. Avoid dependencies on + previous out of scope value. */ + tree clobber + = build_constructor (TREE_TYPE (var), NULL); + TREE_THIS_VOLATILE (clobber) = 1; + gimple *g = gimple_build_assign (var, clobber); + gsi_replace (&gsi, g, GSI_SAME_STMT); + } continue; } } -- 2.11.0
On 01/20/2017 12:49 PM, Martin Liška wrote: > Great, thanks a lot. I'm going to re-trigger asan-bootstrap with your patch. > I'm also adding gcc/testsuite/gcc.dg/asan/use-after-scope-10.c that is a valid > test-case for this issue. Hi. Unfortunately this way would not work as clobber marks content of the memory as uninitialize is different behavior that just marking a memory can be used (and maybe already contains a value). This shows the problem: #include <string.h> char cc; char ptr[] = "sparta2"; void get(char **x) { *x = ptr; } int main() { char *here = &cc; for (;;) { next_line: if (here == NULL) __builtin_abort(); get (&here); if (strcmp (here, "sparta") == 0) goto next_line; else if (strcmp (here, "sparta2") == 0) break; } } With the patch, DSE would optimize out '*here = &cc;' and thus aborts. The problem is definitely related to goto magic, where we are more defensive in placement of ASAN_MARK(UNPOISON,...). Hope your optimization is still valid for situations w/o artificial ASAN_MARK(UNPOISON,...) placed due to goto magic. Do we still want to do it now, or postponing to GCC 8 would be better option? Thanks, Martin
On Fri, Jan 20, 2017 at 03:08:21PM +0100, Martin Liška wrote: > Unfortunately this way would not work as clobber marks content of the memory as uninitialize > is different behavior that just marking a memory can be used (and maybe already contains a value). > > This shows the problem: > > #include <string.h> > > char cc; > char ptr[] = "sparta2"; > > void get(char **x) > { > *x = ptr; > } > > int main() > { > char *here = &cc; > > for (;;) > { > next_line: > if (here == NULL) > __builtin_abort(); > get (&here); > if (strcmp (here, "sparta") == 0) > goto next_line; > else if (strcmp (here, "sparta2") == 0) > break; > } > } > > With the patch, DSE would optimize out '*here = &cc;' and thus aborts. The problem is definitely > related to goto magic, where we are more defensive in placement of ASAN_MARK(UNPOISON,...). > Hope your optimization is still valid for situations w/o artificial ASAN_MARK(UNPOISON,...) placed due > to goto magic. > > Do we still want to do it now, or postponing to GCC 8 would be better option? I'd still like to resolve it for GCC 7 if at all possible, I think otherwise -fsanitize=address is by default unnecessarily slower (so it is a regression anyway). So, do we always amit ASAN_MARK(UNPOISON, ...) at the start of scope and then yet another ASAN_MARK(UNPOISON, ...) at the goto destination? At least on the above testcase that is the case, so if we say split ASAN_MARK_UNPOISON into something that is used at the start of scope (we'd emit the clobber for those) and others (we would not), then perhaps we could get around that. The above is BTW a clear case where shouldn't emit UNPOISON on the label, as the goto doesn't cross its initialization. But I can see with gotos from outside of some var's scope into it we wouldn't handle it properly. Perhaps for now set some flag/attribute/whatever on vars for which we emit the conservative UNPOISON and never allow those to be made non-addressable (i.e. for those say that POISON/UNPOISON actually makes them always addressable)? Jakub
On 2017.01.20 at 15:27 +0100, Jakub Jelinek wrote: > On Fri, Jan 20, 2017 at 03:08:21PM +0100, Martin Liška wrote: > > Unfortunately this way would not work as clobber marks content of the memory as uninitialize > > is different behavior that just marking a memory can be used (and maybe already contains a value). > > > > This shows the problem: > > > > #include <string.h> > > > > char cc; > > char ptr[] = "sparta2"; > > > > void get(char **x) > > { > > *x = ptr; > > } > > > > int main() > > { > > char *here = &cc; > > > > for (;;) > > { > > next_line: > > if (here == NULL) > > __builtin_abort(); > > get (&here); > > if (strcmp (here, "sparta") == 0) > > goto next_line; > > else if (strcmp (here, "sparta2") == 0) > > break; > > } > > } > > > > With the patch, DSE would optimize out '*here = &cc;' and thus aborts. The problem is definitely > > related to goto magic, where we are more defensive in placement of ASAN_MARK(UNPOISON,...). > > Hope your optimization is still valid for situations w/o artificial ASAN_MARK(UNPOISON,...) placed due > > to goto magic. > > > > Do we still want to do it now, or postponing to GCC 8 would be better option? > > I'd still like to resolve it for GCC 7 if at all possible, I think otherwise > -fsanitize=address is by default unnecessarily slower (so it is a regression > anyway). Another possibility would be to disable use-after-scope for gcc-7 (like LLVM) and re-enable it for gcc-8. -- Markusdiff --git a/gcc/opts.c b/gcc/opts.c index 5f573a16ff15..2664b54133e4 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -993,7 +993,7 @@ finish_options (struct gcc_options *opts, struct gcc_options *opts_set, enabled. */ if ((opts->x_flag_sanitize & SANITIZE_USER_ADDRESS) && !opts_set->x_flag_sanitize_address_use_after_scope) - opts->x_flag_sanitize_address_use_after_scope = true; + opts->x_flag_sanitize_address_use_after_scope = false; /* Force -fstack-reuse=none in case -fsanitize-address-use-after-scope is enabled. */
On 01/20/2017 03:27 PM, Jakub Jelinek wrote: > On Fri, Jan 20, 2017 at 03:08:21PM +0100, Martin Liška wrote: >> Unfortunately this way would not work as clobber marks content of the memory as uninitialize >> is different behavior that just marking a memory can be used (and maybe already contains a value). >> >> This shows the problem: >> >> #include <string.h> >> >> char cc; >> char ptr[] = "sparta2"; >> >> void get(char **x) >> { >> *x = ptr; >> } >> >> int main() >> { >> char *here = &cc; >> >> for (;;) >> { >> next_line: >> if (here == NULL) >> __builtin_abort(); >> get (&here); >> if (strcmp (here, "sparta") == 0) >> goto next_line; >> else if (strcmp (here, "sparta2") == 0) >> break; >> } >> } >> >> With the patch, DSE would optimize out '*here = &cc;' and thus aborts. The problem is definitely >> related to goto magic, where we are more defensive in placement of ASAN_MARK(UNPOISON,...). >> Hope your optimization is still valid for situations w/o artificial ASAN_MARK(UNPOISON,...) placed due >> to goto magic. >> >> Do we still want to do it now, or postponing to GCC 8 would be better option? > > I'd still like to resolve it for GCC 7 if at all possible, I think otherwise > -fsanitize=address is by default unnecessarily slower (so it is a regression > anyway). Good, I hope I have patch that finally works as we want. It add attribute to variables that are unpoisoned as live switch variables, or are defined in a label which address is taken. With [1] I can bootstrap-asan and the patch can bootstrap on ppc64le-redhat-linux and survives regression tests. I'm sending latest diff, as well as the final pair of patches I would like to install. Ready to be installed? Martin > So, do we always amit ASAN_MARK(UNPOISON, ...) at the start of scope and > then yet another ASAN_MARK(UNPOISON, ...) at the goto destination? > At least on the above testcase that is the case, so if we say split > ASAN_MARK_UNPOISON into something that is used at the start of scope > (we'd emit the clobber for those) and others (we would not), then perhaps we > could get around that. The above is BTW a clear case where shouldn't emit > UNPOISON on the label, as the goto doesn't cross its initialization. > But I can see with gotos from outside of some var's scope into it we > wouldn't handle it properly. Perhaps for now set some > flag/attribute/whatever on vars for which we emit the conservative > UNPOISON and never allow those to be made non-addressable (i.e. for those > say that POISON/UNPOISON actually makes them always addressable)? > > Jakub > [1] https://gcc.gnu.org/ml/gcc-patches/2017-01/msg01446.htmldiff --git a/gcc/gimplify.c b/gcc/gimplify.c index 2777a23eb93..1b076fdf45c 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -1206,8 +1206,19 @@ asan_poison_variables (hash_set<tree> *variables, bool poison, gimple_seq *seq_p sorted_variables.qsort (sort_by_decl_uid); - for (unsigned i = 0; i < sorted_variables.length (); i++) - asan_poison_variable (sorted_variables[i], poison, seq_p); + unsigned i; + tree var; + FOR_EACH_VEC_ELT (sorted_variables, i, var) + { + asan_poison_variable (var, poison, seq_p); + + /* Add use_after_scope_memory attribute for the variable in order + to prevent re-written into SSA. */ + DECL_ATTRIBUTES (var) + = tree_cons (get_identifier ("use_after_scope_memory"), + build_int_cst (integer_type_node, 1), + DECL_ATTRIBUTES (var)); + } } /* Gimplify a BIND_EXPR. Just voidify and recurse. */ diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c index 5bd9004e715..2b33da93a23 100644 --- a/gcc/tree-ssa.c +++ b/gcc/tree-ssa.c @@ -1565,6 +1565,10 @@ is_asan_mark_p (gimple *stmt) && VAR_P (TREE_OPERAND (addr, 0))) { tree var = TREE_OPERAND (addr, 0); + if (lookup_attribute ("use_after_scope_memory", + DECL_ATTRIBUTES (var))) + return false; + unsigned addressable = TREE_ADDRESSABLE (var); TREE_ADDRESSABLE (var) = 0; bool r = is_gimple_reg (var);
On Mon, Jan 23, 2017 at 10:19:33AM +0100, Martin Liška wrote: > diff --git a/gcc/gimplify.c b/gcc/gimplify.c > index 2777a23eb93..1b076fdf45c 100644 > --- a/gcc/gimplify.c > +++ b/gcc/gimplify.c > @@ -1206,8 +1206,19 @@ asan_poison_variables (hash_set<tree> *variables, bool poison, gimple_seq *seq_p > > sorted_variables.qsort (sort_by_decl_uid); > > - for (unsigned i = 0; i < sorted_variables.length (); i++) > - asan_poison_variable (sorted_variables[i], poison, seq_p); > + unsigned i; > + tree var; > + FOR_EACH_VEC_ELT (sorted_variables, i, var) > + { > + asan_poison_variable (var, poison, seq_p); > + > + /* Add use_after_scope_memory attribute for the variable in order > + to prevent re-written into SSA. */ > + DECL_ATTRIBUTES (var) > + = tree_cons (get_identifier ("use_after_scope_memory"), Please use "use after scope memory" to make it clear it is internal only attribute users can't specify. Also, can't asan_poison_variables be performed on the same var multiple times (multiple labels, or switches, ...)? If yes, then the addition of the attribute should be guarded with if (!lookup_attribute ("use after scope memory", DECL_ATTRIBUTES (vars))) so that you don't add the attributes many times. > + build_int_cst (integer_type_node, 1), > + DECL_ATTRIBUTES (var)); Please use: integer_one_node, DECL_ATTRIBUTES (var)); instead. > --- a/gcc/tree-ssa.c > +++ b/gcc/tree-ssa.c > @@ -1565,6 +1565,10 @@ is_asan_mark_p (gimple *stmt) > && VAR_P (TREE_OPERAND (addr, 0))) > { > tree var = TREE_OPERAND (addr, 0); > + if (lookup_attribute ("use_after_scope_memory", > + DECL_ATTRIBUTES (var))) > + return false; See above. Patchset is ok for trunk with these nits fixed, thanks. Jakub
On 01/23/2017 10:38 AM, Jakub Jelinek wrote: > On Mon, Jan 23, 2017 at 10:19:33AM +0100, Martin Liška wrote: >> diff --git a/gcc/gimplify.c b/gcc/gimplify.c >> index 2777a23eb93..1b076fdf45c 100644 >> --- a/gcc/gimplify.c >> +++ b/gcc/gimplify.c >> @@ -1206,8 +1206,19 @@ asan_poison_variables (hash_set<tree> *variables, bool poison, gimple_seq *seq_p >> >> sorted_variables.qsort (sort_by_decl_uid); >> >> - for (unsigned i = 0; i < sorted_variables.length (); i++) >> - asan_poison_variable (sorted_variables[i], poison, seq_p); >> + unsigned i; >> + tree var; >> + FOR_EACH_VEC_ELT (sorted_variables, i, var) >> + { >> + asan_poison_variable (var, poison, seq_p); >> + >> + /* Add use_after_scope_memory attribute for the variable in order >> + to prevent re-written into SSA. */ >> + DECL_ATTRIBUTES (var) >> + = tree_cons (get_identifier ("use_after_scope_memory"), > > Please use "use after scope memory" to make it clear it is internal > only attribute users can't specify. > Also, can't asan_poison_variables be performed on the same var > multiple times (multiple labels, or switches, ...)? > If yes, then the addition of the attribute should be guarded > with if (!lookup_attribute ("use after scope memory", DECL_ATTRIBUTES (vars))) > so that you don't add the attributes many times. > >> + build_int_cst (integer_type_node, 1), >> + DECL_ATTRIBUTES (var)); > > Please use: > integer_one_node, DECL_ATTRIBUTES (var)); > instead. > >> --- a/gcc/tree-ssa.c >> +++ b/gcc/tree-ssa.c >> @@ -1565,6 +1565,10 @@ is_asan_mark_p (gimple *stmt) >> && VAR_P (TREE_OPERAND (addr, 0))) >> { >> tree var = TREE_OPERAND (addr, 0); >> + if (lookup_attribute ("use_after_scope_memory", >> + DECL_ATTRIBUTES (var))) >> + return false; > > See above. > > Patchset is ok for trunk with these nits fixed, thanks. > > Jakub > Great, installed as r244791 and r244793. One more time, really thank you for help, it took some time to finalize the optimization. Martin
Hi! On Mon, 23 Jan 2017 10:19:33 +0100, Martin Liška <mliska@suse.cz> wrote: > --- a/gcc/testsuite/gcc.dg/asan/use-after-scope-3.c > +++ b/gcc/testsuite/gcc.dg/asan/use-after-scope-3.c > @@ -1,5 +1,6 @@ > // { dg-do run } > // { dg-shouldfail "asan" } > +// { dg-additional-options "-O0" } As these tests per "gcc/testsuite/gcc.dg/asan/asan.exp" are run with "gcc-dg-runtest", which will "cycle through a list of optimization options as c-torture does", is it really appropriate to hard-code "-O0" here? Shouldn't instead be all testing be "dg-skip"ped (or similar) unless "-O0" is in effect? > --- a/gcc/testsuite/gcc.dg/asan/use-after-scope-9.c > +++ b/gcc/testsuite/gcc.dg/asan/use-after-scope-9.c > @@ -1,5 +1,6 @@ > // { dg-do run } > // { dg-shouldfail "asan" } > +// { dg-additional-options "-O2 -fdump-tree-asan1" } Likewise. (I didn't check any other such test cases.) > +// { dg-final { scan-tree-dump-times "= ASAN_POISON \\(\\)" 1 "asan1" } } > // { dg-output "ERROR: AddressSanitizer: stack-use-after-scope on address.*(\n|\r\n|\r)" } > // { dg-output "READ of size .*" } > // { dg-output ".*'a' <== Memory access at offset \[0-9\]* is inside this variable.*" } This is PASS for most of all tortute test options, but for "-O2 -flto -fuse-linker-plugin -fno-fat-lto-objects" will produce: PASS: gcc.dg/asan/use-after-scope-9.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects (test for excess errors) PASS: gcc.dg/asan/use-after-scope-9.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects execution test PASS: gcc.dg/asan/use-after-scope-9.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects output pattern test, ERROR: AddressSanitizer: stack-use-after-scope on address.*( | |^M)READ of size .*.*'a' <== Memory access at offset [0-9]* is inside this variable.* {+UNRESOLVED: gcc.dg/asan/use-after-scope-9.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects scan-tree-dump-times asan1 "= ASAN_POISON \\(\\)" 1+} (Notice UNRESOLVED.) Is this to be expected/skipped/fixed? Grüße Thomas
On Thu, Jan 26, 2017 at 09:57:14AM +0100, Thomas Schwinge wrote: > Hi! > > On Mon, 23 Jan 2017 10:19:33 +0100, Martin Liška <mliska@suse.cz> wrote: > > --- a/gcc/testsuite/gcc.dg/asan/use-after-scope-3.c > > +++ b/gcc/testsuite/gcc.dg/asan/use-after-scope-3.c > > @@ -1,5 +1,6 @@ > > // { dg-do run } > > // { dg-shouldfail "asan" } > > +// { dg-additional-options "-O0" } > > As these tests per "gcc/testsuite/gcc.dg/asan/asan.exp" are run with > "gcc-dg-runtest", which will "cycle through a list of optimization > options as c-torture does", is it really appropriate to hard-code "-O0" > here? Shouldn't instead be all testing be "dg-skip"ped (or similar) > unless "-O0" is in effect? Indeed, I see UNRESOLVED: gcc.dg/asan/use-after-scope-9.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects scan-tree-dump-times asan1 "= ASAN_POISON \\\\(\\\\)" 1 too. For that kind of thing, the standard way is to add -ffat-lto-objects. As for -O* in */asan/* tests, that is indeed a bug, most tests do it right. The following patch should fix that, ok for trunk? 2017-01-26 Jakub Jelinek <jakub@redhat.com> * c-c++-common/asan/pr63316.c: Use dg-skip-if instead of dg-options. * c-c++-common/asan/misalign-1.c: Likewise. * c-c++-common/asan/misalign-2.c: Likewise. * g++.dg/asan/pr69276.C: Add dg-skip-if, remove -O0 from dg-additional-options. * gcc.dg/asan/pr66314.c: Remove -Os from dg-options, add dg-skip-if. * gcc.dg/asan/use-after-scope-3.c: Use dg-skip-if instead of dg-options. * gcc.dg/asan/use-after-scope-9.c: Add dg-skip-if, remove -O2 and add -ffat-lto-objects from/to dg-additional-options. * gcc.dg/asan/use-after-scope-10.c: Add dg-skip-if, remove -O2 from dg-additional-options. Jakub--- gcc/testsuite/c-c++-common/asan/pr63316.c.jj 2014-09-24 11:13:43.000000000 +0200 +++ gcc/testsuite/c-c++-common/asan/pr63316.c 2017-01-26 11:38:32.172060026 +0100 @@ -1,6 +1,6 @@ /* PR sanitizer/63316 */ /* { dg-do run } */ -/* { dg-options "-fsanitize=address -O2" } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */ #ifdef __cplusplus extern "C" { --- gcc/testsuite/c-c++-common/asan/misalign-1.c.jj 2014-11-20 08:32:14.000000000 +0100 +++ gcc/testsuite/c-c++-common/asan/misalign-1.c 2017-01-26 11:37:57.964508495 +0100 @@ -1,5 +1,5 @@ /* { dg-do run { target { ilp32 || lp64 } } } */ -/* { dg-options "-O2" } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */ /* { dg-additional-options "-fno-omit-frame-pointer" { target *-*-darwin* } } */ /* { dg-shouldfail "asan" } */ --- gcc/testsuite/c-c++-common/asan/misalign-2.c.jj 2014-11-20 08:32:14.000000000 +0100 +++ gcc/testsuite/c-c++-common/asan/misalign-2.c 2017-01-26 11:38:06.756393231 +0100 @@ -1,5 +1,5 @@ /* { dg-do run { target { ilp32 || lp64 } } } */ -/* { dg-options "-O2" } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } */ /* { dg-additional-options "-fno-omit-frame-pointer" { target *-*-darwin* } } */ /* { dg-shouldfail "asan" } */ --- gcc/testsuite/g++.dg/asan/pr69276.C.jj 2016-02-04 23:14:18.000000000 +0100 +++ gcc/testsuite/g++.dg/asan/pr69276.C 2017-01-26 11:40:10.490771046 +0100 @@ -1,6 +1,7 @@ /* { dg-do run } */ /* { dg-shouldfail "asan" } */ -/* { dg-additional-options "-O0 -fno-lto" } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ +/* { dg-additional-options "-fno-lto" } */ #include <stdlib.h> --- gcc/testsuite/gcc.dg/asan/pr66314.c.jj 2015-08-24 18:26:58.000000000 +0200 +++ gcc/testsuite/gcc.dg/asan/pr66314.c 2017-01-26 11:31:13.234814878 +0100 @@ -1,5 +1,6 @@ /* { dg-do compile } */ -/* { dg-options "-std=gnu89 -Os -fprofile-arcs -fno-sanitize=all -fsanitize=kernel-address" } */ +/* { dg-options "-std=gnu89 -fprofile-arcs -fno-sanitize=all -fsanitize=kernel-address" } */ +/* { dg-skip-if "" { *-*-* } { "*" } { "-Os" } } */ char *a; int d; --- gcc/testsuite/gcc.dg/asan/use-after-scope-3.c.jj 2017-01-23 18:09:36.000000000 +0100 +++ gcc/testsuite/gcc.dg/asan/use-after-scope-3.c 2017-01-26 11:36:56.510314173 +0100 @@ -1,6 +1,6 @@ // { dg-do run } // { dg-shouldfail "asan" } -// { dg-additional-options "-O0" } +// { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } int main (void) --- gcc/testsuite/gcc.dg/asan/use-after-scope-9.c.jj 2017-01-23 18:09:36.000000000 +0100 +++ gcc/testsuite/gcc.dg/asan/use-after-scope-9.c 2017-01-26 11:37:03.891217408 +0100 @@ -1,6 +1,7 @@ // { dg-do run } // { dg-shouldfail "asan" } -// { dg-additional-options "-O2 -fdump-tree-asan1" } +// { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } +// { dg-additional-options "-fdump-tree-asan1 -ffat-lto-objects" } int main (int argc, char **argv) --- gcc/testsuite/gcc.dg/asan/use-after-scope-10.c.jj 2017-01-23 18:09:36.000000000 +0100 +++ gcc/testsuite/gcc.dg/asan/use-after-scope-10.c 2017-01-26 11:36:43.924479176 +0100 @@ -1,6 +1,7 @@ // { dg-do run } // { dg-shouldfail "asan" } -// { dg-additional-options "-O2 -fdump-tree-asan1" } +// { dg-skip-if "" { *-*-* } { "*" } { "-O2" } } +// { dg-additional-options "-fdump-tree-asan1" } int main (int argc, char **argv)
Hi! On Thu, 26 Jan 2017 11:54:07 +0100, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Jan 26, 2017 at 09:57:14AM +0100, Thomas Schwinge wrote: > > On Mon, 23 Jan 2017 10:19:33 +0100, Martin Liška <mliska@suse.cz> wrote: > > > --- a/gcc/testsuite/gcc.dg/asan/use-after-scope-3.c > > > +++ b/gcc/testsuite/gcc.dg/asan/use-after-scope-3.c > > > @@ -1,5 +1,6 @@ > > > // { dg-do run } > > > // { dg-shouldfail "asan" } > > > +// { dg-additional-options "-O0" } > > > > As these tests per "gcc/testsuite/gcc.dg/asan/asan.exp" are run with > > "gcc-dg-runtest", which will "cycle through a list of optimization > > options as c-torture does", is it really appropriate to hard-code "-O0" > > here? Shouldn't instead be all testing be "dg-skip"ped (or similar) > > unless "-O0" is in effect? > > Indeed, I see > UNRESOLVED: gcc.dg/asan/use-after-scope-9.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects scan-tree-dump-times asan1 "= ASAN_POISON \\\\(\\\\)" 1 > too. > > For that kind of thing, the standard way is to add -ffat-lto-objects. > As for -O* in */asan/* tests, that is indeed a bug, most tests do it right. > > The following patch should fix that, ok for trunk? Looks good to me (but I can't approve it, as you know). One additional comment: > --- gcc/testsuite/g++.dg/asan/pr69276.C.jj 2016-02-04 23:14:18.000000000 +0100 > +++ gcc/testsuite/g++.dg/asan/pr69276.C 2017-01-26 11:40:10.490771046 +0100 > @@ -1,6 +1,7 @@ > /* { dg-do run } */ > /* { dg-shouldfail "asan" } */ > -/* { dg-additional-options "-O0 -fno-lto" } */ > +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ > +/* { dg-additional-options "-fno-lto" } */ Probably can get rid of that "-fno-lto", too, as "-flto" is not used together with "-O0"? Grüße Thomas
On Thu, Jan 26, 2017 at 09:44:14PM +0100, Thomas Schwinge wrote: > > The following patch should fix that, ok for trunk? > > Looks good to me (but I can't approve it, as you know). Sure, I know. > > One additional comment: > > > --- gcc/testsuite/g++.dg/asan/pr69276.C.jj 2016-02-04 23:14:18.000000000 +0100 > > +++ gcc/testsuite/g++.dg/asan/pr69276.C 2017-01-26 11:40:10.490771046 +0100 > > @@ -1,6 +1,7 @@ > > /* { dg-do run } */ > > /* { dg-shouldfail "asan" } */ > > -/* { dg-additional-options "-O0 -fno-lto" } */ > > +/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ > > +/* { dg-additional-options "-fno-lto" } */ > > Probably can get rid of that "-fno-lto", too, as "-flto" is not used > together with "-O0"? Yeah, or: /* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */ /* { dg-skip-if "" { *-*-* } { "-flto" } { "" } } */ Jakub
From 66fabb9d15ebfb21e25b4fc81bad8deb6877e198 Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Mon, 19 Dec 2016 15:36:11 +0100 Subject: [PATCH] Speed up use-after-scope (v2): rewrite into SSA gcc/ChangeLog: 2016-12-22 Martin Liska <mliska@suse.cz> * asan.c (create_asan_shadow_var): New function. (asan_expand_poison_ifn): Likewise. * asan.h (asan_expand_poison_ifn): New declaration. * internal-fn.c (expand_ASAN_POISON): Likewise. * internal-fn.def (ASAN_POISON): New builtin. * sanopt.c (pass_sanopt::execute): Expand asan_expand_poison_ifn. * tree-inline.c (copy_decl_for_dup_finish): Make function external. * tree-inline.h (copy_decl_for_dup_finish): Likewise. * tree-ssa.c (is_asan_mark_p): New function. (execute_update_addresses_taken): Rewrite local variables (identified just by use-after-scope as addressable) into SSA. gcc/testsuite/ChangeLog: 2016-12-22 Martin Liska <mliska@suse.cz> * gcc.dg/asan/use-after-scope-3.c: Add additional flags. * gcc.dg/asan/use-after-scope-9.c: Likewise and grep for sanopt optimization for ASAN_POISON. --- gcc/asan.c | 109 +++++++++++++++++++++++++- gcc/asan.h | 2 + gcc/internal-fn.c | 7 ++ gcc/internal-fn.def | 1 + gcc/sanopt.c | 11 +++ gcc/testsuite/gcc.dg/asan/use-after-scope-3.c | 1 + gcc/testsuite/gcc.dg/asan/use-after-scope-9.c | 2 + gcc/tree-inline.c | 2 +- gcc/tree-inline.h | 1 + gcc/tree-ssa.c | 69 +++++++++++++--- 10 files changed, 193 insertions(+), 12 deletions(-) diff --git a/gcc/asan.c b/gcc/asan.c index 53acff0a2fb..187934ad11b 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -32,8 +32,8 @@ along with GCC; see the file COPYING3. If not see #include "tree-pass.h" #include "memmodel.h" #include "tm_p.h" +#include "ssa.h" #include "stringpool.h" -#include "tree-vrp.h" #include "tree-ssanames.h" #include "optabs.h" #include "emit-rtl.h" @@ -59,6 +59,7 @@ along with GCC; see the file COPYING3. If not see #include "params.h" #include "builtins.h" #include "fnmatch.h" +#include "tree-inline.h" /* AddressSanitizer finds out-of-bounds and use-after-free bugs with <2x slowdown on average. @@ -3055,6 +3056,112 @@ asan_expand_check_ifn (gimple_stmt_iterator *iter, bool use_calls) return true; } +/* Create ASAN shadow variable for a VAR_DECL which has been rewritten + into SSA. Already seen VAR_DECLs are stored in SHADOW_VARS_MAPPING. */ + +static tree +create_asan_shadow_var (tree var_decl, + hash_map<tree, tree> &shadow_vars_mapping) +{ + tree *slot = shadow_vars_mapping.get (var_decl); + if (slot == NULL) + { + tree shadow_var = copy_node (var_decl); + + copy_body_data id; + memset (&id, 0, sizeof (copy_body_data)); + id.src_fn = id.dst_fn = current_function_decl; + copy_decl_for_dup_finish (&id, var_decl, shadow_var); + + DECL_ARTIFICIAL (shadow_var) = 1; + DECL_IGNORED_P (shadow_var) = 1; + DECL_SEEN_IN_BIND_EXPR_P (shadow_var) = 0; + gimple_add_tmp_var (shadow_var); + + shadow_vars_mapping.put (var_decl, shadow_var); + return shadow_var; + } + else + return *slot; +} + +bool +asan_expand_poison_ifn (gimple_stmt_iterator *iter, + bool *need_commit_edge_insert, + hash_map<tree, tree> &shadow_vars_mapping) +{ + gimple *g = gsi_stmt (*iter); + tree poisoned_var = gimple_call_lhs (g); + if (!poisoned_var) + { + gsi_remove (iter, true); + return true; + } + + tree shadow_var = create_asan_shadow_var (SSA_NAME_VAR (poisoned_var), + shadow_vars_mapping); + + bool recover_p; + if (flag_sanitize & SANITIZE_USER_ADDRESS) + recover_p = (flag_sanitize_recover & SANITIZE_USER_ADDRESS) != 0; + else + recover_p = (flag_sanitize_recover & SANITIZE_KERNEL_ADDRESS) != 0; + tree size = DECL_SIZE_UNIT (shadow_var); + gimple *poison_call + = gimple_build_call_internal (IFN_ASAN_MARK, 3, + build_int_cst (integer_type_node, + ASAN_MARK_POISON), + build_fold_addr_expr (shadow_var), size); + + use_operand_p use_p; + imm_use_iterator imm_iter; + FOR_EACH_IMM_USE_FAST (use_p, imm_iter, poisoned_var) + { + gimple *use = USE_STMT (use_p); + if (is_gimple_debug (use)) + continue; + + int nargs; + tree fun = report_error_func (false, recover_p, tree_to_uhwi (size), + &nargs); + + gcall *call = gimple_build_call (fun, 1, + build_fold_addr_expr (shadow_var)); + gimple_set_location (call, gimple_location (use)); + gimple *call_to_insert = call; + + /* The USE can be a gimple PHI node. If so, insert the call on + all edges leading to the PHI node. */ + if (is_a <gphi *> (use)) + { + gphi *phi = dyn_cast<gphi *> (use); + for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i) + if (gimple_phi_arg_def (phi, i) == poisoned_var) + { + edge e = gimple_phi_arg_edge (phi, i); + + if (call_to_insert == NULL) + call_to_insert = gimple_copy (call); + + gsi_insert_seq_on_edge (e, call_to_insert); + *need_commit_edge_insert = true; + call_to_insert = NULL; + } + } + else + { + gimple_stmt_iterator gsi = gsi_for_stmt (use); + gsi_insert_before (&gsi, call, GSI_NEW_STMT); + } + } + + SSA_NAME_IS_DEFAULT_DEF (poisoned_var) = true; + SSA_NAME_DEF_STMT (poisoned_var) = gimple_build_nop (); + gsi_replace (iter, poison_call, false); + + return true; +} + /* Instrument the current function. */ static unsigned int diff --git a/gcc/asan.h b/gcc/asan.h index 355a350bfeb..3800aabaf8b 100644 --- a/gcc/asan.h +++ b/gcc/asan.h @@ -30,6 +30,8 @@ extern void initialize_sanitizer_builtins (void); extern tree asan_dynamic_init_call (bool); extern bool asan_expand_check_ifn (gimple_stmt_iterator *, bool); extern bool asan_expand_mark_ifn (gimple_stmt_iterator *); +extern bool asan_expand_poison_ifn (gimple_stmt_iterator *, bool *, + hash_map<tree, tree> &); extern gimple_stmt_iterator create_cond_insert_point (gimple_stmt_iterator *, bool, bool, bool, basic_block *, basic_block *); diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c index b1dbc988b9c..40f5fe6c69c 100644 --- a/gcc/internal-fn.c +++ b/gcc/internal-fn.c @@ -380,6 +380,13 @@ expand_ASAN_MARK (internal_fn, gcall *) gcc_unreachable (); } +/* This should get expanded in the sanopt pass. */ + +static void +expand_ASAN_POISON (internal_fn, gcall *) +{ + gcc_unreachable (); +} /* This should get expanded in the tsan pass. */ diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def index 9a03e17be23..81a6bbdd15c 100644 --- a/gcc/internal-fn.def +++ b/gcc/internal-fn.def @@ -167,6 +167,7 @@ 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_POISON, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL) DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (SUB_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (MUL_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) diff --git a/gcc/sanopt.c b/gcc/sanopt.c index ae716cffcf4..a19c3a1b6dd 100644 --- a/gcc/sanopt.c +++ b/gcc/sanopt.c @@ -894,6 +894,8 @@ pass_sanopt::execute (function *fun) bool use_calls = ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD < INT_MAX && asan_num_accesses >= ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD; + hash_map<tree, tree> shadow_vars_mapping; + bool need_commit_edge_insert = false; FOR_EACH_BB_FN (bb, fun) { gimple_stmt_iterator gsi; @@ -931,6 +933,11 @@ pass_sanopt::execute (function *fun) case IFN_ASAN_MARK: no_next = asan_expand_mark_ifn (&gsi); break; + case IFN_ASAN_POISON: + no_next = asan_expand_poison_ifn (&gsi, + &need_commit_edge_insert, + shadow_vars_mapping); + break; default: break; } @@ -962,6 +969,10 @@ pass_sanopt::execute (function *fun) gsi_next (&gsi); } } + + if (need_commit_edge_insert) + gsi_commit_edge_inserts (); + return 0; } diff --git a/gcc/testsuite/gcc.dg/asan/use-after-scope-3.c b/gcc/testsuite/gcc.dg/asan/use-after-scope-3.c index 9aeed51a770..8b11bea9940 100644 --- a/gcc/testsuite/gcc.dg/asan/use-after-scope-3.c +++ b/gcc/testsuite/gcc.dg/asan/use-after-scope-3.c @@ -1,5 +1,6 @@ // { dg-do run } // { dg-shouldfail "asan" } +// { dg-additional-options "-O0" } int main (void) diff --git a/gcc/testsuite/gcc.dg/asan/use-after-scope-9.c b/gcc/testsuite/gcc.dg/asan/use-after-scope-9.c index 2e30deffa18..5d069dd18ea 100644 --- a/gcc/testsuite/gcc.dg/asan/use-after-scope-9.c +++ b/gcc/testsuite/gcc.dg/asan/use-after-scope-9.c @@ -1,5 +1,6 @@ // { dg-do run } // { dg-shouldfail "asan" } +// { dg-additional-options "-O2 -fdump-tree-asan1" } int main (int argc, char **argv) @@ -15,6 +16,7 @@ main (int argc, char **argv) return *ptr; } +// { dg-final { scan-tree-dump-times "= ASAN_POISON \\(\\)" 1 "asan1" } } // { dg-output "ERROR: AddressSanitizer: stack-use-after-scope on address.*(\n|\r\n|\r)" } // { dg-output "READ of size .*" } // { dg-output ".*'a' <== Memory access at offset \[0-9\]* is inside this variable.*" } diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 0de0b89dbbf..7666320ec00 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -5446,7 +5446,7 @@ declare_inline_vars (tree block, tree vars) but now it will be in the TO_FN. PARM_TO_VAR means enable PARM_DECL to VAR_DECL translation. */ -static tree +tree copy_decl_for_dup_finish (copy_body_data *id, tree decl, tree copy) { /* Don't generate debug information for the copy if we wouldn't have diff --git a/gcc/tree-inline.h b/gcc/tree-inline.h index 9ca2a91f08f..3e55a0ae0e5 100644 --- a/gcc/tree-inline.h +++ b/gcc/tree-inline.h @@ -218,6 +218,7 @@ extern gimple_seq copy_gimple_seq_and_replace_locals (gimple_seq seq); extern bool debug_find_tree (tree, tree); extern tree copy_fn (tree, tree&, tree&); extern const char *copy_forbidden (struct function *fun); +extern tree copy_decl_for_dup_finish (copy_body_data *id, tree decl, tree copy); /* This is in tree-inline.c since the routine uses data structures from the inliner. */ diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c index 62eea8bb8a4..3adbef48037 100644 --- a/gcc/tree-ssa.c +++ b/gcc/tree-ssa.c @@ -41,6 +41,7 @@ along with GCC; see the file COPYING3. If not see #include "cfgexpand.h" #include "tree-cfg.h" #include "tree-dfa.h" +#include "asan.h" /* Pointer map of variable mappings, keyed by edge. */ static hash_map<edge, auto_vec<edge_var_map> > *edge_var_maps; @@ -1550,6 +1551,30 @@ maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs, } } +/* Return true when STMT is ASAN mark where second argument is an address + of a local variable. */ + +static bool +is_asan_mark_p (gimple *stmt) +{ + if (!gimple_call_internal_p (stmt, IFN_ASAN_MARK)) + return false; + + tree addr = get_base_address (gimple_call_arg (stmt, 1)); + if (TREE_CODE (addr) == ADDR_EXPR + && VAR_P (TREE_OPERAND (addr, 0))) + { + tree var = TREE_OPERAND (addr, 0); + unsigned addressable = TREE_ADDRESSABLE (var); + TREE_ADDRESSABLE (var) = 0; + bool r = is_gimple_reg (var); + TREE_ADDRESSABLE (var) = addressable; + return r; + } + + return false; +} + /* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables. */ void @@ -1575,17 +1600,23 @@ execute_update_addresses_taken (void) enum gimple_code code = gimple_code (stmt); tree decl; - if (code == GIMPLE_CALL - && optimize_atomic_compare_exchange_p (stmt)) + if (code == GIMPLE_CALL) { - /* For __atomic_compare_exchange_N if the second argument - is &var, don't mark var addressable; - if it becomes non-addressable, we'll rewrite it into - ATOMIC_COMPARE_EXCHANGE call. */ - tree arg = gimple_call_arg (stmt, 1); - gimple_call_set_arg (stmt, 1, null_pointer_node); - gimple_ior_addresses_taken (addresses_taken, stmt); - gimple_call_set_arg (stmt, 1, arg); + if (optimize_atomic_compare_exchange_p (stmt)) + { + /* For __atomic_compare_exchange_N if the second argument + is &var, don't mark var addressable; + if it becomes non-addressable, we'll rewrite it into + ATOMIC_COMPARE_EXCHANGE call. */ + tree arg = gimple_call_arg (stmt, 1); + gimple_call_set_arg (stmt, 1, null_pointer_node); + gimple_ior_addresses_taken (addresses_taken, stmt); + gimple_call_set_arg (stmt, 1, arg); + } + else if (is_asan_mark_p (stmt)) + ; + else + gimple_ior_addresses_taken (addresses_taken, stmt); } else /* Note all addresses taken by the stmt. */ @@ -1841,6 +1872,24 @@ execute_update_addresses_taken (void) continue; } } + else if (is_asan_mark_p (stmt)) + { + tree var = TREE_OPERAND (gimple_call_arg (stmt, 1), 0); + if (bitmap_bit_p (suitable_for_renaming, DECL_UID (var))) + { + unlink_stmt_vdef (stmt); + if (asan_mark_p (stmt, ASAN_MARK_POISON)) + { + gcall *call + = gimple_build_call_internal (IFN_ASAN_POISON, 0); + gimple_call_set_lhs (call, var); + gsi_replace (&gsi, call, GSI_SAME_STMT); + } + else + gsi_remove (&gsi, true); + continue; + } + } for (i = 0; i < gimple_call_num_args (stmt); ++i) { tree *argp = gimple_call_arg_ptr (stmt, i); -- 2.11.0