Message ID | b9e3639e-611f-fa1f-ddbc-a08c277e172d@gmail.com |
---|---|
State | New |
Headers | show |
Series | warn on returning alloca and VLA (PR 71924, 90549) | expand |
On 5/22/19 3:34 PM, Martin Sebor wrote: > -Wreturn-local-addr detects a subset of instances of returning > the address of a local object from a function but the warning > doesn't try to handle alloca or VLAs, or some non-trivial cases > of ordinary automatic variables[1]. > > The attached patch extends the implementation of the warning to > detect those. It still doesn't detect instances where the address > is the result of a built-in such strcpy[2]. > > Tested on x86_64-linux. > > Martin > > [1] For example, this is only diagnosed with the patch: > > void* f (int i) > { > struct S { int a[2]; } s[2]; > return &s->a[i]; > } > > [2] The following is not diagnosed even with the patch: > > void sink (void*); > > void* f (int i) > { > char a[6]; > char *p = __builtin_strcpy (a, "123"); > sink (p); > return p; > } > > I would expect detecting to be possible and useful. Maybe as > a follow-up. > > gcc-71924.diff > > PR middle-end/71924 - missing -Wreturn-local-addr returning alloca result > PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an address of a local array plus offset > > gcc/ChangeLog: > > PR c/71924 > * gimple-ssa-isolate-paths.c (is_addr_local): New function. > (warn_return_addr_local_phi_arg, warn_return_addr_local): Same. > (find_implicit_erroneous_behavior): Call warn_return_addr_local_phi_arg. > (find_explicit_erroneous_behavior): Call warn_return_addr_local. > > gcc/testsuite/ChangeLog: > > PR c/71924 > * gcc.dg/Wreturn-local-addr-2.c: New test. > * gcc.dg/Walloca-4.c: Prune expected warnings. > * gcc.dg/pr41551.c: Same. > * gcc.dg/pr59523.c: Same. > * gcc.dg/tree-ssa/pr88775-2.c: Same. > * gcc.dg/winline-7.c: Same. > > diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c > index 33fe352bb23..2933ecf502e 100644 > --- a/gcc/gimple-ssa-isolate-paths.c > +++ b/gcc/gimple-ssa-isolate-paths.c > @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple *stmt) > return false; > } > > +/* Return true if EXPR is a expression of pointer type that refers > + to the address of a variable with automatic storage duration. > + If so, set *PLOC to the location of the object or the call that > + allocated it (for alloca and VLAs). When PMAYBE is non-null, > + also consider PHI statements and set *PMAYBE when some but not > + all arguments of such statements refer to local variables, and > + to clear it otherwise. */ > + > +static bool > +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL, > + hash_set<gphi *> *visited = NULL) > +{ > + if (TREE_CODE (exp) == ADDR_EXPR) > + { > + tree baseaddr = get_base_address (TREE_OPERAND (exp, 0)); > + if (TREE_CODE (baseaddr) == MEM_REF) > + return is_addr_local (TREE_OPERAND (baseaddr, 0), ploc, pmaybe, visited); > + > + if ((!VAR_P (baseaddr) > + || is_global_var (baseaddr)) > + && TREE_CODE (baseaddr) != PARM_DECL) > + return false; > + > + *ploc = DECL_SOURCE_LOCATION (baseaddr); > + return true; > + } > + > + if (TREE_CODE (exp) == SSA_NAME) > + { > + gimple *def_stmt = SSA_NAME_DEF_STMT (exp); > + enum gimple_code code = gimple_code (def_stmt); > + > + if (is_gimple_assign (def_stmt)) > + { > + tree type = TREE_TYPE (gimple_assign_lhs (def_stmt)); > + if (POINTER_TYPE_P (type)) > + { > + tree ptr = gimple_assign_rhs1 (def_stmt); > + return is_addr_local (ptr, ploc, pmaybe, visited); > + } > + return false; > + } > + > + if (code == GIMPLE_CALL > + && gimple_call_builtin_p (def_stmt)) > + { > + tree fn = gimple_call_fndecl (def_stmt); > + int code = DECL_FUNCTION_CODE (fn); > + if (code != BUILT_IN_ALLOCA > + && code != BUILT_IN_ALLOCA_WITH_ALIGN) > + return false; > + > + *ploc = gimple_location (def_stmt); > + return true; > + } > + > + if (code == GIMPLE_PHI && pmaybe) > + { > + unsigned count = 0; > + gphi *phi_stmt = as_a <gphi *> (def_stmt); > + > + unsigned nargs = gimple_phi_num_args (phi_stmt); > + for (unsigned i = 0; i < nargs; ++i) > + { > + if (!visited->add (phi_stmt)) > + { > + tree arg = gimple_phi_arg_def (phi_stmt, i); > + if (is_addr_local (arg, ploc, pmaybe, visited)) > + ++count; > + } > + } > + > + *pmaybe = count && count < nargs; > + return count != 0; > + } > + } > + > + return false; > +} Is there some reason you didn't query the alias oracle here? It would seem a fairly natural fit. Ultimately given a pointer (which will be an SSA_NAME) you want to ask whether or not it conclusively points into the stack. That would seem to dramatically simplify is_addr_local. The rest looks pretty reasonable. Jeff
On 5/29/19 11:45 AM, Jeff Law wrote: > On 5/22/19 3:34 PM, Martin Sebor wrote: >> -Wreturn-local-addr detects a subset of instances of returning >> the address of a local object from a function but the warning >> doesn't try to handle alloca or VLAs, or some non-trivial cases >> of ordinary automatic variables[1]. >> >> The attached patch extends the implementation of the warning to >> detect those. It still doesn't detect instances where the address >> is the result of a built-in such strcpy[2]. >> >> Tested on x86_64-linux. >> >> Martin >> >> [1] For example, this is only diagnosed with the patch: >> >> void* f (int i) >> { >> struct S { int a[2]; } s[2]; >> return &s->a[i]; >> } >> >> [2] The following is not diagnosed even with the patch: >> >> void sink (void*); >> >> void* f (int i) >> { >> char a[6]; >> char *p = __builtin_strcpy (a, "123"); >> sink (p); >> return p; >> } >> >> I would expect detecting to be possible and useful. Maybe as >> a follow-up. >> >> gcc-71924.diff >> >> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca result >> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an address of a local array plus offset >> >> gcc/ChangeLog: >> >> PR c/71924 >> * gimple-ssa-isolate-paths.c (is_addr_local): New function. >> (warn_return_addr_local_phi_arg, warn_return_addr_local): Same. >> (find_implicit_erroneous_behavior): Call warn_return_addr_local_phi_arg. >> (find_explicit_erroneous_behavior): Call warn_return_addr_local. >> >> gcc/testsuite/ChangeLog: >> >> PR c/71924 >> * gcc.dg/Wreturn-local-addr-2.c: New test. >> * gcc.dg/Walloca-4.c: Prune expected warnings. >> * gcc.dg/pr41551.c: Same. >> * gcc.dg/pr59523.c: Same. >> * gcc.dg/tree-ssa/pr88775-2.c: Same. >> * gcc.dg/winline-7.c: Same. >> >> diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c >> index 33fe352bb23..2933ecf502e 100644 >> --- a/gcc/gimple-ssa-isolate-paths.c >> +++ b/gcc/gimple-ssa-isolate-paths.c >> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple *stmt) >> return false; >> } >> >> +/* Return true if EXPR is a expression of pointer type that refers >> + to the address of a variable with automatic storage duration. >> + If so, set *PLOC to the location of the object or the call that >> + allocated it (for alloca and VLAs). When PMAYBE is non-null, >> + also consider PHI statements and set *PMAYBE when some but not >> + all arguments of such statements refer to local variables, and >> + to clear it otherwise. */ >> + >> +static bool >> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL, >> + hash_set<gphi *> *visited = NULL) >> +{ >> + if (TREE_CODE (exp) == ADDR_EXPR) >> + { >> + tree baseaddr = get_base_address (TREE_OPERAND (exp, 0)); >> + if (TREE_CODE (baseaddr) == MEM_REF) >> + return is_addr_local (TREE_OPERAND (baseaddr, 0), ploc, pmaybe, visited); >> + >> + if ((!VAR_P (baseaddr) >> + || is_global_var (baseaddr)) >> + && TREE_CODE (baseaddr) != PARM_DECL) >> + return false; >> + >> + *ploc = DECL_SOURCE_LOCATION (baseaddr); >> + return true; >> + } >> + >> + if (TREE_CODE (exp) == SSA_NAME) >> + { >> + gimple *def_stmt = SSA_NAME_DEF_STMT (exp); >> + enum gimple_code code = gimple_code (def_stmt); >> + >> + if (is_gimple_assign (def_stmt)) >> + { >> + tree type = TREE_TYPE (gimple_assign_lhs (def_stmt)); >> + if (POINTER_TYPE_P (type)) >> + { >> + tree ptr = gimple_assign_rhs1 (def_stmt); >> + return is_addr_local (ptr, ploc, pmaybe, visited); >> + } >> + return false; >> + } >> + >> + if (code == GIMPLE_CALL >> + && gimple_call_builtin_p (def_stmt)) >> + { >> + tree fn = gimple_call_fndecl (def_stmt); >> + int code = DECL_FUNCTION_CODE (fn); >> + if (code != BUILT_IN_ALLOCA >> + && code != BUILT_IN_ALLOCA_WITH_ALIGN) >> + return false; >> + >> + *ploc = gimple_location (def_stmt); >> + return true; >> + } >> + >> + if (code == GIMPLE_PHI && pmaybe) >> + { >> + unsigned count = 0; >> + gphi *phi_stmt = as_a <gphi *> (def_stmt); >> + >> + unsigned nargs = gimple_phi_num_args (phi_stmt); >> + for (unsigned i = 0; i < nargs; ++i) >> + { >> + if (!visited->add (phi_stmt)) >> + { >> + tree arg = gimple_phi_arg_def (phi_stmt, i); >> + if (is_addr_local (arg, ploc, pmaybe, visited)) >> + ++count; >> + } >> + } >> + >> + *pmaybe = count && count < nargs; >> + return count != 0; >> + } >> + } >> + >> + return false; >> +} > Is there some reason you didn't query the alias oracle here? It would > seem a fairly natural fit. Ultimately given a pointer (which will be an > SSA_NAME) you want to ask whether or not it conclusively points into the > stack. > > That would seem to dramatically simplify is_addr_local. I did think about it but decided against changing the existing design (iterating over PHI arguments), for a couple of reasons: 1) It feels like a bigger change than my simple "bug fix" calls for. 2) I'm not familiar enough with the alias oracle to say for sure if it can be used to give the same results as the existing implementation. I.e., make it possible to identify and isolate each path that returns a local address (rather than just answering: yes, this pointer may point to some local in this function). If the alias oracle can be used to give the same results without excessive false positives then I think it would be fine to make use of it. Is that something you consider a prerequisite for this change or should I look into it as a followup? FWIW, I'm working on enhancing this to detect returning freed pointers (under a different option). That seems like a good opportunity to also look into making use of the alias oracle. Besides these enhancements, there's also a request to diagnose dereferencing pointers to compound literals whose lifetime has ended (PR 89990), or more generally, those to any such local object. It's about preventing essentially the same problem (accessing bad data or corrupting others) but it seems that both the analysis and the handling will be sufficiently different to consider implementing it somewhere else. What are your thoughts on that? Martin > > The rest looks pretty reasonable. > > Jeff >
On 5/30/19 8:52 AM, Martin Sebor wrote: > On 5/29/19 11:45 AM, Jeff Law wrote: >> On 5/22/19 3:34 PM, Martin Sebor wrote: >>> -Wreturn-local-addr detects a subset of instances of returning >>> the address of a local object from a function but the warning >>> doesn't try to handle alloca or VLAs, or some non-trivial cases >>> of ordinary automatic variables[1]. >>> >>> The attached patch extends the implementation of the warning to >>> detect those. It still doesn't detect instances where the address >>> is the result of a built-in such strcpy[2]. >>> >>> Tested on x86_64-linux. >>> >>> Martin >>> >>> [1] For example, this is only diagnosed with the patch: >>> >>> void* f (int i) >>> { >>> struct S { int a[2]; } s[2]; >>> return &s->a[i]; >>> } >>> >>> [2] The following is not diagnosed even with the patch: >>> >>> void sink (void*); >>> >>> void* f (int i) >>> { >>> char a[6]; >>> char *p = __builtin_strcpy (a, "123"); >>> sink (p); >>> return p; >>> } >>> >>> I would expect detecting to be possible and useful. Maybe as >>> a follow-up. >>> >>> gcc-71924.diff >>> >>> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca >>> result >>> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an >>> address of a local array plus offset >>> >>> gcc/ChangeLog: >>> >>> PR c/71924 >>> * gimple-ssa-isolate-paths.c (is_addr_local): New function. >>> (warn_return_addr_local_phi_arg, warn_return_addr_local): Same. >>> (find_implicit_erroneous_behavior): Call >>> warn_return_addr_local_phi_arg. >>> (find_explicit_erroneous_behavior): Call warn_return_addr_local. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR c/71924 >>> * gcc.dg/Wreturn-local-addr-2.c: New test. >>> * gcc.dg/Walloca-4.c: Prune expected warnings. >>> * gcc.dg/pr41551.c: Same. >>> * gcc.dg/pr59523.c: Same. >>> * gcc.dg/tree-ssa/pr88775-2.c: Same. >>> * gcc.dg/winline-7.c: Same. >>> >>> diff --git a/gcc/gimple-ssa-isolate-paths.c >>> b/gcc/gimple-ssa-isolate-paths.c >>> index 33fe352bb23..2933ecf502e 100644 >>> --- a/gcc/gimple-ssa-isolate-paths.c >>> +++ b/gcc/gimple-ssa-isolate-paths.c >>> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple >>> *stmt) >>> return false; >>> } >>> +/* Return true if EXPR is a expression of pointer type that refers >>> + to the address of a variable with automatic storage duration. >>> + If so, set *PLOC to the location of the object or the call that >>> + allocated it (for alloca and VLAs). When PMAYBE is non-null, >>> + also consider PHI statements and set *PMAYBE when some but not >>> + all arguments of such statements refer to local variables, and >>> + to clear it otherwise. */ >>> + >>> +static bool >>> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL, >>> + hash_set<gphi *> *visited = NULL) >>> +{ >>> + if (TREE_CODE (exp) == ADDR_EXPR) >>> + { >>> + tree baseaddr = get_base_address (TREE_OPERAND (exp, 0)); >>> + if (TREE_CODE (baseaddr) == MEM_REF) >>> + return is_addr_local (TREE_OPERAND (baseaddr, 0), ploc, pmaybe, >>> visited); >>> + >>> + if ((!VAR_P (baseaddr) >>> + || is_global_var (baseaddr)) >>> + && TREE_CODE (baseaddr) != PARM_DECL) >>> + return false; >>> + >>> + *ploc = DECL_SOURCE_LOCATION (baseaddr); >>> + return true; >>> + } >>> + >>> + if (TREE_CODE (exp) == SSA_NAME) >>> + { >>> + gimple *def_stmt = SSA_NAME_DEF_STMT (exp); >>> + enum gimple_code code = gimple_code (def_stmt); >>> + >>> + if (is_gimple_assign (def_stmt)) >>> + { >>> + tree type = TREE_TYPE (gimple_assign_lhs (def_stmt)); >>> + if (POINTER_TYPE_P (type)) >>> + { >>> + tree ptr = gimple_assign_rhs1 (def_stmt); >>> + return is_addr_local (ptr, ploc, pmaybe, visited); >>> + } >>> + return false; >>> + } >>> + >>> + if (code == GIMPLE_CALL >>> + && gimple_call_builtin_p (def_stmt)) >>> + { >>> + tree fn = gimple_call_fndecl (def_stmt); >>> + int code = DECL_FUNCTION_CODE (fn); >>> + if (code != BUILT_IN_ALLOCA >>> + && code != BUILT_IN_ALLOCA_WITH_ALIGN) >>> + return false; >>> + >>> + *ploc = gimple_location (def_stmt); >>> + return true; >>> + } >>> + >>> + if (code == GIMPLE_PHI && pmaybe) >>> + { >>> + unsigned count = 0; >>> + gphi *phi_stmt = as_a <gphi *> (def_stmt); >>> + >>> + unsigned nargs = gimple_phi_num_args (phi_stmt); >>> + for (unsigned i = 0; i < nargs; ++i) >>> + { >>> + if (!visited->add (phi_stmt)) >>> + { >>> + tree arg = gimple_phi_arg_def (phi_stmt, i); >>> + if (is_addr_local (arg, ploc, pmaybe, visited)) >>> + ++count; >>> + } >>> + } >>> + >>> + *pmaybe = count && count < nargs; >>> + return count != 0; >>> + } >>> + } >>> + >>> + return false; >>> +} >> Is there some reason you didn't query the alias oracle here? It would >> seem a fairly natural fit. Ultimately given a pointer (which will be an >> SSA_NAME) you want to ask whether or not it conclusively points into the >> stack. >> >> That would seem to dramatically simplify is_addr_local. > > I did think about it but decided against changing the existing > design (iterating over PHI arguments), for a couple of reasons: > > 1) It feels like a bigger change than my simple "bug fix" calls > for. I suspect the net result will be simpler though ;-) I think you just get a pt solution and iterate over the things the solution says the pointer can point to. > 2) I'm not familiar enough with the alias oracle to say for sure > if it can be used to give the same results as the existing > implementation. I.e., make it possible to identify and > isolate each path that returns a local address (rather than > just answering: yes, this pointer may point to some local > in this function). Precision of the oracle is certainly the big question. > > If the alias oracle can be used to give the same results without > excessive false positives then I think it would be fine to make > use of it. Is that something you consider a prerequisite for > this change or should I look into it as a followup? I think we should explore it a bit before making a final decision. It may guide us for other work in this space (like detecting escaping locals). I think a dirty prototype to see if it's even in the right ballpark would make sense. > > FWIW, I'm working on enhancing this to detect returning freed > pointers (under a different option). That seems like a good > opportunity to also look into making use of the alias oracle. Yes. I think there's two interesting cases here to ponder. If we free a pointer that must point to a local, then we can warn & trap. If we free a pointer that may point to a local, then we can only warn (unless we can isolate the path). > > Besides these enhancements, there's also a request to diagnose > dereferencing pointers to compound literals whose lifetime has > ended (PR 89990), or more generally, those to any such local > object. It's about preventing essentially the same problem > (accessing bad data or corrupting others) but it seems that > both the analysis and the handling will be sufficiently > different to consider implementing it somewhere else. What > are your thoughts on that? I think the tough problem here is we lose binding scopes as we lower into gimple, so solving it in the general case may be tough. We've started adding some clobbers into the IL to denote object death points, but I'm not sure if they're sufficient to implement this kind of warning. Jeff
On 5/30/19 9:13 AM, Jeff Law wrote: > On 5/30/19 8:52 AM, Martin Sebor wrote: >> On 5/29/19 11:45 AM, Jeff Law wrote: >>> On 5/22/19 3:34 PM, Martin Sebor wrote: >>>> -Wreturn-local-addr detects a subset of instances of returning >>>> the address of a local object from a function but the warning >>>> doesn't try to handle alloca or VLAs, or some non-trivial cases >>>> of ordinary automatic variables[1]. >>>> >>>> The attached patch extends the implementation of the warning to >>>> detect those. It still doesn't detect instances where the address >>>> is the result of a built-in such strcpy[2]. >>>> >>>> Tested on x86_64-linux. >>>> >>>> Martin >>>> >>>> [1] For example, this is only diagnosed with the patch: >>>> >>>> void* f (int i) >>>> { >>>> struct S { int a[2]; } s[2]; >>>> return &s->a[i]; >>>> } >>>> >>>> [2] The following is not diagnosed even with the patch: >>>> >>>> void sink (void*); >>>> >>>> void* f (int i) >>>> { >>>> char a[6]; >>>> char *p = __builtin_strcpy (a, "123"); >>>> sink (p); >>>> return p; >>>> } >>>> >>>> I would expect detecting to be possible and useful. Maybe as >>>> a follow-up. >>>> >>>> gcc-71924.diff >>>> >>>> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca >>>> result >>>> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an >>>> address of a local array plus offset >>>> >>>> gcc/ChangeLog: >>>> >>>> PR c/71924 >>>> * gimple-ssa-isolate-paths.c (is_addr_local): New function. >>>> (warn_return_addr_local_phi_arg, warn_return_addr_local): Same. >>>> (find_implicit_erroneous_behavior): Call >>>> warn_return_addr_local_phi_arg. >>>> (find_explicit_erroneous_behavior): Call warn_return_addr_local. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> PR c/71924 >>>> * gcc.dg/Wreturn-local-addr-2.c: New test. >>>> * gcc.dg/Walloca-4.c: Prune expected warnings. >>>> * gcc.dg/pr41551.c: Same. >>>> * gcc.dg/pr59523.c: Same. >>>> * gcc.dg/tree-ssa/pr88775-2.c: Same. >>>> * gcc.dg/winline-7.c: Same. >>>> >>>> diff --git a/gcc/gimple-ssa-isolate-paths.c >>>> b/gcc/gimple-ssa-isolate-paths.c >>>> index 33fe352bb23..2933ecf502e 100644 >>>> --- a/gcc/gimple-ssa-isolate-paths.c >>>> +++ b/gcc/gimple-ssa-isolate-paths.c >>>> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple >>>> *stmt) >>>> return false; >>>> } >>>> +/* Return true if EXPR is a expression of pointer type that refers >>>> + to the address of a variable with automatic storage duration. >>>> + If so, set *PLOC to the location of the object or the call that >>>> + allocated it (for alloca and VLAs). When PMAYBE is non-null, >>>> + also consider PHI statements and set *PMAYBE when some but not >>>> + all arguments of such statements refer to local variables, and >>>> + to clear it otherwise. */ >>>> + >>>> +static bool >>>> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL, >>>> + hash_set<gphi *> *visited = NULL) >>>> +{ >>>> + if (TREE_CODE (exp) == ADDR_EXPR) >>>> + { >>>> + tree baseaddr = get_base_address (TREE_OPERAND (exp, 0)); >>>> + if (TREE_CODE (baseaddr) == MEM_REF) >>>> + return is_addr_local (TREE_OPERAND (baseaddr, 0), ploc, pmaybe, >>>> visited); >>>> + >>>> + if ((!VAR_P (baseaddr) >>>> + || is_global_var (baseaddr)) >>>> + && TREE_CODE (baseaddr) != PARM_DECL) >>>> + return false; >>>> + >>>> + *ploc = DECL_SOURCE_LOCATION (baseaddr); >>>> + return true; >>>> + } >>>> + >>>> + if (TREE_CODE (exp) == SSA_NAME) >>>> + { >>>> + gimple *def_stmt = SSA_NAME_DEF_STMT (exp); >>>> + enum gimple_code code = gimple_code (def_stmt); >>>> + >>>> + if (is_gimple_assign (def_stmt)) >>>> + { >>>> + tree type = TREE_TYPE (gimple_assign_lhs (def_stmt)); >>>> + if (POINTER_TYPE_P (type)) >>>> + { >>>> + tree ptr = gimple_assign_rhs1 (def_stmt); >>>> + return is_addr_local (ptr, ploc, pmaybe, visited); >>>> + } >>>> + return false; >>>> + } >>>> + >>>> + if (code == GIMPLE_CALL >>>> + && gimple_call_builtin_p (def_stmt)) >>>> + { >>>> + tree fn = gimple_call_fndecl (def_stmt); >>>> + int code = DECL_FUNCTION_CODE (fn); >>>> + if (code != BUILT_IN_ALLOCA >>>> + && code != BUILT_IN_ALLOCA_WITH_ALIGN) >>>> + return false; >>>> + >>>> + *ploc = gimple_location (def_stmt); >>>> + return true; >>>> + } >>>> + >>>> + if (code == GIMPLE_PHI && pmaybe) >>>> + { >>>> + unsigned count = 0; >>>> + gphi *phi_stmt = as_a <gphi *> (def_stmt); >>>> + >>>> + unsigned nargs = gimple_phi_num_args (phi_stmt); >>>> + for (unsigned i = 0; i < nargs; ++i) >>>> + { >>>> + if (!visited->add (phi_stmt)) >>>> + { >>>> + tree arg = gimple_phi_arg_def (phi_stmt, i); >>>> + if (is_addr_local (arg, ploc, pmaybe, visited)) >>>> + ++count; >>>> + } >>>> + } >>>> + >>>> + *pmaybe = count && count < nargs; >>>> + return count != 0; >>>> + } >>>> + } >>>> + >>>> + return false; >>>> +} >>> Is there some reason you didn't query the alias oracle here? It would >>> seem a fairly natural fit. Ultimately given a pointer (which will be an >>> SSA_NAME) you want to ask whether or not it conclusively points into the >>> stack. >>> >>> That would seem to dramatically simplify is_addr_local. >> >> I did think about it but decided against changing the existing >> design (iterating over PHI arguments), for a couple of reasons: >> >> 1) It feels like a bigger change than my simple "bug fix" calls >> for. > I suspect the net result will be simpler though ;-) I think you just > get a pt solution and iterate over the things the solution says the > pointer can point to. > > >> 2) I'm not familiar enough with the alias oracle to say for sure >> if it can be used to give the same results as the existing >> implementation. I.e., make it possible to identify and >> isolate each path that returns a local address (rather than >> just answering: yes, this pointer may point to some local >> in this function). > Precision of the oracle is certainly the big question. > > >> >> If the alias oracle can be used to give the same results without >> excessive false positives then I think it would be fine to make >> use of it. Is that something you consider a prerequisite for >> this change or should I look into it as a followup? > I think we should explore it a bit before making a final decision. It > may guide us for other work in this space (like detecting escaping > locals). I think a dirty prototype to see if it's even in the right > ballpark would make sense. Okay, let me look into it. >> FWIW, I'm working on enhancing this to detect returning freed >> pointers (under a different option). That seems like a good >> opportunity to also look into making use of the alias oracle. > Yes. I think there's two interesting cases here to ponder. If we free > a pointer that must point to a local, then we can warn & trap. If we > free a pointer that may point to a local, then we can only warn (unless > we can isolate the path). I wasn't actually thinking of freeing locals but it sounds like a useful enhancement as well. Thanks for the suggestion! :) To be clear, what I'm working on is detecting: void* f (void *p) { free (p); // note: pointer was freed here // ... return p; // warning: returning a freed pointer } >> Besides these enhancements, there's also a request to diagnose >> dereferencing pointers to compound literals whose lifetime has >> ended (PR 89990), or more generally, those to any such local >> object. It's about preventing essentially the same problem >> (accessing bad data or corrupting others) but it seems that >> both the analysis and the handling will be sufficiently >> different to consider implementing it somewhere else. What >> are your thoughts on that? > I think the tough problem here is we lose binding scopes as we lower > into gimple, so solving it in the general case may be tough. We've > started adding some clobbers into the IL to denote object death points, > but I'm not sure if they're sufficient to implement this kind of warning. I was afraid that might be a problem. Thanks Martin
On 5/30/19 9:34 AM, Martin Sebor wrote: >>> If the alias oracle can be used to give the same results without >>> excessive false positives then I think it would be fine to make >>> use of it. Is that something you consider a prerequisite for >>> this change or should I look into it as a followup? >> I think we should explore it a bit before making a final decision. It >> may guide us for other work in this space (like detecting escaping >> locals). I think a dirty prototype to see if it's even in the right >> ballpark would make sense. > > Okay, let me look into it. Sounds good. Again, go with a quick prototype to see if it's likely feasible. The tests you've added should dramatically help evaluating if the oracle is up to the task. > >>> FWIW, I'm working on enhancing this to detect returning freed >>> pointers (under a different option). That seems like a good >>> opportunity to also look into making use of the alias oracle. >> Yes. I think there's two interesting cases here to ponder. If we free >> a pointer that must point to a local, then we can warn & trap. If we >> free a pointer that may point to a local, then we can only warn (unless >> we can isolate the path). > > I wasn't actually thinking of freeing locals but it sounds like > a useful enhancement as well. Thanks for the suggestion! :) > > To be clear, what I'm working on is detecting: > > void* f (void *p) > { > free (p); // note: pointer was freed here > // ... > return p; // warning: returning a freed pointer > } Ah, we were talking about different things. Though what you're doing might be better modeled in a true global static analyzer as a use-after-free problem. My sense is that translation-unit local version of that problem really isn't that useful in practice. THough I guess there isn't anything bad with having a TU local version. > >>> Besides these enhancements, there's also a request to diagnose >>> dereferencing pointers to compound literals whose lifetime has >>> ended (PR 89990), or more generally, those to any such local >>> object. It's about preventing essentially the same problem >>> (accessing bad data or corrupting others) but it seems that >>> both the analysis and the handling will be sufficiently >>> different to consider implementing it somewhere else. What >>> are your thoughts on that? >> I think the tough problem here is we lose binding scopes as we lower >> into gimple, so solving it in the general case may be tough. We've >> started adding some clobbers into the IL to denote object death points, >> but I'm not sure if they're sufficient to implement this kind of warning. > > I was afraid that might be a problem. Way back in the early days of tree-ssa we kept the binding scopes. But that proved problematical in various ways. Mostly they just got in the way of analysis an optimization and we spent far too much time in the optimizers working around them or removing those which were empty. They'd be helpful in this kind of analysis, stack slot sharing vs the inliner and a couple other problems. I don't know if the pendulum has moved far enough to revisit :-) jeff
On Thu, May 30, 2019 at 12:16 PM Jeff Law <law@redhat.com> wrote: > > On 5/30/19 9:34 AM, Martin Sebor wrote: > > >>> If the alias oracle can be used to give the same results without > >>> excessive false positives then I think it would be fine to make > >>> use of it. Is that something you consider a prerequisite for > >>> this change or should I look into it as a followup? > >> I think we should explore it a bit before making a final decision. It > >> may guide us for other work in this space (like detecting escaping > >> locals). I think a dirty prototype to see if it's even in the right > >> ballpark would make sense. > > > > Okay, let me look into it. > Sounds good. Again, go with a quick prototype to see if it's likely > feasible. The tests you've added should dramatically help evaluating if > the oracle is up to the task. > > > > >>> FWIW, I'm working on enhancing this to detect returning freed > >>> pointers (under a different option). That seems like a good > >>> opportunity to also look into making use of the alias oracle. > >> Yes. I think there's two interesting cases here to ponder. If we free > >> a pointer that must point to a local, then we can warn & trap. If we > >> free a pointer that may point to a local, then we can only warn (unless > >> we can isolate the path). > > > > I wasn't actually thinking of freeing locals but it sounds like > > a useful enhancement as well. Thanks for the suggestion! :) > > > > To be clear, what I'm working on is detecting: > > > > void* f (void *p) > > { > > free (p); // note: pointer was freed here > > // ... > > return p; // warning: returning a freed pointer > > } > Ah, we were talking about different things. Though what you're doing > might be better modeled in a true global static analyzer as a > use-after-free problem. My sense is that translation-unit local version > of that problem really isn't that useful in practice. THough I guess > there isn't anything bad with having a TU local version. > > > > > >>> Besides these enhancements, there's also a request to diagnose > >>> dereferencing pointers to compound literals whose lifetime has > >>> ended (PR 89990), or more generally, those to any such local > >>> object. It's about preventing essentially the same problem > >>> (accessing bad data or corrupting others) but it seems that > >>> both the analysis and the handling will be sufficiently > >>> different to consider implementing it somewhere else. What > >>> are your thoughts on that? > >> I think the tough problem here is we lose binding scopes as we lower > >> into gimple, so solving it in the general case may be tough. We've > >> started adding some clobbers into the IL to denote object death points, > >> but I'm not sure if they're sufficient to implement this kind of warning. > > > > I was afraid that might be a problem. > Way back in the early days of tree-ssa we kept the binding scopes. But > that proved problematical in various ways. Mostly they just got in the > way of analysis an optimization and we spent far too much time in the > optimizers working around them or removing those which were empty. > > They'd be helpful in this kind of analysis, stack slot sharing vs the > inliner and a couple other problems. I don't know if the pendulum has > moved far enough to revisit :-) Why wouldn't clobbers be sufficient? Jaason
On 5/30/19 10:15 AM, Jeff Law wrote: > On 5/30/19 9:34 AM, Martin Sebor wrote: > >>>> If the alias oracle can be used to give the same results without >>>> excessive false positives then I think it would be fine to make >>>> use of it. Is that something you consider a prerequisite for >>>> this change or should I look into it as a followup? >>> I think we should explore it a bit before making a final decision. It >>> may guide us for other work in this space (like detecting escaping >>> locals). I think a dirty prototype to see if it's even in the right >>> ballpark would make sense. >> >> Okay, let me look into it. > Sounds good. Again, go with a quick prototype to see if it's likely > feasible. The tests you've added should dramatically help evaluating if > the oracle is up to the task. So to expand on what I said on the phone when we spoke: the problem I quickly ran into with the prototype is that I wasn't able to find a way to identify pointers to alloca/VLA storage. In the the points-to solution for the pointer being returned they both have the vars_contains_escaped_heap flag set. That seems like an omission that shouldn't be hard to fix, but on its own, I don't think it would be sufficient. In the IL a VLA is represented as a pointer to an array, but when returning a pointer into a VLA (at some offset so it's an SSA_NAME), the pointer's point-to solution doesn't include the VLA pointer or (AFAICS) make it possible to tell even that it is a VLA. For example here: f (int n) { int * p; int[0:D.1912] * a.1; sizetype _1; void * saved_stack.3_3; sizetype _6; <bb 2> [local count: 1073741824]: saved_stack.3_3 = __builtin_stack_save (); _1 = (sizetype) n_2(D); _6 = _1 * 4; a.1_8 = __builtin_alloca_with_align (_6, 32); p_9 = a.1_8 + _6; __builtin_stack_restore (saved_stack.3_3); return p_9; } p_9's solution's is: p_9, points-to vars: { D.1925 } (escaped, escaped heap) I couldn't find out how to determine that D.1925 is a VLA (or even what it is). It's not among the function's local variables that FOR_EACH_LOCAL_DECL iterates over. By replacing the VLA in the test case with a call to malloc I get this for the returned p_7: p_7, points-to NULL, points-to vars: { D.1916 } (escaped, escaped heap) Again, I see no way to quickly tell that this pointer points into the block returned from malloc. I'm sure Richard will correct me if there is a simple way to do it but I couldn't find one. If there is a way to identify that the returned pointer is for a VLA (or alloca), for parity with the current patch we also need to find the location of the VLA declaration or the alloca call so that the warning could point to it. Unless there's a straight path from the mysterious D.1925 above to that decl/call statement, finding it would likely require some sor of traversal. At that point I wouldn't be surprised if the complexity of the solution didn't approach or even exceed the current approach. Martin >>>> FWIW, I'm working on enhancing this to detect returning freed >>>> pointers (under a different option). That seems like a good >>>> opportunity to also look into making use of the alias oracle. >>> Yes. I think there's two interesting cases here to ponder. If we free >>> a pointer that must point to a local, then we can warn & trap. If we >>> free a pointer that may point to a local, then we can only warn (unless >>> we can isolate the path). >> >> I wasn't actually thinking of freeing locals but it sounds like >> a useful enhancement as well. Thanks for the suggestion! :) >> >> To be clear, what I'm working on is detecting: >> >> void* f (void *p) >> { >> free (p); // note: pointer was freed here >> // ... >> return p; // warning: returning a freed pointer >> } > Ah, we were talking about different things. Though what you're doing > might be better modeled in a true global static analyzer as a > use-after-free problem. My sense is that translation-unit local version > of that problem really isn't that useful in practice. THough I guess > there isn't anything bad with having a TU local version. > > >> >>>> Besides these enhancements, there's also a request to diagnose >>>> dereferencing pointers to compound literals whose lifetime has >>>> ended (PR 89990), or more generally, those to any such local >>>> object. It's about preventing essentially the same problem >>>> (accessing bad data or corrupting others) but it seems that >>>> both the analysis and the handling will be sufficiently >>>> different to consider implementing it somewhere else. What >>>> are your thoughts on that? >>> I think the tough problem here is we lose binding scopes as we lower >>> into gimple, so solving it in the general case may be tough. We've >>> started adding some clobbers into the IL to denote object death points, >>> but I'm not sure if they're sufficient to implement this kind of warning. >> >> I was afraid that might be a problem. > Way back in the early days of tree-ssa we kept the binding scopes. But > that proved problematical in various ways. Mostly they just got in the > way of analysis an optimization and we spent far too much time in the > optimizers working around them or removing those which were empty. > > They'd be helpful in this kind of analysis, stack slot sharing vs the > inliner and a couple other problems. I don't know if the pendulum has > moved far enough to revisit :-) > > jeff >
On 5/30/19 11:23 AM, Jason Merrill wrote: > On Thu, May 30, 2019 at 12:16 PM Jeff Law <law@redhat.com> wrote: >> >> On 5/30/19 9:34 AM, Martin Sebor wrote: >> >>>>> If the alias oracle can be used to give the same results without >>>>> excessive false positives then I think it would be fine to make >>>>> use of it. Is that something you consider a prerequisite for >>>>> this change or should I look into it as a followup? >>>> I think we should explore it a bit before making a final decision. It >>>> may guide us for other work in this space (like detecting escaping >>>> locals). I think a dirty prototype to see if it's even in the right >>>> ballpark would make sense. >>> >>> Okay, let me look into it. >> Sounds good. Again, go with a quick prototype to see if it's likely >> feasible. The tests you've added should dramatically help evaluating if >> the oracle is up to the task. >> >>> >>>>> FWIW, I'm working on enhancing this to detect returning freed >>>>> pointers (under a different option). That seems like a good >>>>> opportunity to also look into making use of the alias oracle. >>>> Yes. I think there's two interesting cases here to ponder. If we free >>>> a pointer that must point to a local, then we can warn & trap. If we >>>> free a pointer that may point to a local, then we can only warn (unless >>>> we can isolate the path). >>> >>> I wasn't actually thinking of freeing locals but it sounds like >>> a useful enhancement as well. Thanks for the suggestion! :) >>> >>> To be clear, what I'm working on is detecting: >>> >>> void* f (void *p) >>> { >>> free (p); // note: pointer was freed here >>> // ... >>> return p; // warning: returning a freed pointer >>> } >> Ah, we were talking about different things. Though what you're doing >> might be better modeled in a true global static analyzer as a >> use-after-free problem. My sense is that translation-unit local version >> of that problem really isn't that useful in practice. THough I guess >> there isn't anything bad with having a TU local version. >> >> >>> >>>>> Besides these enhancements, there's also a request to diagnose >>>>> dereferencing pointers to compound literals whose lifetime has >>>>> ended (PR 89990), or more generally, those to any such local >>>>> object. It's about preventing essentially the same problem >>>>> (accessing bad data or corrupting others) but it seems that >>>>> both the analysis and the handling will be sufficiently >>>>> different to consider implementing it somewhere else. What >>>>> are your thoughts on that? >>>> I think the tough problem here is we lose binding scopes as we lower >>>> into gimple, so solving it in the general case may be tough. We've >>>> started adding some clobbers into the IL to denote object death points, >>>> but I'm not sure if they're sufficient to implement this kind of warning. >>> >>> I was afraid that might be a problem. >> Way back in the early days of tree-ssa we kept the binding scopes. But >> that proved problematical in various ways. Mostly they just got in the >> way of analysis an optimization and we spent far too much time in the >> optimizers working around them or removing those which were empty. >> >> They'd be helpful in this kind of analysis, stack slot sharing vs the >> inliner and a couple other problems. I don't know if the pendulum has >> moved far enough to revisit :-) > > Why wouldn't clobbers be sufficient? I haven't looked into the clobbers in any detail. If we're aggressively emitting them at the end of object life, then they may be sufficient to start tackling these issues. jeff
On 5/30/19 4:56 PM, Martin Sebor wrote: > On 5/30/19 10:15 AM, Jeff Law wrote: >> On 5/30/19 9:34 AM, Martin Sebor wrote: >> >>>>> If the alias oracle can be used to give the same results without >>>>> excessive false positives then I think it would be fine to make >>>>> use of it. Is that something you consider a prerequisite for >>>>> this change or should I look into it as a followup? >>>> I think we should explore it a bit before making a final decision. It >>>> may guide us for other work in this space (like detecting escaping >>>> locals). I think a dirty prototype to see if it's even in the right >>>> ballpark would make sense. >>> >>> Okay, let me look into it. >> Sounds good. Again, go with a quick prototype to see if it's likely >> feasible. The tests you've added should dramatically help evaluating if >> the oracle is up to the task. > > So to expand on what I said on the phone when we spoke: the problem > I quickly ran into with the prototype is that I wasn't able to find > a way to identify pointers to alloca/VLA storage. Your analysis matches my very quick read of the aliasing code. It may be the case that the Steensgaard patent got in the way here. > > In the the points-to solution for the pointer being returned they > both have the vars_contains_escaped_heap flag set. That seems like > an omission that shouldn't be hard to fix, but on its own, I don't > think it would be sufficient. RIght. In theory the result of an alloca call shouldn't alias anything in the heap -- but there were some implementations of alloca that were built on top of malloc (ugh). That flag may be catering to that case. But in the case of a __builtin_alloca that shouldn't apply. Hmm. That ultimately might be a bug. > > In the IL a VLA is represented as a pointer to an array, but when > returning a pointer into a VLA (at some offset so it's an SSA_NAME), > the pointer's point-to solution doesn't include the VLA pointer or > (AFAICS) make it possible to tell even that it is a VLA. For example > here: > > f (int n) > { > int * p; > int[0:D.1912] * a.1; > sizetype _1; > void * saved_stack.3_3; > sizetype _6; > > <bb 2> [local count: 1073741824]: > saved_stack.3_3 = __builtin_stack_save (); > _1 = (sizetype) n_2(D); > _6 = _1 * 4; > a.1_8 = __builtin_alloca_with_align (_6, 32); > p_9 = a.1_8 + _6; > __builtin_stack_restore (saved_stack.3_3); > return p_9; > } > > p_9's solution's is: > > p_9, points-to vars: { D.1925 } (escaped, escaped heap) > > I couldn't find out how to determine that D.1925 is a VLA (or even > what it is). It's not among the function's local variables that > FOR_EACH_LOCAL_DECL iterates over. It's possible that decl was created internally as part of the alias oracle's analysis. See make_heapvar in tree-ssa-structalias.c > > By replacing the VLA in the test case with a call to malloc I get > this for the returned p_7: > > p_7, points-to NULL, points-to vars: { D.1916 } (escaped, escaped heap) > > Again, I see no way to quickly tell that this pointer points into > the block returned from malloc. If there's a way to make that determination it'd have to be on the variable since the pt_solution flag bits don't carry a storage class directly. You might try to get a handle on those decls and dump them to see if there's anything easily identifiable. But it may be easier to dig into the code that creates them. A real quick scan of the aliasing code also shows the "variable_info" structure. It's private to the aliasing code, but might guide you at things to look at. Regardless, I don't see an immediate path forward using the oracle to identify objects in the stack for your patch. WHich is unfortunate. Jeff
On 5/22/19 3:34 PM, Martin Sebor wrote: > -Wreturn-local-addr detects a subset of instances of returning > the address of a local object from a function but the warning > doesn't try to handle alloca or VLAs, or some non-trivial cases > of ordinary automatic variables[1]. > > The attached patch extends the implementation of the warning to > detect those. It still doesn't detect instances where the address > is the result of a built-in such strcpy[2]. > > Tested on x86_64-linux. > > Martin > > [1] For example, this is only diagnosed with the patch: > > void* f (int i) > { > struct S { int a[2]; } s[2]; > return &s->a[i]; > } > > [2] The following is not diagnosed even with the patch: > > void sink (void*); > > void* f (int i) > { > char a[6]; > char *p = __builtin_strcpy (a, "123"); > sink (p); > return p; > } > > I would expect detecting to be possible and useful. Maybe as > a follow-up. > > gcc-71924.diff > > PR middle-end/71924 - missing -Wreturn-local-addr returning alloca result > PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an address of a local array plus offset > > gcc/ChangeLog: > > PR c/71924 > * gimple-ssa-isolate-paths.c (is_addr_local): New function. > (warn_return_addr_local_phi_arg, warn_return_addr_local): Same. > (find_implicit_erroneous_behavior): Call warn_return_addr_local_phi_arg. > (find_explicit_erroneous_behavior): Call warn_return_addr_local. > > gcc/testsuite/ChangeLog: > > PR c/71924 > * gcc.dg/Wreturn-local-addr-2.c: New test. > * gcc.dg/Walloca-4.c: Prune expected warnings. > * gcc.dg/pr41551.c: Same. > * gcc.dg/pr59523.c: Same. > * gcc.dg/tree-ssa/pr88775-2.c: Same. > * gcc.dg/winline-7.c: Same. > > diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c > index 33fe352bb23..2933ecf502e 100644 > --- a/gcc/gimple-ssa-isolate-paths.c > +++ b/gcc/gimple-ssa-isolate-paths.c > @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple *stmt) > return false; > } > > +/* Return true if EXPR is a expression of pointer type that refers > + to the address of a variable with automatic storage duration. > + If so, set *PLOC to the location of the object or the call that > + allocated it (for alloca and VLAs). When PMAYBE is non-null, > + also consider PHI statements and set *PMAYBE when some but not > + all arguments of such statements refer to local variables, and > + to clear it otherwise. */ > + > +static bool > +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL, > + hash_set<gphi *> *visited = NULL) > +{ > + if (TREE_CODE (exp) == SSA_NAME) > + { > + gimple *def_stmt = SSA_NAME_DEF_STMT (exp); > + enum gimple_code code = gimple_code (def_stmt); > + > + if (is_gimple_assign (def_stmt)) > + { > + tree type = TREE_TYPE (gimple_assign_lhs (def_stmt)); > + if (POINTER_TYPE_P (type)) > + { > + tree ptr = gimple_assign_rhs1 (def_stmt); > + return is_addr_local (ptr, ploc, pmaybe, visited); > + } > + return false; > + } So this is going to recurse on the rhs1 of something like POINTER_PLUS_EXPR, that's a good thing :-) But isn't it non-selective about the codes where we recurse? Consider ptr = (cond) ? res1 : res2 I think we'll end up recursing on the condition rather than looking at res1 and res2. I suspect there are a very limited number of expression codes that appear on the RHS where we'd want to recurse on one or both operands. POINTER_PLUS_EXPR, NOP_EXPR, maybe COND_EXPR (where you have to recurse on both and logically and the result), BIT_AND (maybe we masked off some bits in an address). That's probably about it :-) Are there any other codes you've seen or think would be useful in practice to recurse through? I'd rather list them explicitly rather than just recurse down through every rhs1 we encounter. > + > + if (code == GIMPLE_PHI && pmaybe) > + { > + unsigned count = 0; > + gphi *phi_stmt = as_a <gphi *> (def_stmt); > + > + unsigned nargs = gimple_phi_num_args (phi_stmt); > + for (unsigned i = 0; i < nargs; ++i) > + { > + if (!visited->add (phi_stmt)) > + { > + tree arg = gimple_phi_arg_def (phi_stmt, i); > + if (is_addr_local (arg, ploc, pmaybe, visited)) > + ++count; > + } > + } So imagine p = phi (x1, x1, x2) Where both x1 and x2 would pass is_addr_local. I think this code would incorrectly set pmaybe. We process the first instance of x1, find it is local and bump count. When we encounter the second instance, it's in our map and we do nothing. THen we process x2 and bump count again. So count would be 2. But count is going to be less than nargs so we'll set *pmaybe to true. ISTM you'd have to record the result for each phi argument and query that to determine if you should bump the counter if you've already visited the argument. It also seems to me that you can break the loop as soon as you've got a nonzero count and get a false return from is_addr_local. Not sure if that'll matter in practice or not. The other approach here (and I'm not suggesting you implement it) would be to use the propagation engine. That's probably overkill here since we'd probably end up computing a whole lot of things we don't need. My sense is an on-demand approach like you've done is probably less computationally expensive. jeff
On Fri, May 31, 2019 at 5:35 PM Jeff Law <law@redhat.com> wrote:> > On 5/30/19 4:56 PM, Martin Sebor wrote: > > On 5/30/19 10:15 AM, Jeff Law wrote: > >> On 5/30/19 9:34 AM, Martin Sebor wrote: > >> > >>>>> If the alias oracle can be used to give the same results without > >>>>> excessive false positives then I think it would be fine to make > >>>>> use of it. Is that something you consider a prerequisite for > >>>>> this change or should I look into it as a followup? > >>>> I think we should explore it a bit before making a final decision. It > >>>> may guide us for other work in this space (like detecting escaping > >>>> locals). I think a dirty prototype to see if it's even in the right > >>>> ballpark would make sense. > >>> > >>> Okay, let me look into it. > >> Sounds good. Again, go with a quick prototype to see if it's likely > >> feasible. The tests you've added should dramatically help evaluating if > >> the oracle is up to the task. > > > > So to expand on what I said on the phone when we spoke: the problem > > I quickly ran into with the prototype is that I wasn't able to find > > a way to identify pointers to alloca/VLA storage. > Your analysis matches my very quick read of the aliasing code. It may > be the case that the Steensgaard patent got in the way here. > > > > > In the the points-to solution for the pointer being returned they > > both have the vars_contains_escaped_heap flag set. That seems like > > an omission that shouldn't be hard to fix, but on its own, I don't > > think it would be sufficient. > RIght. In theory the result of an alloca call shouldn't alias anything > in the heap -- but there were some implementations of alloca that were > built on top of malloc (ugh). That flag may be catering to that case. > > But in the case of a __builtin_alloca that shouldn't apply. Hmm. That > ultimately might be a bug. > > > > > In the IL a VLA is represented as a pointer to an array, but when > > returning a pointer into a VLA (at some offset so it's an SSA_NAME), > > the pointer's point-to solution doesn't include the VLA pointer or > > (AFAICS) make it possible to tell even that it is a VLA. For example > > here: > > > > f (int n) > > { > > int * p; > > int[0:D.1912] * a.1; > > sizetype _1; > > void * saved_stack.3_3; > > sizetype _6; > > > > <bb 2> [local count: 1073741824]: > > saved_stack.3_3 = __builtin_stack_save (); > > _1 = (sizetype) n_2(D); > > _6 = _1 * 4; > > a.1_8 = __builtin_alloca_with_align (_6, 32); > > p_9 = a.1_8 + _6; > > __builtin_stack_restore (saved_stack.3_3); > > return p_9; > > } > > > > p_9's solution's is: > > > > p_9, points-to vars: { D.1925 } (escaped, escaped heap) > > > > I couldn't find out how to determine that D.1925 is a VLA (or even > > what it is). It's not among the function's local variables that > > FOR_EACH_LOCAL_DECL iterates over. > It's possible that decl was created internally as part of the alias > oracle's analysis. Yes. Note that only the UID was reserved the fake decl doesn't live on. Note that for the testcase above the "local" alloca storage escapes which means you run into a catch-22 here given points-to computes a conservative correct solution and you want to detect escaping locals. Usually detecting a pointer to local storage can be done by using ptr_deref_may_alias_global_p but of course in this case the storage was marked global by PTA itself (and our PTA is not flow-sensitive and it doesn't distinguish an escape through a return stmt from an escape through a call which is relevant even for local storage). Feature-wise the PTA solver is missing sth like a "filter" we could put in front of return stmts that doesn't let addresses of locals leak. The simplest way of implementing this might be to not include 'returns' in the constraints at all (in non-IPA mode) and handle them by post-processing the solver result. That gets us some additional flow-sensitivity and a way to filter locals. Let me see if I can cook up this. That may ultimatively also help the warning code where you then can use ptr_deref_may_alias_global_p. Sth like the attached - completely untested (the is_global_var check is likely too simplistic...). It does the job on alloca for me. p_5, points-to NULL, points-to vars: { D.1913 } _6, points-to NULL, points-to vars: { D.1913 } foo (int n) { void * p; long unsigned int _1; void * _6; <bb 2> : _1 = (long unsigned int) n_2(D); p_5 = __builtin_alloca (_1); _6 = p_5; return p_5; Richard. > See make_heapvar in tree-ssa-structalias.c > > > > By replacing the VLA in the test case with a call to malloc I get > > this for the returned p_7: > > > > p_7, points-to NULL, points-to vars: { D.1916 } (escaped, escaped heap) > > > > Again, I see no way to quickly tell that this pointer points into > > the block returned from malloc. > If there's a way to make that determination it'd have to be on the > variable since the pt_solution flag bits don't carry a storage class > directly. > > You might try to get a handle on those decls and dump them to see if > there's anything easily identifiable. But it may be easier to dig into > the code that creates them. A real quick scan of the aliasing code also > shows the "variable_info" structure. It's private to the aliasing code, > but might guide you at things to look at. > > Regardless, I don't see an immediate path forward using the oracle to > identify objects in the stack for your patch. WHich is unfortunate. > > > > > Jeff
On Mon, Jun 3, 2019 at 11:37 AM Richard Biener <richard.guenther@gmail.com> wrote: > > On Fri, May 31, 2019 at 5:35 PM Jeff Law <law@redhat.com> wrote:> > > On 5/30/19 4:56 PM, Martin Sebor wrote: > > > On 5/30/19 10:15 AM, Jeff Law wrote: > > >> On 5/30/19 9:34 AM, Martin Sebor wrote: > > >> > > >>>>> If the alias oracle can be used to give the same results without > > >>>>> excessive false positives then I think it would be fine to make > > >>>>> use of it. Is that something you consider a prerequisite for > > >>>>> this change or should I look into it as a followup? > > >>>> I think we should explore it a bit before making a final decision. It > > >>>> may guide us for other work in this space (like detecting escaping > > >>>> locals). I think a dirty prototype to see if it's even in the right > > >>>> ballpark would make sense. > > >>> > > >>> Okay, let me look into it. > > >> Sounds good. Again, go with a quick prototype to see if it's likely > > >> feasible. The tests you've added should dramatically help evaluating if > > >> the oracle is up to the task. > > > > > > So to expand on what I said on the phone when we spoke: the problem > > > I quickly ran into with the prototype is that I wasn't able to find > > > a way to identify pointers to alloca/VLA storage. > > Your analysis matches my very quick read of the aliasing code. It may > > be the case that the Steensgaard patent got in the way here. > > > > > > > > In the the points-to solution for the pointer being returned they > > > both have the vars_contains_escaped_heap flag set. That seems like > > > an omission that shouldn't be hard to fix, but on its own, I don't > > > think it would be sufficient. > > RIght. In theory the result of an alloca call shouldn't alias anything > > in the heap -- but there were some implementations of alloca that were > > built on top of malloc (ugh). That flag may be catering to that case. > > > > But in the case of a __builtin_alloca that shouldn't apply. Hmm. That > > ultimately might be a bug. > > > > > > > > In the IL a VLA is represented as a pointer to an array, but when > > > returning a pointer into a VLA (at some offset so it's an SSA_NAME), > > > the pointer's point-to solution doesn't include the VLA pointer or > > > (AFAICS) make it possible to tell even that it is a VLA. For example > > > here: > > > > > > f (int n) > > > { > > > int * p; > > > int[0:D.1912] * a.1; > > > sizetype _1; > > > void * saved_stack.3_3; > > > sizetype _6; > > > > > > <bb 2> [local count: 1073741824]: > > > saved_stack.3_3 = __builtin_stack_save (); > > > _1 = (sizetype) n_2(D); > > > _6 = _1 * 4; > > > a.1_8 = __builtin_alloca_with_align (_6, 32); > > > p_9 = a.1_8 + _6; > > > __builtin_stack_restore (saved_stack.3_3); > > > return p_9; > > > } > > > > > > p_9's solution's is: > > > > > > p_9, points-to vars: { D.1925 } (escaped, escaped heap) > > > > > > I couldn't find out how to determine that D.1925 is a VLA (or even > > > what it is). It's not among the function's local variables that > > > FOR_EACH_LOCAL_DECL iterates over. > > It's possible that decl was created internally as part of the alias > > oracle's analysis. > > Yes. Note that only the UID was reserved the fake decl doesn't > live on. > > Note that for the testcase above the "local" alloca storage escapes > which means you run into a catch-22 here given points-to computes > a conservative correct solution and you want to detect escaping > locals. Usually detecting a pointer to local storage can be done > by using ptr_deref_may_alias_global_p but of course in this > case the storage was marked global by PTA itself (and our PTA > is not flow-sensitive and it doesn't distinguish an escape through > a return stmt from an escape through a call which is relevant > even for local storage). > > Feature-wise the PTA solver is missing sth like a "filter" > we could put in front of return stmts that doesn't let > addresses of locals leak. The simplest way of implementing > this might be to not include 'returns' in the constraints at all > (in non-IPA mode) and handle them by post-processing the > solver result. That gets us some additional flow-sensitivity > and a way to filter locals. Let me see if I can cook up this. > > That may ultimatively also help the warning code where you > then can use ptr_deref_may_alias_global_p. > > Sth like the attached - completely untested (the > is_global_var check is likely too simplistic...). It does > the job on alloca for me. > > p_5, points-to NULL, points-to vars: { D.1913 } > _6, points-to NULL, points-to vars: { D.1913 } > > foo (int n) > { > void * p; > long unsigned int _1; > void * _6; > > <bb 2> : > _1 = (long unsigned int) n_2(D); > p_5 = __builtin_alloca (_1); > _6 = p_5; > return p_5; I am testing the following fixed and more complete patch (still the actual return values we process optimally is restricted). I'm not sure whether it will really help real-world testcases since the lack of flow-sensitivity in general and the lack of distinguishing between escapes via calls vs. escapes via return pessimizes things (escapes to global memory complicates things there). Also in IPA mode we cannot post-process returns like in the hack^Wpatch, a "filter" op would need to be added, complicating the solver. Bootstrap / regtest running on x86_64-unknown-linux-gnu. Richard. 2019-06-03 Richard Biener <rguenther@suse.de> * tree-ssa-structalias.c: Include tree-cfg.h. (make_heapvar): Do not make heap vars artificial. (find_func_aliases_for_builtin_call): Handle stack allocation functions. (find_func_aliases): Delay processing of simple enough returns in non-IPA mode. (set_uids_in_ptset): Adjust. (find_what_var_points_to): Likewise. (compute_points_to_sets): Post-process return statements, amending the escaped solution. * gcc.dg/tree-ssa/alias-37.c: New testcase. > > Richard. > > > See make_heapvar in tree-ssa-structalias.c > > > > > > By replacing the VLA in the test case with a call to malloc I get > > > this for the returned p_7: > > > > > > p_7, points-to NULL, points-to vars: { D.1916 } (escaped, escaped heap) > > > > > > Again, I see no way to quickly tell that this pointer points into > > > the block returned from malloc. > > If there's a way to make that determination it'd have to be on the > > variable since the pt_solution flag bits don't carry a storage class > > directly. > > > > You might try to get a handle on those decls and dump them to see if > > there's anything easily identifiable. But it may be easier to dig into > > the code that creates them. A real quick scan of the aliasing code also > > shows the "variable_info" structure. It's private to the aliasing code, > > but might guide you at things to look at. > > > > Regardless, I don't see an immediate path forward using the oracle to > > identify objects in the stack for your patch. WHich is unfortunate. > > > > > > > > > > Jeff
On 6/3/19 3:37 AM, Richard Biener wrote: > On Fri, May 31, 2019 at 5:35 PM Jeff Law <law@redhat.com> wrote:> >> On 5/30/19 4:56 PM, Martin Sebor wrote: >>> On 5/30/19 10:15 AM, Jeff Law wrote: >>>> On 5/30/19 9:34 AM, Martin Sebor wrote: >>>> >>>>>>> If the alias oracle can be used to give the same results without >>>>>>> excessive false positives then I think it would be fine to make >>>>>>> use of it. Is that something you consider a prerequisite for >>>>>>> this change or should I look into it as a followup? >>>>>> I think we should explore it a bit before making a final decision. It >>>>>> may guide us for other work in this space (like detecting escaping >>>>>> locals). I think a dirty prototype to see if it's even in the right >>>>>> ballpark would make sense. >>>>> >>>>> Okay, let me look into it. >>>> Sounds good. Again, go with a quick prototype to see if it's likely >>>> feasible. The tests you've added should dramatically help evaluating if >>>> the oracle is up to the task. >>> >>> So to expand on what I said on the phone when we spoke: the problem >>> I quickly ran into with the prototype is that I wasn't able to find >>> a way to identify pointers to alloca/VLA storage. >> Your analysis matches my very quick read of the aliasing code. It may >> be the case that the Steensgaard patent got in the way here. >> >>> >>> In the the points-to solution for the pointer being returned they >>> both have the vars_contains_escaped_heap flag set. That seems like >>> an omission that shouldn't be hard to fix, but on its own, I don't >>> think it would be sufficient. >> RIght. In theory the result of an alloca call shouldn't alias anything >> in the heap -- but there were some implementations of alloca that were >> built on top of malloc (ugh). That flag may be catering to that case. >> >> But in the case of a __builtin_alloca that shouldn't apply. Hmm. That >> ultimately might be a bug. >> >>> >>> In the IL a VLA is represented as a pointer to an array, but when >>> returning a pointer into a VLA (at some offset so it's an SSA_NAME), >>> the pointer's point-to solution doesn't include the VLA pointer or >>> (AFAICS) make it possible to tell even that it is a VLA. For example >>> here: >>> >>> f (int n) >>> { >>> int * p; >>> int[0:D.1912] * a.1; >>> sizetype _1; >>> void * saved_stack.3_3; >>> sizetype _6; >>> >>> <bb 2> [local count: 1073741824]: >>> saved_stack.3_3 = __builtin_stack_save (); >>> _1 = (sizetype) n_2(D); >>> _6 = _1 * 4; >>> a.1_8 = __builtin_alloca_with_align (_6, 32); >>> p_9 = a.1_8 + _6; >>> __builtin_stack_restore (saved_stack.3_3); >>> return p_9; >>> } >>> >>> p_9's solution's is: >>> >>> p_9, points-to vars: { D.1925 } (escaped, escaped heap) >>> >>> I couldn't find out how to determine that D.1925 is a VLA (or even >>> what it is). It's not among the function's local variables that >>> FOR_EACH_LOCAL_DECL iterates over. >> It's possible that decl was created internally as part of the alias >> oracle's analysis. > > Yes. Note that only the UID was reserved the fake decl doesn't > live on. > > Note that for the testcase above the "local" alloca storage escapes > which means you run into a catch-22 here given points-to computes > a conservative correct solution and you want to detect escaping > locals. Usually detecting a pointer to local storage can be done > by using ptr_deref_may_alias_global_p but of course in this > case the storage was marked global by PTA itself (and our PTA > is not flow-sensitive and it doesn't distinguish an escape through > a return stmt from an escape through a call which is relevant > even for local storage). Good point. The inability to tell why something escaped is another significant hurdle. > > Feature-wise the PTA solver is missing sth like a "filter" > we could put in front of return stmts that doesn't let > addresses of locals leak. The simplest way of implementing > this might be to not include 'returns' in the constraints at all > (in non-IPA mode) and handle them by post-processing the > solver result. That gets us some additional flow-sensitivity > and a way to filter locals. Let me see if I can cook up this. Another thought would be to somehow flag how the pointer escaped. ie, was it as an argument, return value or stored to memory? Though in the end that level of detail may not be useful, so collecting it may not be worth the effort. > > That may ultimatively also help the warning code where you > then can use ptr_deref_may_alias_global_p. Another thought would be to use the alias oracle to prune what we look at. If we ask the oracle and the value doesn't escape, then it's not worth walking up the use-def chain to see where it came from. Jeff
On 5/31/19 2:46 PM, Jeff Law wrote: > On 5/22/19 3:34 PM, Martin Sebor wrote: >> -Wreturn-local-addr detects a subset of instances of returning >> the address of a local object from a function but the warning >> doesn't try to handle alloca or VLAs, or some non-trivial cases >> of ordinary automatic variables[1]. >> >> The attached patch extends the implementation of the warning to >> detect those. It still doesn't detect instances where the address >> is the result of a built-in such strcpy[2]. >> >> Tested on x86_64-linux. >> >> Martin >> >> [1] For example, this is only diagnosed with the patch: >> >> void* f (int i) >> { >> struct S { int a[2]; } s[2]; >> return &s->a[i]; >> } >> >> [2] The following is not diagnosed even with the patch: >> >> void sink (void*); >> >> void* f (int i) >> { >> char a[6]; >> char *p = __builtin_strcpy (a, "123"); >> sink (p); >> return p; >> } >> >> I would expect detecting to be possible and useful. Maybe as >> a follow-up. >> >> gcc-71924.diff >> >> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca result >> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an address of a local array plus offset >> >> gcc/ChangeLog: >> >> PR c/71924 >> * gimple-ssa-isolate-paths.c (is_addr_local): New function. >> (warn_return_addr_local_phi_arg, warn_return_addr_local): Same. >> (find_implicit_erroneous_behavior): Call warn_return_addr_local_phi_arg. >> (find_explicit_erroneous_behavior): Call warn_return_addr_local. >> >> gcc/testsuite/ChangeLog: >> >> PR c/71924 >> * gcc.dg/Wreturn-local-addr-2.c: New test. >> * gcc.dg/Walloca-4.c: Prune expected warnings. >> * gcc.dg/pr41551.c: Same. >> * gcc.dg/pr59523.c: Same. >> * gcc.dg/tree-ssa/pr88775-2.c: Same. >> * gcc.dg/winline-7.c: Same. >> >> diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c >> index 33fe352bb23..2933ecf502e 100644 >> --- a/gcc/gimple-ssa-isolate-paths.c >> +++ b/gcc/gimple-ssa-isolate-paths.c >> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple *stmt) >> return false; >> } >> >> +/* Return true if EXPR is a expression of pointer type that refers >> + to the address of a variable with automatic storage duration. >> + If so, set *PLOC to the location of the object or the call that >> + allocated it (for alloca and VLAs). When PMAYBE is non-null, >> + also consider PHI statements and set *PMAYBE when some but not >> + all arguments of such statements refer to local variables, and >> + to clear it otherwise. */ >> + >> +static bool >> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL, >> + hash_set<gphi *> *visited = NULL) >> +{ >> + if (TREE_CODE (exp) == SSA_NAME) >> + { >> + gimple *def_stmt = SSA_NAME_DEF_STMT (exp); >> + enum gimple_code code = gimple_code (def_stmt); >> + >> + if (is_gimple_assign (def_stmt)) >> + { >> + tree type = TREE_TYPE (gimple_assign_lhs (def_stmt)); >> + if (POINTER_TYPE_P (type)) >> + { >> + tree ptr = gimple_assign_rhs1 (def_stmt); >> + return is_addr_local (ptr, ploc, pmaybe, visited); >> + } >> + return false; >> + } > So this is going to recurse on the rhs1 of something like > POINTER_PLUS_EXPR, that's a good thing :-) But isn't it non-selective > about the codes where we recurse? > > Consider > > ptr = (cond) ? res1 : res2 > > I think we'll end up recursing on the condition rather than looking at > res1 and res2. > > > I suspect there are a very limited number of expression codes that > appear on the RHS where we'd want to recurse on one or both operands. > > POINTER_PLUS_EXPR, NOP_EXPR, maybe COND_EXPR (where you have to recurse > on both and logically and the result), BIT_AND (maybe we masked off some > bits in an address). That's probably about it :-) > > Are there any other codes you've seen or think would be useful in > practice to recurse through? I'd rather list them explicitly rather > than just recurse down through every rhs1 we encounter. I don't have a list of codes to test for. I initially contemplated enumerating them but in the end decided the pointer type check would be sufficient. I wouldn't expect a COND_EXPR here. Don't they get transformed into PHIs? In all my tests they do and and running the whole test suite with an assert that it doesn't come up doesn't expose any either. (I left the assert for COND_EXPR there.) If a COND_EXPR really can come up in a GIMPLE assignment here can you please show me how so I can add a test for it? I've added tests to exercise all C expressions that evaluate to pointers. I don't know of any others where what you bring up should be a concern and I don't want to try to hardwire tests for any that I can't to exercise in the testsuite or don't know how. If you know of some I'm happy to add them and adjust the code. >> + >> + if (code == GIMPLE_PHI && pmaybe) >> + { >> + unsigned count = 0; >> + gphi *phi_stmt = as_a <gphi *> (def_stmt); >> + >> + unsigned nargs = gimple_phi_num_args (phi_stmt); >> + for (unsigned i = 0; i < nargs; ++i) >> + { >> + if (!visited->add (phi_stmt)) >> + { >> + tree arg = gimple_phi_arg_def (phi_stmt, i); >> + if (is_addr_local (arg, ploc, pmaybe, visited)) >> + ++count; >> + } >> + } > So imagine > > p = phi (x1, x1, x2) > > Where both x1 and x2 would pass is_addr_local. I think this code would > incorrectly set pmaybe. I suppose it would but this happens readily with or without my patch, for example here: int* f (int i) { int a[2], b[2]; int *p = i ? a : b; return p; } We end up with two instances of "function may return address of local variable," one for each local, because find_implicit_erroneous_behavior only issues the "may return" kind of warning. I don't particularly like this -- that's why I added PMAYBE to the new code. But to avoid mission creep I'd decided not draw the line there and avoid trying to fix the problem completely. (I've enhanced this in the attached update.) > > We process the first instance of x1, find it is local and bump count. > When we encounter the second instance, it's in our map and we do > nothing. THen we process x2 and bump count again. So count would be 2. > But count is going to be less than nargs so we'll set *pmaybe to true. > > ISTM you'd have to record the result for each phi argument and query > that to determine if you should bump the counter if you've already > visited the argument. I suspect that no solution will be perfect, at a minimum because multiple return statements sometimes get merged into one, so what in the source code is a single return that definitely returns the address a local may end up merged with one that returns a null (and triggering a maybe kind of warning). I have also seen warnings in some non-trivial test cases that suggest that some return statements get split up into two where a "may return" could turn into a "definitely does return." > It also seems to me that you can break the loop as soon as you've got a > nonzero count and get a false return from is_addr_local. Not sure if > that'll matter in practice or not. This is only possible if we're willing to lose some detail (i.e., if we are willing to only point to one variable when returning the address of two or more). I don't find that very user-friendly. To wrap up, while I would have preferred taking a simpler approach at first and making bigger design changes later, I have redesigned the warning for better accuracy as you suggested above. The attached revised patch first collects the return statements in a hash table along with the locations of the local variables whose addresses each statement returns, and then issues just one warning for each statement, listing all the locals it refers to in subsequent notes. In addition, since it was nearly trivial, I also added checking for returning addresses of locals via calls to built-ins like memcpy. I have beefed up the test suite to exercise non-trivial functions involving different kinds of expressions and statements, including loops. Except for one xfail due to missing location information (bug 90735 - missing location in -Wreturn-local-addr on a function with two return statements) I haven't found any issues. The improved warning does find many more problems than the current solution. > The other approach here (and I'm not suggesting you implement it) would > be to use the propagation engine. That's probably overkill here since > we'd probably end up computing a whole lot of things we don't need. My > sense is an on-demand approach like you've done is probably less > computationally expensive. I'm certainly not opposed to making further improvements (as I mentioned, I'd like to add checking for returning freed pointers and freeing locals as you suggested). But I would prefer to make these in incremental steps rather than growing what was at first meant to be just small bug fix/enhancement for alloca and VLAs. The attached update has been tested on x86_64-linux. Martin
On Mon, Jun 3, 2019 at 1:27 PM Richard Biener <richard.guenther@gmail.com> wrote: > > On Mon, Jun 3, 2019 at 11:37 AM Richard Biener > <richard.guenther@gmail.com> wrote: > > > > On Fri, May 31, 2019 at 5:35 PM Jeff Law <law@redhat.com> wrote:> > > > On 5/30/19 4:56 PM, Martin Sebor wrote: > > > > On 5/30/19 10:15 AM, Jeff Law wrote: > > > >> On 5/30/19 9:34 AM, Martin Sebor wrote: > > > >> > > > >>>>> If the alias oracle can be used to give the same results without > > > >>>>> excessive false positives then I think it would be fine to make > > > >>>>> use of it. Is that something you consider a prerequisite for > > > >>>>> this change or should I look into it as a followup? > > > >>>> I think we should explore it a bit before making a final decision. It > > > >>>> may guide us for other work in this space (like detecting escaping > > > >>>> locals). I think a dirty prototype to see if it's even in the right > > > >>>> ballpark would make sense. > > > >>> > > > >>> Okay, let me look into it. > > > >> Sounds good. Again, go with a quick prototype to see if it's likely > > > >> feasible. The tests you've added should dramatically help evaluating if > > > >> the oracle is up to the task. > > > > > > > > So to expand on what I said on the phone when we spoke: the problem > > > > I quickly ran into with the prototype is that I wasn't able to find > > > > a way to identify pointers to alloca/VLA storage. > > > Your analysis matches my very quick read of the aliasing code. It may > > > be the case that the Steensgaard patent got in the way here. > > > > > > > > > > > In the the points-to solution for the pointer being returned they > > > > both have the vars_contains_escaped_heap flag set. That seems like > > > > an omission that shouldn't be hard to fix, but on its own, I don't > > > > think it would be sufficient. > > > RIght. In theory the result of an alloca call shouldn't alias anything > > > in the heap -- but there were some implementations of alloca that were > > > built on top of malloc (ugh). That flag may be catering to that case. > > > > > > But in the case of a __builtin_alloca that shouldn't apply. Hmm. That > > > ultimately might be a bug. > > > > > > > > > > > In the IL a VLA is represented as a pointer to an array, but when > > > > returning a pointer into a VLA (at some offset so it's an SSA_NAME), > > > > the pointer's point-to solution doesn't include the VLA pointer or > > > > (AFAICS) make it possible to tell even that it is a VLA. For example > > > > here: > > > > > > > > f (int n) > > > > { > > > > int * p; > > > > int[0:D.1912] * a.1; > > > > sizetype _1; > > > > void * saved_stack.3_3; > > > > sizetype _6; > > > > > > > > <bb 2> [local count: 1073741824]: > > > > saved_stack.3_3 = __builtin_stack_save (); > > > > _1 = (sizetype) n_2(D); > > > > _6 = _1 * 4; > > > > a.1_8 = __builtin_alloca_with_align (_6, 32); > > > > p_9 = a.1_8 + _6; > > > > __builtin_stack_restore (saved_stack.3_3); > > > > return p_9; > > > > } > > > > > > > > p_9's solution's is: > > > > > > > > p_9, points-to vars: { D.1925 } (escaped, escaped heap) > > > > > > > > I couldn't find out how to determine that D.1925 is a VLA (or even > > > > what it is). It's not among the function's local variables that > > > > FOR_EACH_LOCAL_DECL iterates over. > > > It's possible that decl was created internally as part of the alias > > > oracle's analysis. > > > > Yes. Note that only the UID was reserved the fake decl doesn't > > live on. > > > > Note that for the testcase above the "local" alloca storage escapes > > which means you run into a catch-22 here given points-to computes > > a conservative correct solution and you want to detect escaping > > locals. Usually detecting a pointer to local storage can be done > > by using ptr_deref_may_alias_global_p but of course in this > > case the storage was marked global by PTA itself (and our PTA > > is not flow-sensitive and it doesn't distinguish an escape through > > a return stmt from an escape through a call which is relevant > > even for local storage). > > > > Feature-wise the PTA solver is missing sth like a "filter" > > we could put in front of return stmts that doesn't let > > addresses of locals leak. The simplest way of implementing > > this might be to not include 'returns' in the constraints at all > > (in non-IPA mode) and handle them by post-processing the > > solver result. That gets us some additional flow-sensitivity > > and a way to filter locals. Let me see if I can cook up this. > > > > That may ultimatively also help the warning code where you > > then can use ptr_deref_may_alias_global_p. > > > > Sth like the attached - completely untested (the > > is_global_var check is likely too simplistic...). It does > > the job on alloca for me. > > > > p_5, points-to NULL, points-to vars: { D.1913 } > > _6, points-to NULL, points-to vars: { D.1913 } > > > > foo (int n) > > { > > void * p; > > long unsigned int _1; > > void * _6; > > > > <bb 2> : > > _1 = (long unsigned int) n_2(D); > > p_5 = __builtin_alloca (_1); > > _6 = p_5; > > return p_5; > > I am testing the following fixed and more complete patch (still > the actual return values we process optimally is restricted). > I'm not sure whether it will really help real-world testcases > since the lack of flow-sensitivity in general and the lack of > distinguishing between escapes via calls vs. escapes via > return pessimizes things (escapes to global memory complicates > things there). Also in IPA mode we cannot post-process > returns like in the hack^Wpatch, a "filter" op would need to be > added, complicating the solver. > > Bootstrap / regtest running on x86_64-unknown-linux-gnu. Needs another iteration since it miscompiles gengtype and struct S { int *mem; }; struct S * __attribute__((noinline,noipa)) foo () { struct S *s = __builtin_malloc (sizeof (struct S)); s->mem = __builtin_malloc (sizeof (int)); s->mem[0] = 1; return s; } int main() { struct S *s = foo(); if (s->mem[0] != 1) __builtin_abort (); return 0; } Richard. > Richard. > > 2019-06-03 Richard Biener <rguenther@suse.de> > > * tree-ssa-structalias.c: Include tree-cfg.h. > (make_heapvar): Do not make heap vars artificial. > (find_func_aliases_for_builtin_call): Handle stack allocation > functions. > (find_func_aliases): Delay processing of simple enough returns > in non-IPA mode. > (set_uids_in_ptset): Adjust. > (find_what_var_points_to): Likewise. > (compute_points_to_sets): Post-process return statements, > amending the escaped solution. > > * gcc.dg/tree-ssa/alias-37.c: New testcase. > > > > > Richard. > > > > > See make_heapvar in tree-ssa-structalias.c > > > > > > > > By replacing the VLA in the test case with a call to malloc I get > > > > this for the returned p_7: > > > > > > > > p_7, points-to NULL, points-to vars: { D.1916 } (escaped, escaped heap) > > > > > > > > Again, I see no way to quickly tell that this pointer points into > > > > the block returned from malloc. > > > If there's a way to make that determination it'd have to be on the > > > variable since the pt_solution flag bits don't carry a storage class > > > directly. > > > > > > You might try to get a handle on those decls and dump them to see if > > > there's anything easily identifiable. But it may be easier to dig into > > > the code that creates them. A real quick scan of the aliasing code also > > > shows the "variable_info" structure. It's private to the aliasing code, > > > but might guide you at things to look at. > > > > > > Regardless, I don't see an immediate path forward using the oracle to > > > identify objects in the stack for your patch. WHich is unfortunate. > > > > > > > > > > > > > > > Jeff
On 6/3/19 5:24 PM, Martin Sebor wrote: > On 5/31/19 2:46 PM, Jeff Law wrote: >> On 5/22/19 3:34 PM, Martin Sebor wrote: >>> -Wreturn-local-addr detects a subset of instances of returning >>> the address of a local object from a function but the warning >>> doesn't try to handle alloca or VLAs, or some non-trivial cases >>> of ordinary automatic variables[1]. >>> >>> The attached patch extends the implementation of the warning to >>> detect those. It still doesn't detect instances where the address >>> is the result of a built-in such strcpy[2]. >>> >>> Tested on x86_64-linux. >>> >>> Martin >>> >>> [1] For example, this is only diagnosed with the patch: >>> >>> void* f (int i) >>> { >>> struct S { int a[2]; } s[2]; >>> return &s->a[i]; >>> } >>> >>> [2] The following is not diagnosed even with the patch: >>> >>> void sink (void*); >>> >>> void* f (int i) >>> { >>> char a[6]; >>> char *p = __builtin_strcpy (a, "123"); >>> sink (p); >>> return p; >>> } >>> >>> I would expect detecting to be possible and useful. Maybe as >>> a follow-up. >>> >>> gcc-71924.diff >>> >>> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca >>> result >>> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an >>> address of a local array plus offset >>> >>> gcc/ChangeLog: >>> >>> PR c/71924 >>> * gimple-ssa-isolate-paths.c (is_addr_local): New function. >>> (warn_return_addr_local_phi_arg, warn_return_addr_local): Same. >>> (find_implicit_erroneous_behavior): Call >>> warn_return_addr_local_phi_arg. >>> (find_explicit_erroneous_behavior): Call warn_return_addr_local. >>> >>> gcc/testsuite/ChangeLog: >>> >>> PR c/71924 >>> * gcc.dg/Wreturn-local-addr-2.c: New test. >>> * gcc.dg/Walloca-4.c: Prune expected warnings. >>> * gcc.dg/pr41551.c: Same. >>> * gcc.dg/pr59523.c: Same. >>> * gcc.dg/tree-ssa/pr88775-2.c: Same. >>> * gcc.dg/winline-7.c: Same. >>> >>> diff --git a/gcc/gimple-ssa-isolate-paths.c >>> b/gcc/gimple-ssa-isolate-paths.c >>> index 33fe352bb23..2933ecf502e 100644 >>> --- a/gcc/gimple-ssa-isolate-paths.c >>> +++ b/gcc/gimple-ssa-isolate-paths.c >>> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple >>> *stmt) >>> return false; >>> } >>> +/* Return true if EXPR is a expression of pointer type that refers >>> + to the address of a variable with automatic storage duration. >>> + If so, set *PLOC to the location of the object or the call that >>> + allocated it (for alloca and VLAs). When PMAYBE is non-null, >>> + also consider PHI statements and set *PMAYBE when some but not >>> + all arguments of such statements refer to local variables, and >>> + to clear it otherwise. */ >>> + >>> +static bool >>> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL, >>> + hash_set<gphi *> *visited = NULL) >>> +{ >>> + if (TREE_CODE (exp) == SSA_NAME) >>> + { >>> + gimple *def_stmt = SSA_NAME_DEF_STMT (exp); >>> + enum gimple_code code = gimple_code (def_stmt); >>> + >>> + if (is_gimple_assign (def_stmt)) >>> + { >>> + tree type = TREE_TYPE (gimple_assign_lhs (def_stmt)); >>> + if (POINTER_TYPE_P (type)) >>> + { >>> + tree ptr = gimple_assign_rhs1 (def_stmt); >>> + return is_addr_local (ptr, ploc, pmaybe, visited); >>> + } >>> + return false; >>> + } >> So this is going to recurse on the rhs1 of something like >> POINTER_PLUS_EXPR, that's a good thing :-) But isn't it non-selective >> about the codes where we recurse? >> >> Consider >> >> ptr = (cond) ? res1 : res2 >> >> I think we'll end up recursing on the condition rather than looking at >> res1 and res2. >> >> >> I suspect there are a very limited number of expression codes that >> appear on the RHS where we'd want to recurse on one or both operands. >> >> POINTER_PLUS_EXPR, NOP_EXPR, maybe COND_EXPR (where you have to recurse >> on both and logically and the result), BIT_AND (maybe we masked off some >> bits in an address). That's probably about it :-) >> >> Are there any other codes you've seen or think would be useful in >> practice to recurse through? I'd rather list them explicitly rather >> than just recurse down through every rhs1 we encounter. > > I don't have a list of codes to test for. I initially contemplated > enumerating them but in the end decided the pointer type check would > be sufficient. I wouldn't expect a COND_EXPR here. Don't they get > transformed into PHIs? In all my tests they do and and running > the whole test suite with an assert that it doesn't come up doesn't > expose any either. (I left the assert for COND_EXPR there.) If > a COND_EXPR really can come up in a GIMPLE assignment here can you > please show me how so I can add a test for it? > > I've added tests to exercise all C expressions that evaluate to > pointers. I don't know of any others where what you bring up > should be a concern and I don't want to try to hardwire tests for > any that I can't to exercise in the testsuite or don't know how. > If you know of some I'm happy to add them and adjust the code. > >>> + >>> + if (code == GIMPLE_PHI && pmaybe) >>> + { >>> + unsigned count = 0; >>> + gphi *phi_stmt = as_a <gphi *> (def_stmt); >>> + >>> + unsigned nargs = gimple_phi_num_args (phi_stmt); >>> + for (unsigned i = 0; i < nargs; ++i) >>> + { >>> + if (!visited->add (phi_stmt)) >>> + { >>> + tree arg = gimple_phi_arg_def (phi_stmt, i); >>> + if (is_addr_local (arg, ploc, pmaybe, visited)) >>> + ++count; >>> + } >>> + } >> So imagine >> >> p = phi (x1, x1, x2) >> >> Where both x1 and x2 would pass is_addr_local. I think this code would >> incorrectly set pmaybe. > > I suppose it would but this happens readily with or without my > patch, for example here: > > int* f (int i) > { > int a[2], b[2]; > int *p = i ? a : b; > return p; > } > > We end up with two instances of "function may return address > of local variable," one for each local, because > find_implicit_erroneous_behavior only issues the "may return" > kind of warning. > > I don't particularly like this -- that's why I added PMAYBE to > the new code. But to avoid mission creep I'd decided not draw > the line there and avoid trying to fix the problem completely. > (I've enhanced this in the attached update.) > >> >> We process the first instance of x1, find it is local and bump count. >> When we encounter the second instance, it's in our map and we do >> nothing. THen we process x2 and bump count again. So count would be 2. >> But count is going to be less than nargs so we'll set *pmaybe to true. >> >> ISTM you'd have to record the result for each phi argument and query >> that to determine if you should bump the counter if you've already >> visited the argument. > > I suspect that no solution will be perfect, at a minimum because > multiple return statements sometimes get merged into one, so what > in the source code is a single return that definitely returns > the address a local may end up merged with one that returns > a null (and triggering a maybe kind of warning). I have also > seen warnings in some non-trivial test cases that suggest that > some return statements get split up into two where a "may return" > could turn into a "definitely does return." > >> It also seems to me that you can break the loop as soon as you've got a >> nonzero count and get a false return from is_addr_local. Not sure if >> that'll matter in practice or not. > > This is only possible if we're willing to lose some detail (i.e., > if we are willing to only point to one variable when returning > the address of two or more). I don't find that very user-friendly. > > To wrap up, while I would have preferred taking a simpler approach > at first and making bigger design changes later, I have redesigned > the warning for better accuracy as you suggested above. > > The attached revised patch first collects the return statements > in a hash table along with the locations of the local variables > whose addresses each statement returns, and then issues just one > warning for each statement, listing all the locals it refers to > in subsequent notes. > > In addition, since it was nearly trivial, I also added checking > for returning addresses of locals via calls to built-ins like > memcpy. I didn't fully retest the patch after a minor tweak and sure enough, the tweak was wrong. Please apply the following on top of the patch. (We only want to consider the argument of the call. The LHS is the call itself.) diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c index eb8e754870f..bb9616f26ec 100644 --- a/gcc/gimple-ssa-isolate-paths.c +++ b/gcc/gimple-ssa-isolate-paths.c @@ -475,14 +475,9 @@ is_addr_local (gimple *return_stmt, tree exp, location_t *ploc, case BUILT_IN_STRNCAT_CHK: case BUILT_IN_STRNCPY: case BUILT_IN_STRNCPY_CHK: - /* Check both the argument and the return value. */ - return (is_addr_local (return_stmt, - gimple_call_arg (def_stmt, 0), - ploc, plocmap, visited) - || is_addr_local (return_stmt, - gimple_call_lhs (def_stmt), - ploc, plocmap, visited)); - + return is_addr_local (return_stmt, + gimple_call_arg (def_stmt, 0), + ploc, plocmap, visited); default: return false; } > > I have beefed up the test suite to exercise non-trivial functions > involving different kinds of expressions and statements, including > loops. Except for one xfail due to missing location information > (bug 90735 - missing location in -Wreturn-local-addr on a function > with two return statements) I haven't found any issues. > The improved warning does find many more problems than the current > solution. > >> The other approach here (and I'm not suggesting you implement it) would >> be to use the propagation engine. That's probably overkill here since >> we'd probably end up computing a whole lot of things we don't need. My >> sense is an on-demand approach like you've done is probably less >> computationally expensive. > > I'm certainly not opposed to making further improvements (as I > mentioned, I'd like to add checking for returning freed pointers > and freeing locals as you suggested). But I would prefer to make > these in incremental steps rather than growing what was at first > meant to be just small bug fix/enhancement for alloca and VLAs. > > The attached update has been tested on x86_64-linux. > > Martin
On 6/14/19 2:59 PM, Jeff Law wrote: > On 6/4/19 1:40 PM, Martin Sebor wrote: >> On 6/3/19 5:24 PM, Martin Sebor wrote: >>> On 5/31/19 2:46 PM, Jeff Law wrote: >>>> On 5/22/19 3:34 PM, Martin Sebor wrote: >>>>> -Wreturn-local-addr detects a subset of instances of returning >>>>> the address of a local object from a function but the warning >>>>> doesn't try to handle alloca or VLAs, or some non-trivial cases >>>>> of ordinary automatic variables[1]. >>>>> >>>>> The attached patch extends the implementation of the warning to >>>>> detect those. It still doesn't detect instances where the address >>>>> is the result of a built-in such strcpy[2]. >>>>> >>>>> Tested on x86_64-linux. >>>>> >>>>> Martin >>>>> >>>>> [1] For example, this is only diagnosed with the patch: >>>>> >>>>> void* f (int i) >>>>> { >>>>> struct S { int a[2]; } s[2]; >>>>> return &s->a[i]; >>>>> } >>>>> >>>>> [2] The following is not diagnosed even with the patch: >>>>> >>>>> void sink (void*); >>>>> >>>>> void* f (int i) >>>>> { >>>>> char a[6]; >>>>> char *p = __builtin_strcpy (a, "123"); >>>>> sink (p); >>>>> return p; >>>>> } >>>>> >>>>> I would expect detecting to be possible and useful. Maybe as >>>>> a follow-up. >>>>> >>>>> gcc-71924.diff >>>>> >>>>> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca >>>>> result >>>>> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an >>>>> address of a local array plus offset >>>>> >>>>> gcc/ChangeLog: >>>>> >>>>> PR c/71924 >>>>> * gimple-ssa-isolate-paths.c (is_addr_local): New function. >>>>> (warn_return_addr_local_phi_arg, warn_return_addr_local): Same. >>>>> (find_implicit_erroneous_behavior): Call >>>>> warn_return_addr_local_phi_arg. >>>>> (find_explicit_erroneous_behavior): Call warn_return_addr_local. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> PR c/71924 >>>>> * gcc.dg/Wreturn-local-addr-2.c: New test. >>>>> * gcc.dg/Walloca-4.c: Prune expected warnings. >>>>> * gcc.dg/pr41551.c: Same. >>>>> * gcc.dg/pr59523.c: Same. >>>>> * gcc.dg/tree-ssa/pr88775-2.c: Same. >>>>> * gcc.dg/winline-7.c: Same. >>>>> >>>>> diff --git a/gcc/gimple-ssa-isolate-paths.c >>>>> b/gcc/gimple-ssa-isolate-paths.c >>>>> index 33fe352bb23..2933ecf502e 100644 >>>>> --- a/gcc/gimple-ssa-isolate-paths.c >>>>> +++ b/gcc/gimple-ssa-isolate-paths.c >>>>> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple >>>>> *stmt) >>>>> return false; >>>>> } >>>>> +/* Return true if EXPR is a expression of pointer type that refers >>>>> + to the address of a variable with automatic storage duration. >>>>> + If so, set *PLOC to the location of the object or the call that >>>>> + allocated it (for alloca and VLAs). When PMAYBE is non-null, >>>>> + also consider PHI statements and set *PMAYBE when some but not >>>>> + all arguments of such statements refer to local variables, and >>>>> + to clear it otherwise. */ >>>>> + >>>>> +static bool >>>>> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL, >>>>> + hash_set<gphi *> *visited = NULL) >>>>> +{ >>>>> + if (TREE_CODE (exp) == SSA_NAME) >>>>> + { >>>>> + gimple *def_stmt = SSA_NAME_DEF_STMT (exp); >>>>> + enum gimple_code code = gimple_code (def_stmt); >>>>> + >>>>> + if (is_gimple_assign (def_stmt)) >>>>> + { >>>>> + tree type = TREE_TYPE (gimple_assign_lhs (def_stmt)); >>>>> + if (POINTER_TYPE_P (type)) >>>>> + { >>>>> + tree ptr = gimple_assign_rhs1 (def_stmt); >>>>> + return is_addr_local (ptr, ploc, pmaybe, visited); >>>>> + } >>>>> + return false; >>>>> + } >>>> So this is going to recurse on the rhs1 of something like >>>> POINTER_PLUS_EXPR, that's a good thing :-) But isn't it non-selective >>>> about the codes where we recurse? >>>> >>>> Consider >>>> >>>> ptr = (cond) ? res1 : res2 >>>> >>>> I think we'll end up recursing on the condition rather than looking at >>>> res1 and res2. >>>> >>>> >>>> I suspect there are a very limited number of expression codes that >>>> appear on the RHS where we'd want to recurse on one or both operands. >>>> >>>> POINTER_PLUS_EXPR, NOP_EXPR, maybe COND_EXPR (where you have to recurse >>>> on both and logically and the result), BIT_AND (maybe we masked off some >>>> bits in an address). That's probably about it :-) >>>> >>>> Are there any other codes you've seen or think would be useful in >>>> practice to recurse through? I'd rather list them explicitly rather >>>> than just recurse down through every rhs1 we encounter. >>> >>> I don't have a list of codes to test for. I initially contemplated >>> enumerating them but in the end decided the pointer type check would >>> be sufficient. I wouldn't expect a COND_EXPR here. Don't they get >>> transformed into PHIs? In all my tests they do and and running >>> the whole test suite with an assert that it doesn't come up doesn't >>> expose any either. (I left the assert for COND_EXPR there.) If >>> a COND_EXPR really can come up in a GIMPLE assignment here can you >>> please show me how so I can add a test for it? > A COND_EXPR on the RHS of an assignment is valid gimple. That's what we > need to consider here -- what is and what is not valid gimple. And its > more likely that PHIs will be transformed into RHS COND_EXPRs -- that's > standard practice for if-conversion. > > Gosh, how to get one? It happens all the time :-) Since I know DOM so > well, I just shoved a quick assert into optimize_stmt to abort if we > encounter a gimple assignment where the RHS is a COND_EXPR. It blew up > instantly building libgcc :-) > > COmpile the attached code with -O2 -m32. I wasn't saying it's not valid Gimple, just that I hadn't seen it come up here despite compiling Glibc and the kernel with the patched GCC. The only codes I saw are these: addr_expr, array_ref, bit_and_expr, component_ref, max_expr, mem_ref, nop_expr, parm_decl, pointer_plus_expr, ssa_name, and var_decl What I was asking for is a test case that makes COND_EXPR come up in this context. But I managed to find one by triggering the ICE with GDB. The file reduced to the following test case: extern struct S s; // S has to be an incomplete type void* f (int n) { void *p; int x = 0; for (int i = n; i >= 0; i--) { p = &s; if (p == (void*)-1) x = 1; else if (p) return p; } return x ? (void*)-1 : 0; } and the dump: <bb 6> [local count: 59055800]: # x_10 = PHI <1(5), 0(2)> _5 = x_10 != 0 ? -1B : 0B; <bb 7> [local count: 114863532]: # _3 = PHI <&s(4), _5(6), &s(3)> return _3; It seems a little odd that the COND_EXPR disappears when either of the constant addresses becomes the address of an object (or the result of alloca), and also when the number of iterations of the loop is constant. That's probably why it so rarely comes up in this context. That said, even though I've seen a few references to COND_EXPR in the midle-end I have been assuming that they, in general, do get transformed into PHIs. But checking the internals manual I found in Section 12.6.3 this: A C ?: expression is converted into an if statement with each branch assigning to the same temporary. ... The GIMPLE level if-conversion pass re-introduces ?: expression, if appropriate. It is used to vectorize loops with conditions using vector conditional operations. This GDB test case is likely the result of this reintroduction. > You'll see them for stuff like this: > > > ;; basic block 24, loop depth 0 > ;; pred: 23 > _244 = _1 == 0 ? 1 : 2; > > In a few spots. > > > In addition to COND_EXPR, I'd be worried about VEC_COND_EXPR, > VEC_PERM_EXPR and a host of others. > > And in a more general sense, this kind of permissiveness is not future > proof. What happens if someone needs to add another EXPR node that is > valid on the RHS where such recursion is undesirable? How are they > supposed to know that we've got this permissive recursive call and that > it's going to do the wrong thing? And if it's an EXPR node with no > arguments, then we're going to do a read out of the bounds of the object > and all bets are off at that point (yes we have zero operand EXPR nodes, > but thankfully I don't think they should up in the contexts you care about). Sure. When things are hardcoded this way the same argument can be made both ways. If we just handle A and B and then someone adds an X that matches one of the two, it won't be handled. Either way, some work is necessary to deal with the new Z. One way to avoid it would be by providing an API to query this property of a code (e.g., a function like int gimple_result_aliases_operand (gimple *stmt, int opno)). > I'd be much more comfortable with a tight predicate controlling recursion. I've added ADDR_EXPR, COND_EXPR, MAX_EXPR, MIN_EXPR, NOP_EXPR, and POINTER_PLUS_EXPR based on the survey I did. I don't have tests for the rest so I've left them out for now. If/when I come up with them I'll add them. >>>>> + >>>>> + if (code == GIMPLE_PHI && pmaybe) >>>>> + { >>>>> + unsigned count = 0; >>>>> + gphi *phi_stmt = as_a <gphi *> (def_stmt); >>>>> + >>>>> + unsigned nargs = gimple_phi_num_args (phi_stmt); >>>>> + for (unsigned i = 0; i < nargs; ++i) >>>>> + { >>>>> + if (!visited->add (phi_stmt)) >>>>> + { >>>>> + tree arg = gimple_phi_arg_def (phi_stmt, i); >>>>> + if (is_addr_local (arg, ploc, pmaybe, visited)) >>>>> + ++count; >>>>> + } >>>>> + } >>>> So imagine >>>> >>>> p = phi (x1, x1, x2) >>>> >>>> Where both x1 and x2 would pass is_addr_local. I think this code would >>>> incorrectly set pmaybe. >>> >>> I suppose it would but this happens readily with or without my >>> patch, for example here: > But those seem like distinct issues. The one I pointed out looks like a > pretty simple case to handle. It's just a logic error. It wasn't a logic error. I simply didn't think it was necessary to worry about the accuracy of an inherently inaccurate warning, and I didn't want make more intrusive changes than was called for by my simple fix/enhancement. I was also keeping in mind your preference for smaller change sets. But since you insist I went ahead and improved things. The warning is all the better for it, and the changes I made exposed a number of other bugs in GCC. At the same time, it seems that the bar for a simple bug fixes and small enhancements should not be in excess of what's possible by the original design. Anyway, attached is an updated revision with the recursion limited to just the codes you mentioned, and hopefully with an acceptable accuracy. The patch depends on a fix for the hash_map bug 90923 that I just posted: https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01105.html Martin > >>> >>> int* f (int i) >>> { >>> int a[2], b[2]; >>> int *p = i ? a : b; >>> return p; >>> } >>> >>> We end up with two instances of "function may return address >>> of local variable," one for each local, because >>> find_implicit_erroneous_behavior only issues the "may return" >>> kind of warning. > This looks more like a failing of the core approach in the erroneous > path code. > >>> >>> I suspect that no solution will be perfect, at a minimum because >>> multiple return statements sometimes get merged into one, so what >>> in the source code is a single return that definitely returns >>> the address a local may end up merged with one that returns >>> a null (and triggering a maybe kind of warning). I have also >>> seen warnings in some non-trivial test cases that suggest that >>> some return statements get split up into two where a "may return" >>> could turn into a "definitely does return." > Certainly. I don't expect any solution will be perfect here, but I > think fixing the logic error in the noted loop would help > >>> >>>> It also seems to me that you can break the loop as soon as you've got a >>>> nonzero count and get a false return from is_addr_local. Not sure if >>>> that'll matter in practice or not. >>> >>> This is only possible if we're willing to lose some detail (i.e., >>> if we are willing to only point to one variable when returning >>> the address of two or more). I don't find that very user-friendly. > ACK. Ignore that comment then. > > > Jeff >
Ping: did my reply and updated patch resolve your concerns? https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01106.html On 6/18/19 9:19 PM, Martin Sebor wrote: > On 6/14/19 2:59 PM, Jeff Law wrote: >> On 6/4/19 1:40 PM, Martin Sebor wrote: >>> On 6/3/19 5:24 PM, Martin Sebor wrote: >>>> On 5/31/19 2:46 PM, Jeff Law wrote: >>>>> On 5/22/19 3:34 PM, Martin Sebor wrote: >>>>>> -Wreturn-local-addr detects a subset of instances of returning >>>>>> the address of a local object from a function but the warning >>>>>> doesn't try to handle alloca or VLAs, or some non-trivial cases >>>>>> of ordinary automatic variables[1]. >>>>>> >>>>>> The attached patch extends the implementation of the warning to >>>>>> detect those. It still doesn't detect instances where the address >>>>>> is the result of a built-in such strcpy[2]. >>>>>> >>>>>> Tested on x86_64-linux. >>>>>> >>>>>> Martin >>>>>> >>>>>> [1] For example, this is only diagnosed with the patch: >>>>>> >>>>>> void* f (int i) >>>>>> { >>>>>> struct S { int a[2]; } s[2]; >>>>>> return &s->a[i]; >>>>>> } >>>>>> >>>>>> [2] The following is not diagnosed even with the patch: >>>>>> >>>>>> void sink (void*); >>>>>> >>>>>> void* f (int i) >>>>>> { >>>>>> char a[6]; >>>>>> char *p = __builtin_strcpy (a, "123"); >>>>>> sink (p); >>>>>> return p; >>>>>> } >>>>>> >>>>>> I would expect detecting to be possible and useful. Maybe as >>>>>> a follow-up. >>>>>> >>>>>> gcc-71924.diff >>>>>> >>>>>> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca >>>>>> result >>>>>> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an >>>>>> address of a local array plus offset >>>>>> >>>>>> gcc/ChangeLog: >>>>>> >>>>>> PR c/71924 >>>>>> * gimple-ssa-isolate-paths.c (is_addr_local): New function. >>>>>> (warn_return_addr_local_phi_arg, warn_return_addr_local): Same. >>>>>> (find_implicit_erroneous_behavior): Call >>>>>> warn_return_addr_local_phi_arg. >>>>>> (find_explicit_erroneous_behavior): Call warn_return_addr_local. >>>>>> >>>>>> gcc/testsuite/ChangeLog: >>>>>> >>>>>> PR c/71924 >>>>>> * gcc.dg/Wreturn-local-addr-2.c: New test. >>>>>> * gcc.dg/Walloca-4.c: Prune expected warnings. >>>>>> * gcc.dg/pr41551.c: Same. >>>>>> * gcc.dg/pr59523.c: Same. >>>>>> * gcc.dg/tree-ssa/pr88775-2.c: Same. >>>>>> * gcc.dg/winline-7.c: Same. >>>>>> >>>>>> diff --git a/gcc/gimple-ssa-isolate-paths.c >>>>>> b/gcc/gimple-ssa-isolate-paths.c >>>>>> index 33fe352bb23..2933ecf502e 100644 >>>>>> --- a/gcc/gimple-ssa-isolate-paths.c >>>>>> +++ b/gcc/gimple-ssa-isolate-paths.c >>>>>> @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple >>>>>> *stmt) >>>>>> return false; >>>>>> } >>>>>> +/* Return true if EXPR is a expression of pointer type that refers >>>>>> + to the address of a variable with automatic storage duration. >>>>>> + If so, set *PLOC to the location of the object or the call that >>>>>> + allocated it (for alloca and VLAs). When PMAYBE is non-null, >>>>>> + also consider PHI statements and set *PMAYBE when some but not >>>>>> + all arguments of such statements refer to local variables, and >>>>>> + to clear it otherwise. */ >>>>>> + >>>>>> +static bool >>>>>> +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL, >>>>>> + hash_set<gphi *> *visited = NULL) >>>>>> +{ >>>>>> + if (TREE_CODE (exp) == SSA_NAME) >>>>>> + { >>>>>> + gimple *def_stmt = SSA_NAME_DEF_STMT (exp); >>>>>> + enum gimple_code code = gimple_code (def_stmt); >>>>>> + >>>>>> + if (is_gimple_assign (def_stmt)) >>>>>> + { >>>>>> + tree type = TREE_TYPE (gimple_assign_lhs (def_stmt)); >>>>>> + if (POINTER_TYPE_P (type)) >>>>>> + { >>>>>> + tree ptr = gimple_assign_rhs1 (def_stmt); >>>>>> + return is_addr_local (ptr, ploc, pmaybe, visited); >>>>>> + } >>>>>> + return false; >>>>>> + } >>>>> So this is going to recurse on the rhs1 of something like >>>>> POINTER_PLUS_EXPR, that's a good thing :-) But isn't it >>>>> non-selective >>>>> about the codes where we recurse? >>>>> >>>>> Consider >>>>> >>>>> ptr = (cond) ? res1 : res2 >>>>> >>>>> I think we'll end up recursing on the condition rather than looking at >>>>> res1 and res2. >>>>> >>>>> >>>>> I suspect there are a very limited number of expression codes that >>>>> appear on the RHS where we'd want to recurse on one or both operands. >>>>> >>>>> POINTER_PLUS_EXPR, NOP_EXPR, maybe COND_EXPR (where you have to >>>>> recurse >>>>> on both and logically and the result), BIT_AND (maybe we masked off >>>>> some >>>>> bits in an address). That's probably about it :-) >>>>> >>>>> Are there any other codes you've seen or think would be useful in >>>>> practice to recurse through? I'd rather list them explicitly rather >>>>> than just recurse down through every rhs1 we encounter. >>>> >>>> I don't have a list of codes to test for. I initially contemplated >>>> enumerating them but in the end decided the pointer type check would >>>> be sufficient. I wouldn't expect a COND_EXPR here. Don't they get >>>> transformed into PHIs? In all my tests they do and and running >>>> the whole test suite with an assert that it doesn't come up doesn't >>>> expose any either. (I left the assert for COND_EXPR there.) If >>>> a COND_EXPR really can come up in a GIMPLE assignment here can you >>>> please show me how so I can add a test for it? >> A COND_EXPR on the RHS of an assignment is valid gimple. That's what we >> need to consider here -- what is and what is not valid gimple. And its >> more likely that PHIs will be transformed into RHS COND_EXPRs -- that's >> standard practice for if-conversion. >> >> Gosh, how to get one? It happens all the time :-) Since I know DOM so >> well, I just shoved a quick assert into optimize_stmt to abort if we >> encounter a gimple assignment where the RHS is a COND_EXPR. It blew up >> instantly building libgcc :-) >> >> COmpile the attached code with -O2 -m32. > > I wasn't saying it's not valid Gimple, just that I hadn't seen it > come up here despite compiling Glibc and the kernel with the patched > GCC. The only codes I saw are these: > > addr_expr, array_ref, bit_and_expr, component_ref, max_expr, > mem_ref, nop_expr, parm_decl, pointer_plus_expr, ssa_name, > and var_decl > > What I was asking for is a test case that makes COND_EXPR come up > in this context. But I managed to find one by triggering the ICE > with GDB. The file reduced to the following test case: > > extern struct S s; // S has to be an incomplete type > > void* f (int n) > { > void *p; > int x = 0; > > for (int i = n; i >= 0; i--) > { > p = &s; > if (p == (void*)-1) > x = 1; > else if (p) > return p; > } > > return x ? (void*)-1 : 0; > } > > and the dump: > > <bb 6> [local count: 59055800]: > # x_10 = PHI <1(5), 0(2)> > _5 = x_10 != 0 ? -1B : 0B; > > <bb 7> [local count: 114863532]: > # _3 = PHI <&s(4), _5(6), &s(3)> > return _3; > > It seems a little odd that the COND_EXPR disappears when either > of the constant addresses becomes the address of an object (or > the result of alloca), and also when the number of iterations > of the loop is constant. That's probably why it so rarely comes > up in this context. > > That said, even though I've seen a few references to COND_EXPR > in the midle-end I have been assuming that they, in general, do > get transformed into PHIs. But checking the internals manual > I found in Section 12.6.3 this: > > A C ?: expression is converted into an if statement with each > branch assigning to the same temporary. ... The GIMPLE level > if-conversion pass re-introduces ?: expression, if appropriate. > It is used to vectorize loops with conditions using vector > conditional operations. > > This GDB test case is likely the result of this reintroduction. > >> You'll see them for stuff like this: >> >> >> ;; basic block 24, loop depth 0 >> ;; pred: 23 >> _244 = _1 == 0 ? 1 : 2; >> >> In a few spots. >> >> >> In addition to COND_EXPR, I'd be worried about VEC_COND_EXPR, >> VEC_PERM_EXPR and a host of others. >> >> And in a more general sense, this kind of permissiveness is not future >> proof. What happens if someone needs to add another EXPR node that is >> valid on the RHS where such recursion is undesirable? How are they >> supposed to know that we've got this permissive recursive call and that >> it's going to do the wrong thing? And if it's an EXPR node with no >> arguments, then we're going to do a read out of the bounds of the object >> and all bets are off at that point (yes we have zero operand EXPR nodes, >> but thankfully I don't think they should up in the contexts you care >> about). > > Sure. When things are hardcoded this way the same argument can > be made both ways. If we just handle A and B and then someone > adds an X that matches one of the two, it won't be handled. Either > way, some work is necessary to deal with the new Z. One way to > avoid it would be by providing an API to query this property of > a code (e.g., a function like int gimple_result_aliases_operand > (gimple *stmt, int opno)). > >> I'd be much more comfortable with a tight predicate controlling >> recursion. > > I've added ADDR_EXPR, COND_EXPR, MAX_EXPR, MIN_EXPR, NOP_EXPR, and > POINTER_PLUS_EXPR based on the survey I did. I don't have tests for > the rest so I've left them out for now. If/when I come up with them > I'll add them. > >>>>>> + >>>>>> + if (code == GIMPLE_PHI && pmaybe) >>>>>> + { >>>>>> + unsigned count = 0; >>>>>> + gphi *phi_stmt = as_a <gphi *> (def_stmt); >>>>>> + >>>>>> + unsigned nargs = gimple_phi_num_args (phi_stmt); >>>>>> + for (unsigned i = 0; i < nargs; ++i) >>>>>> + { >>>>>> + if (!visited->add (phi_stmt)) >>>>>> + { >>>>>> + tree arg = gimple_phi_arg_def (phi_stmt, i); >>>>>> + if (is_addr_local (arg, ploc, pmaybe, visited)) >>>>>> + ++count; >>>>>> + } >>>>>> + } >>>>> So imagine >>>>> >>>>> p = phi (x1, x1, x2) >>>>> >>>>> Where both x1 and x2 would pass is_addr_local. I think this code >>>>> would >>>>> incorrectly set pmaybe. >>>> >>>> I suppose it would but this happens readily with or without my >>>> patch, for example here: >> But those seem like distinct issues. The one I pointed out looks like a >> pretty simple case to handle. It's just a logic error. > > It wasn't a logic error. I simply didn't think it was necessary > to worry about the accuracy of an inherently inaccurate warning, > and I didn't want make more intrusive changes than was called for > by my simple fix/enhancement. I was also keeping in mind your > preference for smaller change sets. But since you insist I went > ahead and improved things. The warning is all the better for it, > and the changes I made exposed a number of other bugs in GCC. > At the same time, it seems that the bar for a simple bug fixes > and small enhancements should not be in excess of what's possible > by the original design. > > Anyway, attached is an updated revision with the recursion limited > to just the codes you mentioned, and hopefully with an acceptable > accuracy. The patch depends on a fix for the hash_map bug 90923 > that I just posted: > https://gcc.gnu.org/ml/gcc-patches/2019-06/msg01105.html > > Martin > >> >>>> >>>> int* f (int i) >>>> { >>>> int a[2], b[2]; >>>> int *p = i ? a : b; >>>> return p; >>>> } >>>> >>>> We end up with two instances of "function may return address >>>> of local variable," one for each local, because >>>> find_implicit_erroneous_behavior only issues the "may return" >>>> kind of warning. >> This looks more like a failing of the core approach in the erroneous >> path code. >> >>>> >>>> I suspect that no solution will be perfect, at a minimum because >>>> multiple return statements sometimes get merged into one, so what >>>> in the source code is a single return that definitely returns >>>> the address a local may end up merged with one that returns >>>> a null (and triggering a maybe kind of warning). I have also >>>> seen warnings in some non-trivial test cases that suggest that >>>> some return statements get split up into two where a "may return" >>>> could turn into a "definitely does return." >> Certainly. I don't expect any solution will be perfect here, but I >> think fixing the logic error in the noted loop would help >> >>>> >>>>> It also seems to me that you can break the loop as soon as you've >>>>> got a >>>>> nonzero count and get a false return from is_addr_local. Not sure if >>>>> that'll matter in practice or not. >>>> >>>> This is only possible if we're willing to lose some detail (i.e., >>>> if we are willing to only point to one variable when returning >>>> the address of two or more). I don't find that very user-friendly. >> ACK. Ignore that comment then. >> >> >> Jeff >> >
On 6/18/19 9:19 PM, Martin Sebor wrote: > On 6/14/19 2:59 PM, Jeff Law wrote: [ big snip ] >> A COND_EXPR on the RHS of an assignment is valid gimple. That's what we >> need to consider here -- what is and what is not valid gimple. And its >> more likely that PHIs will be transformed into RHS COND_EXPRs -- that's >> standard practice for if-conversion. >> >> Gosh, how to get one? It happens all the time :-) Since I know DOM so >> well, I just shoved a quick assert into optimize_stmt to abort if we >> encounter a gimple assignment where the RHS is a COND_EXPR. It blew up >> instantly building libgcc :-) >> >> COmpile the attached code with -O2 -m32. > > I wasn't saying it's not valid Gimple, just that I hadn't seen it > come up here despite compiling Glibc and the kernel with the patched > GCC. The only codes I saw are these: > > addr_expr, array_ref, bit_and_expr, component_ref, max_expr, > mem_ref, nop_expr, parm_decl, pointer_plus_expr, ssa_name, > and var_decl The only one here that's really surprising is the MAX_EXPR. But it is what it is. > > What I was asking for is a test case that makes COND_EXPR come up > in this context. But I managed to find one by triggering the ICE > with GDB. The file reduced to the following test case: Sorry. email can be a tough medium to nail down specific details. > > extern struct S s; // S has to be an incomplete type > > void* f (int n) > { > void *p; > int x = 0; > > for (int i = n; i >= 0; i--) > { > p = &s; > if (p == (void*)-1) > x = 1; > else if (p) > return p; > } > > return x ? (void*)-1 : 0; > } > > and the dump: > > <bb 6> [local count: 59055800]: > # x_10 = PHI <1(5), 0(2)> > _5 = x_10 != 0 ? -1B : 0B; > > <bb 7> [local count: 114863532]: > # _3 = PHI <&s(4), _5(6), &s(3)> > return _3; > > It seems a little odd that the COND_EXPR disappears when either > of the constant addresses becomes the address of an object (or > the result of alloca), and also when the number of iterations > of the loop is constant. That's probably why it so rarely comes > up in this context. Going into phiopt2 we have: ;; basic block 6, loop depth 0 ;; pred: 5 if (x_1 != 0) goto <bb 8>; [71.00%] else goto <bb 7>; [29.00%] ;; succ: 8 ;; 7 ;; basic block 7, loop depth 0 ;; pred: 6 ;; succ: 8 ;; basic block 8, loop depth 0 ;; pred: 3 ;; 7 ;; 6 # _3 = PHI <&s(3), 0B(7), -1B(6)> return _3; The subgraph starting at block #6 is a classic case for turning branchy code into straightline code using a COND_EXPR on the RHS of an assignment. So you end up with something like this: ;; basic block 6, loop depth 0 ;; pred: 5 _5 = x_1 != 0 ? -1B : 0B; ;; succ: 7 ;; basic block 7, loop depth 0 ;; pred: 3 ;; 6 # _3 = PHI <&s(3), _5(6)> return _3; Now for this specific case within phiopt we are limited to cases there the result is 0/1 or 0/-1. That's why you get something different when you exchange one of the constants for the address of an object, or anything else for that matter. This is all a bit academic -- the key point is that we can have a COND_EXPR on the RHS of an assignment. That's allowed by gimple. Sadly this is also likely one of the places where target characteristics come into play -- targets define a BRANCH_COST which can significantly change the decisions for the initial generation of conditionals. It's one of the things that makes writing tests for jump threading, if conversion and other optimizations so damn painful -- on one target we'll have a series of conditional jumps, on anothers we may have a series of logicals, potentially with COND_EXPRs. > > That said, even though I've seen a few references to COND_EXPR > in the midle-end I have been assuming that they, in general, do > get transformed into PHIs. But checking the internals manual > I found in Section 12.6.3 this: > > A C ?: expression is converted into an if statement with each > branch assigning to the same temporary. ... The GIMPLE level > if-conversion pass re-introduces ?: expression, if appropriate. > It is used to vectorize loops with conditions using vector > conditional operations. > > This GDB test case is likely the result of this reintroduction. Nope. It happens much earlier in the pipeline :-) >> >> And in a more general sense, this kind of permissiveness is not future >> proof. What happens if someone needs to add another EXPR node that is >> valid on the RHS where such recursion is undesirable? How are they >> supposed to know that we've got this permissive recursive call and that >> it's going to do the wrong thing? And if it's an EXPR node with no >> arguments, then we're going to do a read out of the bounds of the object >> and all bets are off at that point (yes we have zero operand EXPR nodes, >> but thankfully I don't think they should up in the contexts you care about). >> > > Sure. When things are hardcoded this way the same argument can > be made both ways. If we just handle A and B and then someone > adds an X that matches one of the two, it won't be handled. Either > way, some work is necessary to deal with the new Z. One way to > avoid it would be by providing an API to query this property of > a code (e.g., a function like int gimple_result_aliases_operand > (gimple *stmt, int opno)). Adding a new opcode could have caused the original code to generate incorrect code. In the new version an indirect opcode would just result in something we refuse to analyze -- so we could miss a warning, but more importantly we would _not_ transform the return statement so we'd be safe WRT correct code generation. WRT making an API to do these kinds of queries. Sure. It's a fairly natural extension to stuff we've done in the past. You could (for example) wrap up all kinds existing properties of nodes into an API. You'll see that's already done in a fairly ad-hoc fashion, but it's not even close to being pervasive or standard practice. I would certainly applaud bringing some sanity to this kind of issue and iterating towards a better place. But it's ugly, painful work with little visible end user benefit. > >> But those seem like distinct issues. The one I pointed out looks like a >> pretty simple case to handle. It's just a logic error. > > It wasn't a logic error. I simply didn't think it was necessary > to worry about the accuracy of an inherently inaccurate warning, > and I didn't want make more intrusive changes than was called for > by my simple fix/enhancement. I was also keeping in mind your > preference for smaller change sets. But since you insist I went > ahead and improved things. The warning is all the better for it, > and the changes I made exposed a number of other bugs in GCC. > At the same time, it seems that the bar for a simple bug fixes > and small enhancements should not be in excess of what's possible > by the original design. It looked like a logic error to me that could be easily fixed. I didn't want to make it out to a huge deal. ANd yes, there's a natural tension between addressing small stuff as you see them in a larger kit and breaking things down and iterating. There aren't hard and fast rules here. > > > gcc-71924.diff > > PR middle-end/71924 - missing -Wreturn-local-addr returning alloca result > PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an address of a local array plus offset > > gcc/ChangeLog: > > PR middle-end/71924 > PR middle-end/90549 > * gimple-ssa-isolate-paths.c (isolate_path): Add attribute. > (args_loc_t): New type. > (args_loc_t, locmap_t): same. > (diag_returned_locals): New function. > (is_addr_local): Same. > (handle_return_addr_local_phi_arg, warn_return_addr_local): Same. > (find_implicit_erroneous_behavior): Call warn_return_addr_local_phi_arg. > (find_explicit_erroneous_behavior): Call warn_return_addr_local. > > libgcc/ChangeLog: > * generic-morestack.c: Disable -fisolate-erroneous-paths-dereference > to get around PR libgcc/90918. > > gcc/testsuite/ChangeLog: > > PR middle-end/71924 > PR middle-end/90549 > * gcc.dg/Wreturn-local-addr-2.c: New test. > * gcc.dg/Wreturn-local-addr-4.c: New test. > * gcc.dg/Wreturn-local-addr-5.c: New test. > * gcc.dg/Wreturn-local-addr-6.c: New test. > * gcc.dg/Wreturn-local-addr-7.c: New test. > * gcc.dg/Wreturn-local-addr-8.c: New test. > * gcc.dg/Walloca-4.c: Prune expected warnings. > * gcc.dg/pr41551.c: Same. > * gcc.dg/pr59523.c: Same. > * gcc.dg/tree-ssa/pr88775-2.c: Same. > * gcc.dg/winline-7.c: Same. > > diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c > index 33fe352bb23..13b1f5f2349 100644 > --- a/gcc/gimple-ssa-isolate-paths.c > +++ b/gcc/gimple-ssa-isolate-paths.c > @@ -130,7 +130,7 @@ insert_trap (gimple_stmt_iterator *si_p, tree op) > > Return BB'. */ > > -basic_block > +ATTRIBUTE_RETURNS_NONNULL basic_block > isolate_path (basic_block bb, basic_block duplicate, > edge e, gimple *stmt, tree op, bool ret_zero) > { > @@ -341,6 +341,283 @@ stmt_uses_0_or_null_in_undefined_way (gimple *stmt) > return false; > } > > +/* Describes the property of a return statement that may return > + the address of one or more local variables. */ > +struct args_loc_t > +{ > + args_loc_t (): nargs (), locvec (), ptr (&ptr) > + { > + locvec.create (4); > + } > + > + args_loc_t (const args_loc_t &rhs) > + : nargs (rhs.nargs), locvec (rhs.locvec.copy ()), ptr (&ptr) { } > + > + args_loc_t& operator= (const args_loc_t &rhs) > + { > + nargs = rhs.nargs; > + locvec.release (); > + locvec = rhs.locvec.copy (); > + return *this; > + } > + > + ~args_loc_t () > + { > + locvec.release (); > + gcc_assert (ptr == &ptr); > + } > + > + /* For a PHI in a return statement its number of arguments. When less > + than LOCVEC.LENGTH () implies that an address of local may but need > + not be returned by the statement. Otherwise it implies it definitely > + is returned. Used to issue "may be" vs "is" diagnostics. */ > + unsigned nargs; > + /* The locations of local variables/alloca calls returned by the return > + statement. Avoid using auto_vec here since it's not safe to copy due > + to pr90904. */ > + vec <location_t> locvec; > + void *ptr; > +}; Make this a class, please. We use "struct" for POD. Is there a strong need for an overloaded operator? Our guidelines generally discourage operator overloads. This one seems on the border to me. Others may have different ideas where the line is. If we really don't need the overloaded operator, then we can avoid the controversy completely. > + > +/* Return true if EXPR is a expression of pointer type that refers > + to the address of a variable with automatic storage duration. > + If so, add an entry to *PLOCMAP and insert INTO PLOCMAP->LOCVEC > + the locations of the corresponding local variables whose address > + is returned by the statement. VISITED is a bitmap of PHI nodes > + already visited by recursive calls. */ > + > +static bool > +is_addr_local (gimple *return_stmt, tree exp, locmap_t *plocmap, > + hash_set<gphi *> *visited) > +{ > + if (TREE_CODE (exp) == ADDR_EXPR) > + { > + tree baseaddr = get_base_address (TREE_OPERAND (exp, 0)); > + if (TREE_CODE (baseaddr) == MEM_REF) > + return is_addr_local (return_stmt, TREE_OPERAND (baseaddr, 0), > + plocmap, visited); > + > + if ((!VAR_P (baseaddr) > + || is_global_var (baseaddr)) > + && TREE_CODE (baseaddr) != PARM_DECL) > + return false; > + > + args_loc_t &argsloc = plocmap->get_or_insert (return_stmt); > + argsloc.locvec.safe_push (DECL_SOURCE_LOCATION (baseaddr)); > + return true; > + } > + > + if (!POINTER_TYPE_P (TREE_TYPE (exp))) > + return false; > + > + if (TREE_CODE (exp) == SSA_NAME) > + { > + gimple *def_stmt = SSA_NAME_DEF_STMT (exp); > + enum gimple_code code = gimple_code (def_stmt); > + > + if (is_gimple_assign (def_stmt)) > + { > + tree type = TREE_TYPE (gimple_assign_lhs (def_stmt)); > + if (POINTER_TYPE_P (type)) > + { > + tree_code code = gimple_assign_rhs_code (def_stmt); > + tree ptr1 = NULL_TREE, ptr2 = NULL_TREE; > + if (code == COND_EXPR) > + { > + ptr1 = gimple_assign_rhs2 (def_stmt); > + ptr2 = gimple_assign_rhs3 (def_stmt); > + } > + else if (code == MAX_EXPR || code == MIN_EXPR) > + { > + ptr1 = gimple_assign_rhs1 (def_stmt); > + ptr2 = gimple_assign_rhs2 (def_stmt); > + } > + else if (code == ADDR_EXPR > + || code == NOP_EXPR > + || code == POINTER_PLUS_EXPR) > + ptr1 = gimple_assign_rhs1 (def_stmt); > + > + /* Avoid short-circuiting the logical OR result in case > + both oerands refer to local variables, in which case > + both should be identified in the warning. */ > + bool res1 = false, res2 = false; > + if (ptr1) > + res1 = is_addr_local (return_stmt, ptr1, plocmap, visited); > + if (ptr2) > + res2 = is_addr_local (return_stmt, ptr2, plocmap, visited); > + return res1 || res2; > + } > + return false; > + } s/oerands/operands/ a few lines up. So how do you deal with the case where one operand of a {MIN,MAX,COND}_EXPR is the address of a local, but the other is not? It seems like we'll end up returning true from this function in that case and potentially trigger changing the return value to NULL. Or am I missing something? Jeff
On 6/26/19 6:11 PM, Jeff Law wrote: > On 6/18/19 9:19 PM, Martin Sebor wrote: >> On 6/14/19 2:59 PM, Jeff Law wrote: > [ big snip ] >>> A COND_EXPR on the RHS of an assignment is valid gimple. That's what we >>> need to consider here -- what is and what is not valid gimple. And its >>> more likely that PHIs will be transformed into RHS COND_EXPRs -- that's >>> standard practice for if-conversion. >>> >>> Gosh, how to get one? It happens all the time :-) Since I know DOM so >>> well, I just shoved a quick assert into optimize_stmt to abort if we >>> encounter a gimple assignment where the RHS is a COND_EXPR. It blew up >>> instantly building libgcc :-) >>> >>> COmpile the attached code with -O2 -m32. >> >> I wasn't saying it's not valid Gimple, just that I hadn't seen it >> come up here despite compiling Glibc and the kernel with the patched >> GCC. The only codes I saw are these: >> >> addr_expr, array_ref, bit_and_expr, component_ref, max_expr, >> mem_ref, nop_expr, parm_decl, pointer_plus_expr, ssa_name, >> and var_decl > The only one here that's really surprising is the MAX_EXPR. But it is > what it is. > >> >> What I was asking for is a test case that makes COND_EXPR come up >> in this context. But I managed to find one by triggering the ICE >> with GDB. The file reduced to the following test case: > Sorry. email can be a tough medium to nail down specific details. > >> >> extern struct S s; // S has to be an incomplete type >> >> void* f (int n) >> { >> void *p; >> int x = 0; >> >> for (int i = n; i >= 0; i--) >> { >> p = &s; >> if (p == (void*)-1) >> x = 1; >> else if (p) >> return p; >> } >> >> return x ? (void*)-1 : 0; >> } >> >> and the dump: >> >> <bb 6> [local count: 59055800]: >> # x_10 = PHI <1(5), 0(2)> >> _5 = x_10 != 0 ? -1B : 0B; >> >> <bb 7> [local count: 114863532]: >> # _3 = PHI <&s(4), _5(6), &s(3)> >> return _3; >> >> It seems a little odd that the COND_EXPR disappears when either >> of the constant addresses becomes the address of an object (or >> the result of alloca), and also when the number of iterations >> of the loop is constant. That's probably why it so rarely comes >> up in this context. > Going into phiopt2 we have: > > ;; basic block 6, loop depth 0 > ;; pred: 5 > if (x_1 != 0) > goto <bb 8>; [71.00%] > else > goto <bb 7>; [29.00%] > ;; succ: 8 > ;; 7 > > ;; basic block 7, loop depth 0 > ;; pred: 6 > ;; succ: 8 > > ;; basic block 8, loop depth 0 > ;; pred: 3 > ;; 7 > ;; 6 > # _3 = PHI <&s(3), 0B(7), -1B(6)> > return _3; > > The subgraph starting at block #6 is a classic case for turning branchy > code into straightline code using a COND_EXPR on the RHS of an > assignment. So you end up with something like this: > > ;; basic block 6, loop depth 0 > ;; pred: 5 > _5 = x_1 != 0 ? -1B : 0B; > ;; succ: 7 > > ;; basic block 7, loop depth 0 > ;; pred: 3 > ;; 6 > # _3 = PHI <&s(3), _5(6)> > return _3; > > > Now for this specific case within phiopt we are limited to cases there > the result is 0/1 or 0/-1. That's why you get something different when > you exchange one of the constants for the address of an object, or > anything else for that matter. > > This is all a bit academic -- the key point is that we can have a > COND_EXPR on the RHS of an assignment. That's allowed by gimple. > > Sadly this is also likely one of the places where target characteristics > come into play -- targets define a BRANCH_COST which can significantly > change the decisions for the initial generation of conditionals. It's > one of the things that makes writing tests for jump threading, if > conversion and other optimizations so damn painful -- on one target > we'll have a series of conditional jumps, on anothers we may have a > series of logicals, potentially with COND_EXPRs. > > >> >> That said, even though I've seen a few references to COND_EXPR >> in the midle-end I have been assuming that they, in general, do >> get transformed into PHIs. But checking the internals manual >> I found in Section 12.6.3 this: >> >> A C ?: expression is converted into an if statement with each >> branch assigning to the same temporary. ... The GIMPLE level >> if-conversion pass re-introduces ?: expression, if appropriate. >> It is used to vectorize loops with conditions using vector >> conditional operations. >> >> This GDB test case is likely the result of this reintroduction. > Nope. It happens much earlier in the pipeline :-) > > >>> >>> And in a more general sense, this kind of permissiveness is not future >>> proof. What happens if someone needs to add another EXPR node that is >>> valid on the RHS where such recursion is undesirable? How are they >>> supposed to know that we've got this permissive recursive call and that >>> it's going to do the wrong thing? And if it's an EXPR node with no >>> arguments, then we're going to do a read out of the bounds of the object >>> and all bets are off at that point (yes we have zero operand EXPR nodes, >>> but thankfully I don't think they should up in the contexts you care about). >>> >> >> Sure. When things are hardcoded this way the same argument can >> be made both ways. If we just handle A and B and then someone >> adds an X that matches one of the two, it won't be handled. Either >> way, some work is necessary to deal with the new Z. One way to >> avoid it would be by providing an API to query this property of >> a code (e.g., a function like int gimple_result_aliases_operand >> (gimple *stmt, int opno)). > Adding a new opcode could have caused the original code to generate > incorrect code. In the new version an indirect opcode would just result > in something we refuse to analyze -- so we could miss a warning, but > more importantly we would _not_ transform the return statement so we'd > be safe WRT correct code generation. > > WRT making an API to do these kinds of queries. Sure. It's a fairly > natural extension to stuff we've done in the past. You could (for > example) wrap up all kinds existing properties of nodes into an API. > You'll see that's already done in a fairly ad-hoc fashion, but it's not > even close to being pervasive or standard practice. > > I would certainly applaud bringing some sanity to this kind of issue and > iterating towards a better place. But it's ugly, painful work with > little visible end user benefit. > >> >>> But those seem like distinct issues. The one I pointed out looks like a >>> pretty simple case to handle. It's just a logic error. >> >> It wasn't a logic error. I simply didn't think it was necessary >> to worry about the accuracy of an inherently inaccurate warning, >> and I didn't want make more intrusive changes than was called for >> by my simple fix/enhancement. I was also keeping in mind your >> preference for smaller change sets. But since you insist I went >> ahead and improved things. The warning is all the better for it, >> and the changes I made exposed a number of other bugs in GCC. >> At the same time, it seems that the bar for a simple bug fixes >> and small enhancements should not be in excess of what's possible >> by the original design. > It looked like a logic error to me that could be easily fixed. I didn't > want to make it out to a huge deal. ANd yes, there's a natural tension > between addressing small stuff as you see them in a larger kit and > breaking things down and iterating. There aren't hard and fast rules here. > > > >> >> >> gcc-71924.diff >> >> PR middle-end/71924 - missing -Wreturn-local-addr returning alloca result >> PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an address of a local array plus offset >> >> gcc/ChangeLog: >> >> PR middle-end/71924 >> PR middle-end/90549 >> * gimple-ssa-isolate-paths.c (isolate_path): Add attribute. >> (args_loc_t): New type. >> (args_loc_t, locmap_t): same. >> (diag_returned_locals): New function. >> (is_addr_local): Same. >> (handle_return_addr_local_phi_arg, warn_return_addr_local): Same. >> (find_implicit_erroneous_behavior): Call warn_return_addr_local_phi_arg. >> (find_explicit_erroneous_behavior): Call warn_return_addr_local. >> >> libgcc/ChangeLog: >> * generic-morestack.c: Disable -fisolate-erroneous-paths-dereference >> to get around PR libgcc/90918. >> >> gcc/testsuite/ChangeLog: >> >> PR middle-end/71924 >> PR middle-end/90549 >> * gcc.dg/Wreturn-local-addr-2.c: New test. >> * gcc.dg/Wreturn-local-addr-4.c: New test. >> * gcc.dg/Wreturn-local-addr-5.c: New test. >> * gcc.dg/Wreturn-local-addr-6.c: New test. >> * gcc.dg/Wreturn-local-addr-7.c: New test. >> * gcc.dg/Wreturn-local-addr-8.c: New test. >> * gcc.dg/Walloca-4.c: Prune expected warnings. >> * gcc.dg/pr41551.c: Same. >> * gcc.dg/pr59523.c: Same. >> * gcc.dg/tree-ssa/pr88775-2.c: Same. >> * gcc.dg/winline-7.c: Same. >> >> diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c >> index 33fe352bb23..13b1f5f2349 100644 >> --- a/gcc/gimple-ssa-isolate-paths.c >> +++ b/gcc/gimple-ssa-isolate-paths.c >> @@ -130,7 +130,7 @@ insert_trap (gimple_stmt_iterator *si_p, tree op) >> >> Return BB'. */ >> >> -basic_block >> +ATTRIBUTE_RETURNS_NONNULL basic_block >> isolate_path (basic_block bb, basic_block duplicate, >> edge e, gimple *stmt, tree op, bool ret_zero) >> { >> @@ -341,6 +341,283 @@ stmt_uses_0_or_null_in_undefined_way (gimple *stmt) >> return false; >> } >> >> +/* Describes the property of a return statement that may return >> + the address of one or more local variables. */ >> +struct args_loc_t >> +{ >> + args_loc_t (): nargs (), locvec (), ptr (&ptr) >> + { >> + locvec.create (4); >> + } >> + >> + args_loc_t (const args_loc_t &rhs) >> + : nargs (rhs.nargs), locvec (rhs.locvec.copy ()), ptr (&ptr) { } >> + >> + args_loc_t& operator= (const args_loc_t &rhs) >> + { >> + nargs = rhs.nargs; >> + locvec.release (); >> + locvec = rhs.locvec.copy (); >> + return *this; >> + } >> + >> + ~args_loc_t () >> + { >> + locvec.release (); >> + gcc_assert (ptr == &ptr); >> + } >> + >> + /* For a PHI in a return statement its number of arguments. When less >> + than LOCVEC.LENGTH () implies that an address of local may but need >> + not be returned by the statement. Otherwise it implies it definitely >> + is returned. Used to issue "may be" vs "is" diagnostics. */ >> + unsigned nargs; >> + /* The locations of local variables/alloca calls returned by the return >> + statement. Avoid using auto_vec here since it's not safe to copy due >> + to pr90904. */ >> + vec <location_t> locvec; >> + void *ptr; >> +}; > Make this a class, please. We use "struct" for POD. I've made this change. > Is there a strong need for an overloaded operator? Our guidelines > generally discourage operator overloads. This one seems on the border > to me. Others may have different ideas where the line is. If we really > don't need the overloaded operator, then we can avoid the controversy > completely. The copy assignment operator is necessary for the type to be stored in a hash_map. Otherwise, the implicitly generated assignment will be used instead which is unsafe in vec and results in memory corription (I opened bug 90904 for this). After working around this bug by providing the assignment operator I then ran into the hash_map bug 90959 that also results in memory corruption. I spent a few fun hours tracking down the GCC miscompilations that they led to. The golden rule in C++ is that every type should either be safe to copy and assign with the expected semantics or made not assignable and not copyable by declaring the copy ctor and copy assignment operator private (or deleted in more modern C++). It would be very helpful to detect this kind of problem (classes that aren't safely assignable and copyable because they acquire/release resources in ctors/dtors). Not just to us in GCC but I'm sure to others as well. It's something I've been hoping to implement for a while now. >> + >> +/* Return true if EXPR is a expression of pointer type that refers >> + to the address of a variable with automatic storage duration. >> + If so, add an entry to *PLOCMAP and insert INTO PLOCMAP->LOCVEC >> + the locations of the corresponding local variables whose address >> + is returned by the statement. VISITED is a bitmap of PHI nodes >> + already visited by recursive calls. */ >> + >> +static bool >> +is_addr_local (gimple *return_stmt, tree exp, locmap_t *plocmap, >> + hash_set<gphi *> *visited) >> +{ >> + if (TREE_CODE (exp) == ADDR_EXPR) >> + { >> + tree baseaddr = get_base_address (TREE_OPERAND (exp, 0)); >> + if (TREE_CODE (baseaddr) == MEM_REF) >> + return is_addr_local (return_stmt, TREE_OPERAND (baseaddr, 0), >> + plocmap, visited); >> + >> + if ((!VAR_P (baseaddr) >> + || is_global_var (baseaddr)) >> + && TREE_CODE (baseaddr) != PARM_DECL) >> + return false; >> + >> + args_loc_t &argsloc = plocmap->get_or_insert (return_stmt); >> + argsloc.locvec.safe_push (DECL_SOURCE_LOCATION (baseaddr)); >> + return true; >> + } >> + >> + if (!POINTER_TYPE_P (TREE_TYPE (exp))) >> + return false; >> + >> + if (TREE_CODE (exp) == SSA_NAME) >> + { >> + gimple *def_stmt = SSA_NAME_DEF_STMT (exp); >> + enum gimple_code code = gimple_code (def_stmt); >> + >> + if (is_gimple_assign (def_stmt)) >> + { >> + tree type = TREE_TYPE (gimple_assign_lhs (def_stmt)); >> + if (POINTER_TYPE_P (type)) >> + { >> + tree_code code = gimple_assign_rhs_code (def_stmt); >> + tree ptr1 = NULL_TREE, ptr2 = NULL_TREE; >> + if (code == COND_EXPR) >> + { >> + ptr1 = gimple_assign_rhs2 (def_stmt); >> + ptr2 = gimple_assign_rhs3 (def_stmt); >> + } >> + else if (code == MAX_EXPR || code == MIN_EXPR) >> + { >> + ptr1 = gimple_assign_rhs1 (def_stmt); >> + ptr2 = gimple_assign_rhs2 (def_stmt); >> + } >> + else if (code == ADDR_EXPR >> + || code == NOP_EXPR >> + || code == POINTER_PLUS_EXPR) >> + ptr1 = gimple_assign_rhs1 (def_stmt); >> + >> + /* Avoid short-circuiting the logical OR result in case >> + both oerands refer to local variables, in which case >> + both should be identified in the warning. */ >> + bool res1 = false, res2 = false; >> + if (ptr1) >> + res1 = is_addr_local (return_stmt, ptr1, plocmap, visited); >> + if (ptr2) >> + res2 = is_addr_local (return_stmt, ptr2, plocmap, visited); >> + return res1 || res2; >> + } >> + return false; >> + } > s/oerands/operands/ a few lines up. > > So how do you deal with the case where one operand of a > {MIN,MAX,COND}_EXPR is the address of a local, but the other is not? It > seems like we'll end up returning true from this function in that case > and potentially trigger changing the return value to NULL. Or am I > missing something? I only briefly considered MIN/MAX but because in C and C++ the less than and greater than expressions are only defined for pointers into the same array/object or subobject and I didn't ponder it too hard. (In C they're undefined otherwise, in C++ the result is unspecified.) But you bring it up because you think the address should be returned regardless, even if the expression is invalid (or if it's COND_EXPR with mixed local/nonlocal addresses) so I've changed it to do that for all these explicitly handled codes and added tests to verify it. Retested on x86_64-linux. Martin
On 6/30/19 3:50 PM, Martin Sebor wrote: > On 6/26/19 6:11 PM, Jeff Law wrote: [ Another big snip ] > >> Is there a strong need for an overloaded operator? Our guidelines >> generally discourage operator overloads. This one seems on the border >> to me. Others may have different ideas where the line is. If we really >> don't need the overloaded operator, then we can avoid the controversy >> completely. > > The copy assignment operator is necessary for the type to be stored > in a hash_map. Otherwise, the implicitly generated assignment will > be used instead which is unsafe in vec and results in memory > corription (I opened bug 90904 for this). After working around this > bug by providing the assignment operator I then ran into the hash_map > bug 90959 that also results in memory corruption. I spent a few fun > hours tracking down the GCC miscompilations that they led to. OK. THanks for explaining why we need the copy assignment operator. > > The golden rule in C++ is that every type should either be safe to > copy and assign with the expected semantics or made not assignable > and not copyable by declaring the copy ctor and copy assignment > operator private (or deleted in more modern C++). It would be very > helpful to detect this kind of problem (classes that aren't safely > assignable and copyable because they acquire/release resources in > ctors/dtors). Not just to us in GCC but I'm sure to others as well. > It's something I've been hoping to implement for a while now. You've got my blessing to do that :-) >> >> So how do you deal with the case where one operand of a >> {MIN,MAX,COND}_EXPR is the address of a local, but the other is not? It >> seems like we'll end up returning true from this function in that case >> and potentially trigger changing the return value to NULL. Or am I >> missing something? > > I only briefly considered MIN/MAX but because in C and C++ the less > than and greater than expressions are only defined for pointers into > the same array/object or subobject and I didn't ponder it too hard. > (In C they're undefined otherwise, in C++ the result is unspecified.) > > But you bring it up because you think the address should be returned > regardless, even if the expression is invalid (or if it's COND_EXPR > with mixed local/nonlocal addresses) so I've changed it to do that > for all these explicitly handled codes and added tests to verify it. THanks. So in the thread for strlen/sprintf the issue around unbounded walks up through PHI argument's SSA_NAME_DEF_STMT was raised. I think we've got two options here. One would be to hold this patch until we sort out what the bounds should look like, then adjust this patch to do something similar. Or go forward with this patch, then come back and adjust the walker once we have settled on solution in the other thread. I'm OK with either. Your choice. jeff
On 7/2/19 2:59 PM, Jeff Law wrote: > On 6/30/19 3:50 PM, Martin Sebor wrote: >> On 6/26/19 6:11 PM, Jeff Law wrote: > [ Another big snip ] >> >>> Is there a strong need for an overloaded operator? Our guidelines >>> generally discourage operator overloads. This one seems on the border >>> to me. Others may have different ideas where the line is. If we really >>> don't need the overloaded operator, then we can avoid the controversy >>> completely. >> >> The copy assignment operator is necessary for the type to be stored >> in a hash_map. Otherwise, the implicitly generated assignment will >> be used instead which is unsafe in vec and results in memory >> corription (I opened bug 90904 for this). After working around this >> bug by providing the assignment operator I then ran into the hash_map >> bug 90959 that also results in memory corruption. I spent a few fun >> hours tracking down the GCC miscompilations that they led to. > OK. THanks for explaining why we need the copy assignment operator. > > >> >> The golden rule in C++ is that every type should either be safe to >> copy and assign with the expected semantics or made not assignable >> and not copyable by declaring the copy ctor and copy assignment >> operator private (or deleted in more modern C++). It would be very >> helpful to detect this kind of problem (classes that aren't safely >> assignable and copyable because they acquire/release resources in >> ctors/dtors). Not just to us in GCC but I'm sure to others as well. >> It's something I've been hoping to implement for a while now. > You've got my blessing to do that :-) > >>> >>> So how do you deal with the case where one operand of a >>> {MIN,MAX,COND}_EXPR is the address of a local, but the other is not? It >>> seems like we'll end up returning true from this function in that case >>> and potentially trigger changing the return value to NULL. Or am I >>> missing something? >> >> I only briefly considered MIN/MAX but because in C and C++ the less >> than and greater than expressions are only defined for pointers into >> the same array/object or subobject and I didn't ponder it too hard. >> (In C they're undefined otherwise, in C++ the result is unspecified.) >> >> But you bring it up because you think the address should be returned >> regardless, even if the expression is invalid (or if it's COND_EXPR >> with mixed local/nonlocal addresses) so I've changed it to do that >> for all these explicitly handled codes and added tests to verify it. > THanks. > > So in the thread for strlen/sprintf the issue around unbounded walks up > through PHI argument's SSA_NAME_DEF_STMT was raised. > > I think we've got two options here. > > One would be to hold this patch until we sort out what the bounds should > look like, then adjust this patch to do something similar. > > Or go forward with this patch, then come back and adjust the walker once > we have settled on solution in the other thread. > > I'm OK with either. Your choice. I committed the patch as is until the question of which algorithms to put the limit on is answered (as discussed in the thread Re: sprintf/strlen integration). Martin
PR middle-end/71924 - missing -Wreturn-local-addr returning alloca result PR middle-end/90549 - missing -Wreturn-local-addr maybe returning an address of a local array plus offset gcc/ChangeLog: PR c/71924 * gimple-ssa-isolate-paths.c (is_addr_local): New function. (warn_return_addr_local_phi_arg, warn_return_addr_local): Same. (find_implicit_erroneous_behavior): Call warn_return_addr_local_phi_arg. (find_explicit_erroneous_behavior): Call warn_return_addr_local. gcc/testsuite/ChangeLog: PR c/71924 * gcc.dg/Wreturn-local-addr-2.c: New test. * gcc.dg/Walloca-4.c: Prune expected warnings. * gcc.dg/pr41551.c: Same. * gcc.dg/pr59523.c: Same. * gcc.dg/tree-ssa/pr88775-2.c: Same. * gcc.dg/winline-7.c: Same. diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c index 33fe352bb23..2933ecf502e 100644 --- a/gcc/gimple-ssa-isolate-paths.c +++ b/gcc/gimple-ssa-isolate-paths.c @@ -341,6 +341,135 @@ stmt_uses_0_or_null_in_undefined_way (gimple *stmt) return false; } +/* Return true if EXPR is a expression of pointer type that refers + to the address of a variable with automatic storage duration. + If so, set *PLOC to the location of the object or the call that + allocated it (for alloca and VLAs). When PMAYBE is non-null, + also consider PHI statements and set *PMAYBE when some but not + all arguments of such statements refer to local variables, and + to clear it otherwise. */ + +static bool +is_addr_local (tree exp, location_t *ploc, bool *pmaybe = NULL, + hash_set<gphi *> *visited = NULL) +{ + if (TREE_CODE (exp) == ADDR_EXPR) + { + tree baseaddr = get_base_address (TREE_OPERAND (exp, 0)); + if (TREE_CODE (baseaddr) == MEM_REF) + return is_addr_local (TREE_OPERAND (baseaddr, 0), ploc, pmaybe, visited); + + if ((!VAR_P (baseaddr) + || is_global_var (baseaddr)) + && TREE_CODE (baseaddr) != PARM_DECL) + return false; + + *ploc = DECL_SOURCE_LOCATION (baseaddr); + return true; + } + + if (TREE_CODE (exp) == SSA_NAME) + { + gimple *def_stmt = SSA_NAME_DEF_STMT (exp); + enum gimple_code code = gimple_code (def_stmt); + + if (is_gimple_assign (def_stmt)) + { + tree type = TREE_TYPE (gimple_assign_lhs (def_stmt)); + if (POINTER_TYPE_P (type)) + { + tree ptr = gimple_assign_rhs1 (def_stmt); + return is_addr_local (ptr, ploc, pmaybe, visited); + } + return false; + } + + if (code == GIMPLE_CALL + && gimple_call_builtin_p (def_stmt)) + { + tree fn = gimple_call_fndecl (def_stmt); + int code = DECL_FUNCTION_CODE (fn); + if (code != BUILT_IN_ALLOCA + && code != BUILT_IN_ALLOCA_WITH_ALIGN) + return false; + + *ploc = gimple_location (def_stmt); + return true; + } + + if (code == GIMPLE_PHI && pmaybe) + { + unsigned count = 0; + gphi *phi_stmt = as_a <gphi *> (def_stmt); + + unsigned nargs = gimple_phi_num_args (phi_stmt); + for (unsigned i = 0; i < nargs; ++i) + { + if (!visited->add (phi_stmt)) + { + tree arg = gimple_phi_arg_def (phi_stmt, i); + if (is_addr_local (arg, ploc, pmaybe, visited)) + ++count; + } + } + + *pmaybe = count && count < nargs; + return count != 0; + } + } + + return false; +} + +/* Detect and diagnose returning the address of a local variable in + a PHI result LHS and argument OP and PHI edge E in basic block BB. */ + +static basic_block +warn_return_addr_local_phi_arg (basic_block bb, basic_block duplicate, + tree lhs, tree op, edge e) +{ + location_t origin; + if (!is_addr_local (op, &origin)) + return NULL; + + gimple *use_stmt; + imm_use_iterator iter; + + FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs) + { + greturn *return_stmt = dyn_cast <greturn *> (use_stmt); + if (!return_stmt) + continue; + + if (gimple_return_retval (return_stmt) != lhs) + continue; + + { + auto_diagnostic_group d; + if (warning_at (gimple_location (use_stmt), + OPT_Wreturn_local_addr, + "function may return address " + "of local variable") + && origin != UNKNOWN_LOCATION) + inform (origin, "declared here"); + } + + if ((flag_isolate_erroneous_paths_dereference + || flag_isolate_erroneous_paths_attribute) + && gimple_bb (use_stmt) == bb) + { + duplicate = isolate_path (bb, duplicate, e, + use_stmt, lhs, true); + + /* When we remove an incoming edge, we need to + reprocess the Ith element. */ + cfg_altered = true; + } + } + + return duplicate; +} + /* Look for PHI nodes which feed statements in the same block where the value of the PHI node implies the statement is erroneous. @@ -400,58 +529,19 @@ find_implicit_erroneous_behavior (void) { tree op = gimple_phi_arg_def (phi, i); edge e = gimple_phi_arg_edge (phi, i); - imm_use_iterator iter; - gimple *use_stmt; - next_i = i + 1; - - if (TREE_CODE (op) == ADDR_EXPR) - { - tree valbase = get_base_address (TREE_OPERAND (op, 0)); - if ((VAR_P (valbase) && !is_global_var (valbase)) - || TREE_CODE (valbase) == PARM_DECL) - { - FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs) - { - greturn *return_stmt - = dyn_cast <greturn *> (use_stmt); - if (!return_stmt) - continue; - - if (gimple_return_retval (return_stmt) != lhs) - continue; - - { - auto_diagnostic_group d; - if (warning_at (gimple_location (use_stmt), - OPT_Wreturn_local_addr, - "function may return address " - "of local variable")) - inform (DECL_SOURCE_LOCATION(valbase), - "declared here"); - } - - if ((flag_isolate_erroneous_paths_dereference - || flag_isolate_erroneous_paths_attribute) - && gimple_bb (use_stmt) == bb) - { - duplicate = isolate_path (bb, duplicate, e, - use_stmt, lhs, true); - - /* When we remove an incoming edge, we need to - reprocess the Ith element. */ - next_i = i; - cfg_altered = true; - } - } - } - } + duplicate = warn_return_addr_local_phi_arg (bb, duplicate, + lhs, op, e); + next_i = duplicate ? i : i + 1; if (!integer_zerop (op)) continue; location_t phi_arg_loc = gimple_phi_arg_location (phi, i); + imm_use_iterator iter; + gimple *use_stmt; + /* We've got a NULL PHI argument. Now see if the PHI's result is dereferenced within BB. */ FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs) @@ -482,6 +572,51 @@ find_implicit_erroneous_behavior (void) } } +/* Detect and diagnose returning the address of a local variable + in RETURN_STMT in basic block BB. This only becomes undefined + behavior if the result is used, so we do not insert a trap and + only return NULL instead. */ + +static void +warn_return_addr_local (basic_block bb, greturn *return_stmt) +{ + tree val = gimple_return_retval (return_stmt); + if (!val) + return; + + bool maybe = false; + location_t origin; + hash_set<gphi *> visited_phis; + if (!is_addr_local (val, &origin, &maybe, &visited_phis)) + return; + + /* We only need it for this particular case. */ + calculate_dominance_info (CDI_POST_DOMINATORS); + const char* msg = N_("function returns address of local variable"); + if (maybe + || !dominated_by_p (CDI_POST_DOMINATORS, + single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)), bb)) + msg = N_("function may return address of local variable"); + + { + auto_diagnostic_group d; + if (warning_at (gimple_location (return_stmt), + OPT_Wreturn_local_addr, msg) + && origin != UNKNOWN_LOCATION) + inform (origin, "declared here"); + } + + /* Do not modify code if the user only asked for + warnings. */ + if (flag_isolate_erroneous_paths_dereference + || flag_isolate_erroneous_paths_attribute) + { + tree zero = build_zero_cst (TREE_TYPE (val)); + gimple_return_set_retval (return_stmt, zero); + update_stmt (return_stmt); + } +} + /* Look for statements which exhibit erroneous behavior. For example a NULL pointer dereference. @@ -525,49 +660,10 @@ find_explicit_erroneous_behavior (void) break; } - /* Detect returning the address of a local variable. This only - becomes undefined behavior if the result is used, so we do not - insert a trap and only return NULL instead. */ + /* Look for a return statement that returns the address + of a local variable or the result of alloca. */ if (greturn *return_stmt = dyn_cast <greturn *> (stmt)) - { - tree val = gimple_return_retval (return_stmt); - if (val && TREE_CODE (val) == ADDR_EXPR) - { - tree valbase = get_base_address (TREE_OPERAND (val, 0)); - if ((VAR_P (valbase) && !is_global_var (valbase)) - || TREE_CODE (valbase) == PARM_DECL) - { - /* We only need it for this particular case. */ - calculate_dominance_info (CDI_POST_DOMINATORS); - const char* msg; - bool always_executed = dominated_by_p - (CDI_POST_DOMINATORS, - single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)), bb); - if (always_executed) - msg = N_("function returns address of local variable"); - else - msg = N_("function may return address of " - "local variable"); - { - auto_diagnostic_group d; - if (warning_at (gimple_location (stmt), - OPT_Wreturn_local_addr, msg)) - inform (DECL_SOURCE_LOCATION(valbase), - "declared here"); - } - - /* Do not modify code if the user only asked for - warnings. */ - if (flag_isolate_erroneous_paths_dereference - || flag_isolate_erroneous_paths_attribute) - { - tree zero = build_zero_cst (TREE_TYPE (val)); - gimple_return_set_retval (return_stmt, zero); - update_stmt (stmt); - } - } - } - } + warn_return_addr_local (bb, return_stmt); } } } diff --git a/gcc/testsuite/gcc.dg/Walloca-4.c b/gcc/testsuite/gcc.dg/Walloca-4.c index 85dcb7b9bb9..f888e2db2ed 100644 --- a/gcc/testsuite/gcc.dg/Walloca-4.c +++ b/gcc/testsuite/gcc.dg/Walloca-4.c @@ -7,7 +7,7 @@ { char *src; - _Bool + _Bool use_alloca = (((rear_ptr - w) * sizeof (char)) < 4096U); if (use_alloca) src = (char *) __builtin_alloca ((rear_ptr - w) * sizeof (char)); @@ -15,3 +15,5 @@ src = (char *) __builtin_malloc ((rear_ptr - w) * sizeof (char)); return src; } + +/* { dg-prune-output "-Wreturn-local-addr" } */ diff --git a/gcc/testsuite/gcc.dg/Wreturn-local-addr-2.c b/gcc/testsuite/gcc.dg/Wreturn-local-addr-2.c new file mode 100644 index 00000000000..0e3435c8256 --- /dev/null +++ b/gcc/testsuite/gcc.dg/Wreturn-local-addr-2.c @@ -0,0 +1,293 @@ +/* PR c/71924 - missing -Wreturn-local-addr returning alloca result + { dg-do compile } + { dg-options "-O2 -Wall" } */ + +#define ATTR(...) __attribute__ ((__VA_ARGS__)) + +struct A { int a, b, c; }; +struct B { int a, b, c[]; }; + +void sink (void*, ...); + +ATTR (noipa) void* +return_alloca (int n) +{ + void *p = __builtin_alloca (n); + sink (p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_alloca_index_cst (int n) +{ + int *p = (int*)__builtin_alloca (n); + p = &p[1]; + sink (p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_alloca_plus_cst (int n) +{ + int *p = (int*)__builtin_alloca (n); + p += 1; + sink (p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_alloca_plus_var (int n, int i) +{ + char *p = (char*)__builtin_alloca (n); + p += i; + sink (p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_alloca_member_1 (int n) +{ + struct A *p = (struct A*)__builtin_alloca (n); + sink (&p->a); + return &p->a; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_alloca_member_2 (int n) +{ + struct A *p = (struct A*)__builtin_alloca (n); + sink (&p->b); + return &p->b; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_alloca_flexarray (int n) +{ + struct B *p = (struct B*)__builtin_alloca (n); + sink (p->c); + return p->c; /* { dg-warning "function returns address of local" } */ +} + + +ATTR (noipa) void* +return_array (void) +{ + int a[32]; + void *p = a; + sink (p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_array_index_cst (void) +{ + int a[32]; + void *p = &a[2]; + sink (p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_array_plus_cst (void) +{ + int a[32]; + void *p = a + 2; + sink (p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_array_plus_var (int i) +{ + int a[32]; + void *p = a + i; + sink (p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_array_member_1 (void) +{ + struct A a[2]; + int *p = &a[1].a; + sink (a, p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_array_member_2 (void) +{ + struct A a[32]; + int *p = &a[1].b; + sink (a, p); + return p; /* { dg-warning "function returns address of local" } */ +} + + +ATTR (noipa) void* +return_vla (int n) +{ + char a[n]; + void *p = a; + sink (p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_vla_index_cst (int n) +{ + char a[n]; + char *p = &a[3]; + sink (p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_vla_plus_cst (int n) +{ + char a[n]; + char *p = a + 3; + sink (p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_vla_index_var (int n, int i) +{ + char a[n]; + char *p = &a[i]; + sink (p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_vla_plus_var (int n, int i) +{ + char a[n]; + char *p = a + i; + sink (p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_vla_member_1 (int n, int i) +{ + struct A a[n]; + void *p = &a[i].a; + sink (a, p); + return p; /* { dg-warning "function returns address of local" } */ +} + +ATTR (noipa) void* +return_vla_member_2 (int n, int i) +{ + struct A a[n]; + void *p = &a[i].b; + sink (a, p); + return p; /* { dg-warning "function returns address of local" } */ +} + + +ATTR (noipa) void* +return_alloca_or_alloca (int n, int i) +{ + void *p = i ? __builtin_alloca (n * i) : __builtin_alloca (n); + sink (p); + /* The warning here should really be "function returns". */ + return p; /* { dg-warning "function (returns|may return) address of local" } */ +} + +ATTR (noipa) void* +return_alloca_or_alloca_2 (int n, int i) +{ + void *p0 = __builtin_alloca (n); + void *p1 = __builtin_alloca (n * 2); + void *p = i ? p0 : p1; + sink (p0, p1, p); + /* Same as above. */ + return p; /* { dg-warning "function (returns|may return) address of local" } */ +} + +ATTR (noipa) void* +return_array_or_array (int i) +{ + int a[5]; + int b[7]; + void *p = i ? a : b; + sink (a, b, p); + /* The warning here should really be "function returns". */ + return p; /* { dg-warning "function (returns|may return) address of local" } */ +} + +ATTR (noipa) void* +return_array_or_array_plus_var (int i, int j) +{ + int a[5]; + int b[7]; + + void *p0 = a + i; + void *p1 = b + j; + + void *p = i < j ? p0 : p1; + sink (a, b, p0, p1, p); + /* The warning here should really be "function returns". */ + return p; /* { dg-warning "function (returns|may return) address of local" } */ +} + +extern int global[32]; + +ATTR (noipa) void* +may_return_global_or_alloca (int n, int i) +{ + void *p = i ? global : __builtin_alloca (n); + sink (p); + return p; /* { dg-warning "function may return address of local" } */ +} + + +ATTR (noipa) void* +may_return_global_or_alloca_plus_cst (int n, int i) +{ + int *p = i ? global : (int*)__builtin_alloca (n); + p += 7; + sink (p); + return p; /* { dg-warning "function may return address of local" } */ +} + +ATTR (noipa) void* +may_return_global_or_array (int n, int i) +{ + int a[32]; + void *p = i ? global : a; + sink (p); + return p; /* { dg-warning "function may return address of local" } */ +} + +ATTR (noipa) void* +may_return_global_or_array_plus_cst (int n, int i) +{ + int a[32]; + int *p = i ? global : a; + p += 4; + sink (p); + return p; /* { dg-warning "function may return address of local" } */ +} + +ATTR (noipa) void* +may_return_global_or_vla (int n, int i) +{ + int a[n]; + void *p = i ? global : a; + sink (p); + return p; /* { dg-warning "function may return address of local" } */ +} + +ATTR (noipa) void* +may_return_global_or_vla_plus_cst (int n, int i) +{ + int a[n]; + int *p = i ? global : a; + p += 4; + sink (p); + return p; /* { dg-warning "function may return address of local" } */ +} diff --git a/gcc/testsuite/gcc.dg/pr41551.c b/gcc/testsuite/gcc.dg/pr41551.c index 2f2ad2be97e..e1123206cc6 100644 --- a/gcc/testsuite/gcc.dg/pr41551.c +++ b/gcc/testsuite/gcc.dg/pr41551.c @@ -10,3 +10,5 @@ int main(void) int var, *p = &var; return (double)(uintptr_t)(p); } + +/* { dg-prune-output "-Wreturn-local-addr" } */ diff --git a/gcc/testsuite/gcc.dg/pr59523.c b/gcc/testsuite/gcc.dg/pr59523.c index a6c3302a683..49cbe5dd27a 100644 --- a/gcc/testsuite/gcc.dg/pr59523.c +++ b/gcc/testsuite/gcc.dg/pr59523.c @@ -16,3 +16,5 @@ foo (int a, int *b, int *c, int *d) r[i] = 1; return r; } + +/* { dg-prune-output "-Wreturn-local-addr" } */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr88775-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr88775-2.c index 292ce6edefc..ed5df826432 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/pr88775-2.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr88775-2.c @@ -41,3 +41,5 @@ f5 (void) int c[64] = {}, d[64] = {}; return (__UINTPTR_TYPE__) &c[64] != (__UINTPTR_TYPE__) &d[0]; } + +/* { dg-prune-output "-Wreturn-local-addr" } */ diff --git a/gcc/testsuite/gcc.dg/winline-7.c b/gcc/testsuite/gcc.dg/winline-7.c index 34deca42592..239d748926d 100644 --- a/gcc/testsuite/gcc.dg/winline-7.c +++ b/gcc/testsuite/gcc.dg/winline-7.c @@ -13,3 +13,5 @@ inline void *t (void) { return q (); /* { dg-message "called from here" } */ } + +/* { dg-prune-output "-Wreturn-local-addr" } */