| Message ID | 606cd948-6cba-02a4-f114-35900ab53203@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Nov 01, 2016 at 03:53:46PM +0100, Martin Liška wrote: > @@ -1504,7 +1505,7 @@ non_rewritable_lvalue_p (tree lhs) > > static void > maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs, > - bitmap suitable_for_renaming) > + bitmap suitable_for_renaming, bitmap marked_nonaddressable) > { > /* Global Variables, result decls cannot be changed. */ > if (is_global_var (var) > @@ -1522,6 +1523,7 @@ maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs, > || !bitmap_bit_p (not_reg_needs, DECL_UID (var)))) > { > TREE_ADDRESSABLE (var) = 0; > + bitmap_set_bit (marked_nonaddressable, DECL_UID (var)); Why do you need the marked_nonaddressable bitmap? > if (is_gimple_reg (var)) > bitmap_set_bit (suitable_for_renaming, DECL_UID (var)); > if (dump_file) > @@ -1550,20 +1552,43 @@ maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs, > } > } > > -/* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables. */ > +/* Return true when STMT is ASAN mark where second argument is an address > + of a local variable. */ > > -void > -execute_update_addresses_taken (void) > +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 > + && TREE_CODE (TREE_OPERAND (addr, 0)) == VAR_DECL) Just check here if dropping TREE_ADDRESSABLE from the VAR (use VAR_P btw) would turn it into is_gimple_reg), and don't return true if not. > + return true; > + > + return false; > +} > + > +/* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables. > + If SANITIZE_ASAN_MARK is set to true, sanitize also ASAN_MARK built-ins. */ > + > + > +static void > +execute_update_addresses_taken (bool sanitize_asan_mark = false) I wonder if the sanitize_asan_mark wouldn't better be some PROP_* property set during the asan pass and kept on until end of compilation of that function. That means even if a var only addressable because of ASAN_MARK is discovered after this pass we'd still be able to rewrite it into SSA. Jakub
On Tue, Nov 1, 2016 at 4:12 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Nov 01, 2016 at 03:53:46PM +0100, Martin Liška wrote: >> @@ -1504,7 +1505,7 @@ non_rewritable_lvalue_p (tree lhs) >> >> static void >> maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs, >> - bitmap suitable_for_renaming) >> + bitmap suitable_for_renaming, bitmap marked_nonaddressable) >> { >> /* Global Variables, result decls cannot be changed. */ >> if (is_global_var (var) >> @@ -1522,6 +1523,7 @@ maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs, >> || !bitmap_bit_p (not_reg_needs, DECL_UID (var)))) >> { >> TREE_ADDRESSABLE (var) = 0; >> + bitmap_set_bit (marked_nonaddressable, DECL_UID (var)); > > Why do you need the marked_nonaddressable bitmap? > >> if (is_gimple_reg (var)) >> bitmap_set_bit (suitable_for_renaming, DECL_UID (var)); >> if (dump_file) >> @@ -1550,20 +1552,43 @@ maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs, >> } >> } >> >> -/* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables. */ >> +/* Return true when STMT is ASAN mark where second argument is an address >> + of a local variable. */ >> >> -void >> -execute_update_addresses_taken (void) >> +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 >> + && TREE_CODE (TREE_OPERAND (addr, 0)) == VAR_DECL) > > Just check here if dropping TREE_ADDRESSABLE from the VAR (use VAR_P btw) > would turn it into is_gimple_reg), and don't return true if not. > >> + return true; >> + >> + return false; >> +} >> + >> +/* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables. >> + If SANITIZE_ASAN_MARK is set to true, sanitize also ASAN_MARK built-ins. */ >> + >> + >> +static void >> +execute_update_addresses_taken (bool sanitize_asan_mark = false) > > I wonder if the sanitize_asan_mark wouldn't better be some PROP_* property > set during the asan pass and kept on until end of compilation of that > function. That means even if a var only addressable because of ASAN_MARK is > discovered after this pass we'd still be able to rewrite it into SSA. Note that being TREE_ADDRESSABLE also has effects on alias analysis (didn't follow the patches to see whether you handle ASAN_MARK specially in points-to analysis and/or alias analysis). Generally in update-address-taken you can handle ASAN_MARK similar to MEM_REF (and drop it in the rewrite phase?). As said, I didnt look at the patches and just came by here seeing tree-ssa.c pieces... Richard. > Jakub
On 11/02/2016 10:40 AM, Richard Biener wrote: > On Tue, Nov 1, 2016 at 4:12 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Tue, Nov 01, 2016 at 03:53:46PM +0100, Martin Liška wrote: >>> @@ -1504,7 +1505,7 @@ non_rewritable_lvalue_p (tree lhs) >>> >>> static void >>> maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs, >>> - bitmap suitable_for_renaming) >>> + bitmap suitable_for_renaming, bitmap marked_nonaddressable) >>> { >>> /* Global Variables, result decls cannot be changed. */ >>> if (is_global_var (var) >>> @@ -1522,6 +1523,7 @@ maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs, >>> || !bitmap_bit_p (not_reg_needs, DECL_UID (var)))) >>> { >>> TREE_ADDRESSABLE (var) = 0; >>> + bitmap_set_bit (marked_nonaddressable, DECL_UID (var)); >> >> Why do you need the marked_nonaddressable bitmap? >> >>> if (is_gimple_reg (var)) >>> bitmap_set_bit (suitable_for_renaming, DECL_UID (var)); >>> if (dump_file) >>> @@ -1550,20 +1552,43 @@ maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs, >>> } >>> } >>> >>> -/* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables. */ >>> +/* Return true when STMT is ASAN mark where second argument is an address >>> + of a local variable. */ >>> >>> -void >>> -execute_update_addresses_taken (void) >>> +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 >>> + && TREE_CODE (TREE_OPERAND (addr, 0)) == VAR_DECL) >> >> Just check here if dropping TREE_ADDRESSABLE from the VAR (use VAR_P btw) >> would turn it into is_gimple_reg), and don't return true if not. >> >>> + return true; >>> + >>> + return false; >>> +} >>> + >>> +/* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables. >>> + If SANITIZE_ASAN_MARK is set to true, sanitize also ASAN_MARK built-ins. */ >>> + >>> + >>> +static void >>> +execute_update_addresses_taken (bool sanitize_asan_mark = false) >> >> I wonder if the sanitize_asan_mark wouldn't better be some PROP_* property >> set during the asan pass and kept on until end of compilation of that >> function. That means even if a var only addressable because of ASAN_MARK is >> discovered after this pass we'd still be able to rewrite it into SSA. > > Note that being TREE_ADDRESSABLE also has effects on alias analysis > (didn't follow the patches to see whether you handle ASAN_MARK specially > in points-to analysis and/or alias analysis). Currently all manipulation with shadow memory is done via a pointer type which has created a separate aliasing set: static void asan_init_shadow_ptr_types (void) { asan_shadow_set = new_alias_set (); tree types[3] = { signed_char_type_node, short_integer_type_node, integer_type_node }; for (unsigned i = 0; i < 3; i++) { shadow_ptr_types[i] = build_distinct_type_copy (types[i]); TYPE_ALIAS_SET (shadow_ptr_types[i]) = asan_shadow_set; shadow_ptr_types[i] = build_pointer_type (shadow_ptr_types[i]); } ... Martin > > Generally in update-address-taken you can handle ASAN_MARK similar to > MEM_REF (and drop it in the rewrite phase?). > > As said, I didnt look at the patches and just came by here seeing > tree-ssa.c pieces... > > Richard. > >> Jakub
On Wed, Nov 02, 2016 at 10:40:35AM +0100, Richard Biener wrote: > > I wonder if the sanitize_asan_mark wouldn't better be some PROP_* property > > set during the asan pass and kept on until end of compilation of that > > function. That means even if a var only addressable because of ASAN_MARK is > > discovered after this pass we'd still be able to rewrite it into SSA. > > Note that being TREE_ADDRESSABLE also has effects on alias analysis > (didn't follow the patches to see whether you handle ASAN_MARK specially > in points-to analysis and/or alias analysis). > > Generally in update-address-taken you can handle ASAN_MARK similar to > MEM_REF (and drop it in the rewrite phase?). That is the intent, but we can't do that before the asan pass, because otherwise as Martin explained we don't diagnose at runtime bugs where a variable is used outside of its scope only through a MEM_REF. So we need to wait for asan pass to actually add a real builtin call that takes the address in that case. Except now I really don't see how that can work for the case where the var is used only properly when it is in the scope, because the asan pass will still see those being addressable. Unless I'm missing something we either need to perform further analysis during the addressable subpass - this variable could be made non-addressable, but is used in ASAN_MARK, would if we rewrote it into SSA form there be any SSA uses of the poisoning ASAN_MARK? If yes, keep it addressable, otherwise rewrite into SSA. Or, just rewrite it into SSA always, but turn the poisoning ASAN_MARK into some new internal ECF_CONST call var_5 = ASAN_POISON (); and if we have any uses of those, rewrite it back into addressable immediately or later or something. Jakub
On Wed, Nov 2, 2016 at 10:52 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Nov 02, 2016 at 10:40:35AM +0100, Richard Biener wrote: >> > I wonder if the sanitize_asan_mark wouldn't better be some PROP_* property >> > set during the asan pass and kept on until end of compilation of that >> > function. That means even if a var only addressable because of ASAN_MARK is >> > discovered after this pass we'd still be able to rewrite it into SSA. >> >> Note that being TREE_ADDRESSABLE also has effects on alias analysis >> (didn't follow the patches to see whether you handle ASAN_MARK specially >> in points-to analysis and/or alias analysis). >> >> Generally in update-address-taken you can handle ASAN_MARK similar to >> MEM_REF (and drop it in the rewrite phase?). > > That is the intent, but we can't do that before the asan pass, because > otherwise as Martin explained we don't diagnose at runtime bugs where > a variable is used outside of its scope only through a MEM_REF. > So we need to wait for asan pass to actually add a real builtin call that > takes the address in that case. Except now I really don't see how that > can work for the case where the var is used only properly when it is in the > scope, because the asan pass will still see those being addressable. > > Unless I'm missing something we either need to perform further analysis > during the addressable subpass - this variable could be made > non-addressable, but is used in ASAN_MARK, would if we rewrote it into SSA > form there be any SSA uses of the poisoning ASAN_MARK? If yes, keep it > addressable, otherwise rewrite into SSA. > Or, just rewrite it into SSA always, but turn the poisoning ASAN_MARK into > some new internal ECF_CONST call var_5 = ASAN_POISON (); and if we have any > uses of those, rewrite it back into addressable immediately or later or > something. Or just give up optimizing (asan has a penalty anyway)? Or re-structure ASAN_POISON () similar to clobbers: var = ASAN_POISION (); that avoids the address-taking and thus should be less heavy-weight on optimizations. Richard. > > Jakub
On Wed, Nov 02, 2016 at 01:36:31PM +0100, Richard Biener wrote: > > Unless I'm missing something we either need to perform further analysis > > during the addressable subpass - this variable could be made > > non-addressable, but is used in ASAN_MARK, would if we rewrote it into SSA > > form there be any SSA uses of the poisoning ASAN_MARK? If yes, keep it > > addressable, otherwise rewrite into SSA. > > Or, just rewrite it into SSA always, but turn the poisoning ASAN_MARK into > > some new internal ECF_CONST call var_5 = ASAN_POISON (); and if we have any > > uses of those, rewrite it back into addressable immediately or later or > > something. > > Or just give up optimizing (asan has a penalty anyway)? Or Well, asan has a penalty and -fsanitize-use-after-scope even bigger penalty, but the point is to make that penalty bearable. > re-structure ASAN_POISON () > similar to clobbers: > > var = ASAN_POISION (); > > that avoids the address-taking and thus should be less heavy-weight on > optimizations. Yeah, that is what I meant. The issue is how to report uses of such SSA_NAME when there is no memory. So, either we'd need a special runtime library entrypoint that would report uses after scope even when there is no underlying memory, or we'd need to force it at asan pass time into memory again. Jakub
On Wed, Nov 2, 2016 at 1:56 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Nov 02, 2016 at 01:36:31PM +0100, Richard Biener wrote: >> > Unless I'm missing something we either need to perform further analysis >> > during the addressable subpass - this variable could be made >> > non-addressable, but is used in ASAN_MARK, would if we rewrote it into SSA >> > form there be any SSA uses of the poisoning ASAN_MARK? If yes, keep it >> > addressable, otherwise rewrite into SSA. >> > Or, just rewrite it into SSA always, but turn the poisoning ASAN_MARK into >> > some new internal ECF_CONST call var_5 = ASAN_POISON (); and if we have any >> > uses of those, rewrite it back into addressable immediately or later or >> > something. >> >> Or just give up optimizing (asan has a penalty anyway)? Or > > Well, asan has a penalty and -fsanitize-use-after-scope even bigger penalty, > but the point is to make that penalty bearable. > >> re-structure ASAN_POISON () >> similar to clobbers: >> >> var = ASAN_POISION (); >> >> that avoids the address-taking and thus should be less heavy-weight on >> optimizations. > > Yeah, that is what I meant. The issue is how to report uses of such > SSA_NAME when there is no memory. So, either we'd need a special runtime > library entrypoint that would report uses after scope even when there is no > underlying memory, or we'd need to force it at asan pass time into memory again. Well, there can't be any uses outside the scope -- there are no (memory) uses left if we rewrite the thing into SSA. That is, the address can no longer "escape". Of course there could have been invalid uses before the rewrite into SSA. But those can be diagnosed either immediately before or after re-writing into SSA at compile-time (may be in dead code regions of course). Richard. > Jakub
On Wed, Nov 02, 2016 at 01:59:00PM +0100, Richard Biener wrote: > > Yeah, that is what I meant. The issue is how to report uses of such > > SSA_NAME when there is no memory. So, either we'd need a special runtime > > library entrypoint that would report uses after scope even when there is no > > underlying memory, or we'd need to force it at asan pass time into memory again. > > Well, there can't be any uses outside the scope -- there are no (memory) uses > left if we rewrite the thing into SSA. That is, the address can no > longer "escape". > > Of course there could have been invalid uses before the rewrite into SSA. But > those can be diagnosed either immediately before or after re-writing into SSA > at compile-time (may be in dead code regions of course). Sure, we can warn on those at compile time, but we really should arrange to error on those at runtime if they are ever executed, the UB happens only at runtime, so in dead code isn't fatal. Jakub
On Wed, Nov 2, 2016 at 2:06 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Nov 02, 2016 at 01:59:00PM +0100, Richard Biener wrote: >> > Yeah, that is what I meant. The issue is how to report uses of such >> > SSA_NAME when there is no memory. So, either we'd need a special runtime >> > library entrypoint that would report uses after scope even when there is no >> > underlying memory, or we'd need to force it at asan pass time into memory again. >> >> Well, there can't be any uses outside the scope -- there are no (memory) uses >> left if we rewrite the thing into SSA. That is, the address can no >> longer "escape". >> >> Of course there could have been invalid uses before the rewrite into SSA. But >> those can be diagnosed either immediately before or after re-writing into SSA >> at compile-time (may be in dead code regions of course). > > Sure, we can warn on those at compile time, but we really should arrange to > error on those at runtime if they are ever executed, the UB happens only at > runtime, so in dead code isn't fatal. Then we can replace those uses with a call into the asan runtime diagnosing the issue instead? Richard. > Jakub
On 11/02/2016 02:16 PM, Richard Biener wrote: > On Wed, Nov 2, 2016 at 2:06 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Wed, Nov 02, 2016 at 01:59:00PM +0100, Richard Biener wrote: >>>> Yeah, that is what I meant. The issue is how to report uses of such >>>> SSA_NAME when there is no memory. So, either we'd need a special runtime >>>> library entrypoint that would report uses after scope even when there is no >>>> underlying memory, or we'd need to force it at asan pass time into memory again. >>> >>> Well, there can't be any uses outside the scope -- there are no (memory) uses >>> left if we rewrite the thing into SSA. That is, the address can no >>> longer "escape". >>> >>> Of course there could have been invalid uses before the rewrite into SSA. But >>> those can be diagnosed either immediately before or after re-writing into SSA >>> at compile-time (may be in dead code regions of course). >> >> Sure, we can warn on those at compile time, but we really should arrange to >> error on those at runtime if they are ever executed, the UB happens only at >> runtime, so in dead code isn't fatal. > > Then we can replace those uses with a call into the asan runtime diagnosing the > issue instead? > > Richard. > >> Jakub OK, thanks for the clarification, it's more clear to me. So we want to consider for SSA transformation of ASAN_MARK only is_gimple_reg_types. I'm having a test-case where it converts: foo () { char a; char * p; char _1; int _2; int _8; int _9; <bb 2>: ASAN_MARK (2, &a, 1); a = 0; p_6 = &a; ASAN_MARK (1, &a, 1); _1 = *p_6; if (_1 != 0) goto <bb 3>; else goto <bb 4>; <bb 3>: _9 = 1; goto <bb 5>; <bb 4>: _8 = 0; <bb 5>: # _2 = PHI <_9(3), _8(4)> return _2; } to: foo () { char a; char * p; char _1; int _2; <bb 2>: a_10 = 0; a_12 = ASAN_POISON (); _1 = a_12; if (_1 != 0) goto <bb 4>; else goto <bb 3>; <bb 3>: <bb 4>: # _2 = PHI <1(2), 0(3)> return _2; } and probably the last goal is to convert the newly added internal fn to a runtime call. Hope sanopt pass is the right place where to it? Thanks, Martin
On Wed, Nov 02, 2016 at 03:38:25PM +0100, Martin Liška wrote: > it converts: > foo () > { > char a; > char * p; > char _1; > int _2; > int _8; > int _9; > > <bb 2>: > ASAN_MARK (2, &a, 1); > a = 0; > p_6 = &a; > ASAN_MARK (1, &a, 1); > _1 = *p_6; You shouldn't convert if a is addressable (when ignoring &a in ASAN_MARK calls). Only if there is &a just in ASAN_MARK and MEM_REF, you can convert. > to: > > foo () > { > char a; > char * p; > char _1; > int _2; > > <bb 2>: > a_10 = 0; > a_12 = ASAN_POISON (); > _1 = a_12; > if (_1 != 0) > goto <bb 4>; > else > goto <bb 3>; > > <bb 3>: > > <bb 4>: > # _2 = PHI <1(2), 0(3)> > return _2; > > } > > and probably the last goal is to convert the newly added internal fn to a runtime call. > Hope sanopt pass is the right place where to it? If ASAN_POISON is ECF_CONST and has any uses during sanopt, perhaps best would be to add an artificial variable you give the same name as the underlying var of the SSA_NAME (and alignment, locus etc.) and poison it right away (keep unpoisoning only to the function epilogue) and then ASAN_CHECK replace all uses of that SSA_NAME with ASAN_CHECK + use of (D) SSA_NAME. Jakub
On 11/02/2016 03:51 PM, Jakub Jelinek wrote: > On Wed, Nov 02, 2016 at 03:38:25PM +0100, Martin Liška wrote: >> it converts: >> foo () >> { >> char a; >> char * p; >> char _1; >> int _2; >> int _8; >> int _9; >> >> <bb 2>: >> ASAN_MARK (2, &a, 1); >> a = 0; >> p_6 = &a; >> ASAN_MARK (1, &a, 1); >> _1 = *p_6; > > You shouldn't convert if a is addressable (when ignoring &a in ASAN_MARK > calls). Only if there is &a just in ASAN_MARK and MEM_REF, you can convert. Sure, which should be done in execute_update_addresses_taken via gimple_ior_addresses_taken. > >> to: >> >> foo () >> { >> char a; >> char * p; >> char _1; >> int _2; >> >> <bb 2>: >> a_10 = 0; >> a_12 = ASAN_POISON (); >> _1 = a_12; >> if (_1 != 0) >> goto <bb 4>; >> else >> goto <bb 3>; >> >> <bb 3>: >> >> <bb 4>: >> # _2 = PHI <1(2), 0(3)> >> return _2; >> >> } >> >> and probably the last goal is to convert the newly added internal fn to a runtime call. >> Hope sanopt pass is the right place where to it? > > If ASAN_POISON is ECF_CONST and has any uses during sanopt, perhaps best > would be to add an artificial variable you give the same name as the > underlying var of the SSA_NAME (and alignment, locus etc.) and poison it > right away (keep unpoisoning only to the function epilogue) and then > ASAN_CHECK replace all uses of that SSA_NAME with ASAN_CHECK + use of > (D) SSA_NAME. When I create an ASAN_POISON call in execute_update_addresses_taken, there would not be any ASAN_CHECK generated as it's going to be rewritten to SSA form (like the previous sample I sent). I like the idea of having a parallel variable, which can be poisoned at the very beginning of a function. Whenever we have a use of the SSA_NAME (like a_12 = ASAN_POISON ()), we can simply insert BUILT_IN_ASAN_REPORT_LOADx(¶llel_variable) statement. No change would be necessary for ASAN runtime in such case. Will it work? Thanks, Martin > > Jakub >
On 11/02/2016 03:51 PM, Jakub Jelinek wrote: > On Wed, Nov 02, 2016 at 03:38:25PM +0100, Martin Liška wrote: >> it converts: >> foo () >> { >> char a; >> char * p; >> char _1; >> int _2; >> int _8; >> int _9; >> >> <bb 2>: >> ASAN_MARK (2, &a, 1); >> a = 0; >> p_6 = &a; >> ASAN_MARK (1, &a, 1); >> _1 = *p_6; > > You shouldn't convert if a is addressable (when ignoring &a in ASAN_MARK > calls). Only if there is &a just in ASAN_MARK and MEM_REF, you can convert. > >> to: >> >> foo () >> { >> char a; >> char * p; >> char _1; >> int _2; >> >> <bb 2>: >> a_10 = 0; >> a_12 = ASAN_POISON (); >> _1 = a_12; >> if (_1 != 0) >> goto <bb 4>; >> else >> goto <bb 3>; >> >> <bb 3>: >> >> <bb 4>: >> # _2 = PHI <1(2), 0(3)> >> return _2; >> >> } >> >> and probably the last goal is to convert the newly added internal fn to a runtime call. >> Hope sanopt pass is the right place where to it? > > If ASAN_POISON is ECF_CONST and has any uses during sanopt, perhaps best > would be to add an artificial variable you give the same name as the > underlying var of the SSA_NAME (and alignment, locus etc.) and poison it > right away (keep unpoisoning only to the function epilogue) and then > ASAN_CHECK replace all uses of that SSA_NAME with ASAN_CHECK + use of > (D) SSA_NAME. > > Jakub > Hi. I'm having a semi-working patch that comes up with the ASAN_POISON built-in. Well, to be honest, I still have a feeling that doing the magic with the parallel variable is bit overkill. Maybe a new runtime call would make it easier for us. However, I still don't fully understand why we want to support just is_gimple_reg variables. Let's consider following test-case: void foo() { char *ptr; { char my_char[9]; ptr = &my_char[0]; } } Where I would expect to optimize out: <bb 2>: _5 = (unsigned long) 9; _4 = (unsigned long) &my_char; __builtin___asan_unpoison_stack_memory (_4, _5); _7 = (unsigned long) 9; _6 = (unsigned long) &my_char; __builtin___asan_poison_stack_memory (_6, _7); return; where address of my_char is taken in the original source code, while not during tree-ssa optimization, where the address is used only by ASAN_MARK calls. Doing such transformation can rapidly decrease number of __builtin___asan_{un}poison_stack_memory in tramp3d: from ~36K to ~22K. Thanks for clarification. Martin
Hi! FYI, I think it is much more important to get the initial patch in, so resolve the switch + declarations inside of its body stuff, add testcases for that and post for re-review and check in. These optimizations can be done even in stage3. On Thu, Nov 03, 2016 at 02:34:47PM +0100, Martin Liška wrote: > I'm having a semi-working patch that comes up with the ASAN_POISON built-in. Well, to be honest, > I still have a feeling that doing the magic with the parallel variable is bit overkill. Maybe > a new runtime call would make it easier for us. > > However, I still don't fully understand why we want to support just is_gimple_reg variables. > Let's consider following test-case: > > void foo() > { > char *ptr; > { > char my_char[9]; > ptr = &my_char[0]; > } > } > > Where I would expect to optimize out: > <bb 2>: > _5 = (unsigned long) 9; > _4 = (unsigned long) &my_char; > __builtin___asan_unpoison_stack_memory (_4, _5); > _7 = (unsigned long) 9; > _6 = (unsigned long) &my_char; > __builtin___asan_poison_stack_memory (_6, _7); > return; > > where address of my_char is taken in the original source code, while not during tree-ssa > optimization, where the address is used only by ASAN_MARK calls. But how would you be able to find out if there isn't any return *ptr; after the scope or similar (as MEM_REF)? With is_gimple_reg, they will be turned into SSA form and you can easily verify (uses of ASAN_POISON are a problem if they are encountered at runtime). What would you do for the must_live_in_memory vars? Add some pass that detects it, handle it somehow in addressable pass, handle it in SRA, ... ? > > Doing such transformation can rapidly decrease number of __builtin___asan_{un}poison_stack_memory > in tramp3d: from ~36K to ~22K. > > Thanks for clarification. > Martin Jakub
On 11/03/2016 02:44 PM, Jakub Jelinek wrote: > Hi! > > FYI, I think it is much more important to get the initial patch in, so > resolve the switch + declarations inside of its body stuff, add testcases > for that and post for re-review and check in. > These optimizations can be done even in stage3. I know that it's much urgent to have it done first. I'm currently testing patch for the switch + declaration. Hopefully I'll send it today. > > On Thu, Nov 03, 2016 at 02:34:47PM +0100, Martin Liška wrote: >> I'm having a semi-working patch that comes up with the ASAN_POISON built-in. Well, to be honest, >> I still have a feeling that doing the magic with the parallel variable is bit overkill. Maybe >> a new runtime call would make it easier for us. >> >> However, I still don't fully understand why we want to support just is_gimple_reg variables. >> Let's consider following test-case: >> >> void foo() >> { >> char *ptr; >> { >> char my_char[9]; >> ptr = &my_char[0]; >> } >> } >> >> Where I would expect to optimize out: >> <bb 2>: >> _5 = (unsigned long) 9; >> _4 = (unsigned long) &my_char; >> __builtin___asan_unpoison_stack_memory (_4, _5); >> _7 = (unsigned long) 9; >> _6 = (unsigned long) &my_char; >> __builtin___asan_poison_stack_memory (_6, _7); >> return; >> >> where address of my_char is taken in the original source code, while not during tree-ssa >> optimization, where the address is used only by ASAN_MARK calls. > > But how would you be able to find out if there isn't any return *ptr; after > the scope or similar (as MEM_REF)? With is_gimple_reg, they will be turned > into SSA form and you can easily verify (uses of ASAN_POISON are a problem > if they are encountered at runtime). What would you do for the > must_live_in_memory vars? Add some pass that detects it, handle it somehow > in addressable pass, handle it in SRA, ... ? If there's return of *ptr, there must be a &my_char, and it looks _4 = MEM[(char *)&my_char]; properly identifies that my_char has address taken. M. > >> >> Doing such transformation can rapidly decrease number of __builtin___asan_{un}poison_stack_memory >> in tramp3d: from ~36K to ~22K. >> >> Thanks for clarification. >> Martin > > Jakub >
On Thu, Nov 03, 2016 at 03:02:21PM +0100, Martin Liška wrote: > > But how would you be able to find out if there isn't any return *ptr; after > > the scope or similar (as MEM_REF)? With is_gimple_reg, they will be turned > > into SSA form and you can easily verify (uses of ASAN_POISON are a problem > > if they are encountered at runtime). What would you do for the > > must_live_in_memory vars? Add some pass that detects it, handle it somehow > > in addressable pass, handle it in SRA, ... ? > > If there's return of *ptr, there must be a &my_char, and it looks > _4 = MEM[(char *)&my_char]; > > properly identifies that my_char has address taken. It doesn't. MEM_REF's ADDR_EXPR isn't considered to be address taking. Jakub
On 11/03/2016 03:03 PM, Jakub Jelinek wrote: > On Thu, Nov 03, 2016 at 03:02:21PM +0100, Martin Liška wrote: >>> But how would you be able to find out if there isn't any return *ptr; after >>> the scope or similar (as MEM_REF)? With is_gimple_reg, they will be turned >>> into SSA form and you can easily verify (uses of ASAN_POISON are a problem >>> if they are encountered at runtime). What would you do for the >>> must_live_in_memory vars? Add some pass that detects it, handle it somehow >>> in addressable pass, handle it in SRA, ... ? >> >> If there's return of *ptr, there must be a &my_char, and it looks >> _4 = MEM[(char *)&my_char]; >> >> properly identifies that my_char has address taken. > > It doesn't. MEM_REF's ADDR_EXPR isn't considered to be address taking. > > Jakub > You are of course right, my mistake in the patch draft. Thanks for clarification, Martin
From cf860324da41244745f04a16b184fabe343ac5d9 Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Tue, 1 Nov 2016 11:21:20 +0100 Subject: [PATCH 4/4] Use-after-scope: do not mark variables that are no longer addressable gcc/ChangeLog: 2016-11-01 Martin Liska <mliska@suse.cz> * asan.c (asan_instrument): Call execute_update_addresses_taken_asan_sanitize right after sanitization. * tree-ssa.c (maybe_optimize_var): Mark all variables set to non-addressable. (is_asan_mark_p): New function. (execute_update_addresses_taken): Likewise. (execute_update_addresses_taken_asan_sanitize): Likewise. * tree-ssa.h (execute_update_addresses_taken_asan_sanitize): Declare new function. --- gcc/asan.c | 4 +++ gcc/tree-ssa.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++----------- gcc/tree-ssa.h | 1 + 3 files changed, 84 insertions(+), 17 deletions(-) diff --git a/gcc/asan.c b/gcc/asan.c index 95495d2..5cb37c8 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -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-ssa.h" /* AddressSanitizer finds out-of-bounds and use-after-free bugs with <2x slowdown on average. @@ -2973,6 +2974,9 @@ asan_instrument (void) if (shadow_ptr_types[0] == NULL_TREE) asan_init_shadow_ptr_types (); transform_statements (); + + if (optimize) + execute_update_addresses_taken_asan_sanitize (); return 0; } diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c index 135952b..0633b21 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; @@ -1504,7 +1505,7 @@ non_rewritable_lvalue_p (tree lhs) static void maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs, - bitmap suitable_for_renaming) + bitmap suitable_for_renaming, bitmap marked_nonaddressable) { /* Global Variables, result decls cannot be changed. */ if (is_global_var (var) @@ -1522,6 +1523,7 @@ maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs, || !bitmap_bit_p (not_reg_needs, DECL_UID (var)))) { TREE_ADDRESSABLE (var) = 0; + bitmap_set_bit (marked_nonaddressable, DECL_UID (var)); if (is_gimple_reg (var)) bitmap_set_bit (suitable_for_renaming, DECL_UID (var)); if (dump_file) @@ -1550,20 +1552,43 @@ maybe_optimize_var (tree var, bitmap addresses_taken, bitmap not_reg_needs, } } -/* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables. */ +/* Return true when STMT is ASAN mark where second argument is an address + of a local variable. */ -void -execute_update_addresses_taken (void) +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 + && TREE_CODE (TREE_OPERAND (addr, 0)) == VAR_DECL) + return true; + + return false; +} + +/* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables. + If SANITIZE_ASAN_MARK is set to true, sanitize also ASAN_MARK built-ins. */ + + +static void +execute_update_addresses_taken (bool sanitize_asan_mark = false) { basic_block bb; bitmap addresses_taken = BITMAP_ALLOC (NULL); bitmap not_reg_needs = BITMAP_ALLOC (NULL); bitmap suitable_for_renaming = BITMAP_ALLOC (NULL); + bitmap marked_nonaddressable = BITMAP_ALLOC (NULL); tree var; unsigned i; timevar_push (TV_ADDRESS_TAKEN); + if (dump_file) + fprintf (dump_file, "call execute_update_addresses_taken\n"); + /* Collect into ADDRESSES_TAKEN all variables whose address is taken within the function body. */ FOR_EACH_BB_FN (bb, cfun) @@ -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 (sanitize_asan_mark && is_asan_mark_p (stmt)) + ; + else + gimple_ior_addresses_taken (addresses_taken, stmt); } else /* Note all addresses taken by the stmt. */ @@ -1675,15 +1706,17 @@ execute_update_addresses_taken (void) for -g vs. -g0. */ for (var = DECL_ARGUMENTS (cfun->decl); var; var = DECL_CHAIN (var)) maybe_optimize_var (var, addresses_taken, not_reg_needs, - suitable_for_renaming); + suitable_for_renaming, marked_nonaddressable); FOR_EACH_VEC_SAFE_ELT (cfun->local_decls, i, var) maybe_optimize_var (var, addresses_taken, not_reg_needs, - suitable_for_renaming); + suitable_for_renaming, marked_nonaddressable); /* Operand caches need to be recomputed for operands referencing the updated variables and operands need to be rewritten to expose bare symbols. */ - if (!bitmap_empty_p (suitable_for_renaming)) + if (!bitmap_empty_p (suitable_for_renaming) + || (asan_sanitize_use_after_scope () + && !bitmap_empty_p (marked_nonaddressable))) { FOR_EACH_BB_FN (bb, cfun) for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi);) @@ -1841,6 +1874,17 @@ execute_update_addresses_taken (void) continue; } } + else if (sanitize_asan_mark && 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)) + || bitmap_bit_p (marked_nonaddressable, DECL_UID (var))) + { + unlink_stmt_vdef (stmt); + gsi_remove (&gsi, true); + continue; + } + } for (i = 0; i < gimple_call_num_args (stmt); ++i) { tree *argp = gimple_call_arg_ptr (stmt, i); @@ -1896,9 +1940,27 @@ execute_update_addresses_taken (void) BITMAP_FREE (not_reg_needs); BITMAP_FREE (addresses_taken); BITMAP_FREE (suitable_for_renaming); + BITMAP_FREE (marked_nonaddressable); timevar_pop (TV_ADDRESS_TAKEN); } +/* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables. */ + +void +execute_update_addresses_taken (void) +{ + execute_update_addresses_taken (false); +} + +/* Compute TREE_ADDRESSABLE and DECL_GIMPLE_REG_P for local variables + and sanitize ASAN_MARK built-ins. */ + +void +execute_update_addresses_taken_asan_sanitize (void) +{ + execute_update_addresses_taken (true); +} + namespace { const pass_data pass_data_update_address_taken = diff --git a/gcc/tree-ssa.h b/gcc/tree-ssa.h index 37ad7ae..912d144 100644 --- a/gcc/tree-ssa.h +++ b/gcc/tree-ssa.h @@ -53,6 +53,7 @@ extern tree tree_ssa_strip_useless_type_conversions (tree); extern bool ssa_undefined_value_p (tree, bool = true); extern bool gimple_uses_undefined_value_p (gimple *); extern void execute_update_addresses_taken (void); +extern void execute_update_addresses_taken_asan_sanitize (void); /* Given an edge_var_map V, return the PHI arg definition. */ -- 2.10.1