| Message ID | CAAgBjMnap+eF165TdbZrpAmU2=stko03z0OOzsHwd_Bca+MN9g@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PR83648 | expand |
On January 3, 2018 7:03:26 AM GMT+01:00, Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> wrote: >Hi, >malloc_candidate_p() in ipa-pure-const misses detecting that a >function is malloc-like if the return value is result of PHI and one >of the arguments of PHI is 0. >For example: > >void g(unsigned n) >{ > return (n) ? __builtin_malloc (n) : 0; >} > >The reason is that the following check: >if (TREE_CODE (arg) != SSA_NAME) > DUMP_AND_RETURN ("phi arg is not SSA_NAME.") > >fails for arg with constant value 0 and malloc_candidate_p returns >false. >The patch simply skips the arg if it equals null_pointer_node. >Does it look OK ? Please use integer_zerop instead of a literal compare. I'm not sure how to handle targets where address zero is valid (flag_no_delete_null_pointer_chekcs) >One concern I have is that with the patch, malloc_candidate_p will >return true if all the args to PHI are NULL: >retval = PHI<0, 0> >return retval > >However I expect that PHI with all 0 args would be constant folded to >0 earlier, so this case shouldn't occur in practice ? You may not rely on folding for correctness. >Bootstrapped+tested on x86_64-unknown-linux-gnu. >Cross-testing on arm*-*-* and aarch64*-*-* in progress. > >Thanks, >Prathamesh
On Wed, Jan 03, 2018 at 10:05:30AM +0100, Richard Biener wrote: > >One concern I have is that with the patch, malloc_candidate_p will > >return true if all the args to PHI are NULL: > >retval = PHI<0, 0> > >return retval > > > >However I expect that PHI with all 0 args would be constant folded to > >0 earlier, so this case shouldn't occur in practice ? > > You may not rely on folding for correctness. Yeah. Will the patch handle (I mean punt on) also unfolded if (n) ? 0 : __builtin_malloc (n); and similar? Jakub
> On Wed, Jan 03, 2018 at 10:05:30AM +0100, Richard Biener wrote: > > >One concern I have is that with the patch, malloc_candidate_p will > > >return true if all the args to PHI are NULL: > > >retval = PHI<0, 0> > > >return retval > > > > > >However I expect that PHI with all 0 args would be constant folded to > > >0 earlier, so this case shouldn't occur in practice ? > > > > You may not rely on folding for correctness. .. and at this level i would say even for code quality. Early optimizers are facing a lot of garbage and they are not repeated, so we get code at various intermediate levels of optimizations thorugh the IPA queue. Honza > > Yeah. Will the patch handle (I mean punt on) also unfolded > if (n) ? 0 : __builtin_malloc (n); > and similar? > > Jakub
> diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c > index 09ca3590039..0406d5588d2 100644 > --- a/gcc/ipa-pure-const.c > +++ b/gcc/ipa-pure-const.c > @@ -910,7 +910,8 @@ malloc_candidate_p (function *fun, bool ipa) > #define DUMP_AND_RETURN(reason) \ > { \ > if (dump_file && (dump_flags & TDF_DETAILS)) \ > - fprintf (dump_file, "%s", (reason)); \ > + fprintf (dump_file, "\n%s is not a malloc candidate, reason: %s\n", \ > + (node->name()), (reason)); \ > return false; \ > } > > @@ -961,11 +962,14 @@ malloc_candidate_p (function *fun, bool ipa) > for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i) > { > tree arg = gimple_phi_arg_def (phi, i); > + if (arg == null_pointer_node) > + continue; I think you want operand_equal_p here and also check for flag_delete_null_pointer_checks because otherwise 0 can be legal memory block addrss. > + > if (TREE_CODE (arg) != SSA_NAME) > - DUMP_AND_RETURN("phi arg is not SSA_NAME.") > - if (!(arg == null_pointer_node || check_retval_uses (arg, phi))) > - DUMP_AND_RETURN("phi arg has uses outside phi" > - " and comparisons against 0.") > + DUMP_AND_RETURN ("phi arg is not SSA_NAME."); > + if (!check_retval_uses (arg, phi)) > + DUMP_AND_RETURN ("phi arg has uses outside phi" > + " and comparisons against 0.") So the case of phi(0,0) is not really correctness issue just missed optimization? I would still add code to handle it (it is easy). Do you also handle a case where parameter of phi is another phi? Honza > > gimple *arg_def = SSA_NAME_DEF_STMT (arg); > gcall *call_stmt = dyn_cast<gcall *> (arg_def); > diff --git a/gcc/testsuite/gcc.dg/ipa/pr83648.c b/gcc/testsuite/gcc.dg/ipa/pr83648.c > new file mode 100644 > index 00000000000..03b45de671b > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/ipa/pr83648.c > @@ -0,0 +1,9 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-tree-local-pure-const-details" } */ > + > +void *g(unsigned n) > +{ > + return n ? __builtin_malloc (n) : 0; > +} > + > +/* { dg-final { scan-tree-dump "Function found to be malloc: g" "local-pure-const1" } } */
On 01/02/2018 11:03 PM, Prathamesh Kulkarni wrote: > Hi, > malloc_candidate_p() in ipa-pure-const misses detecting that a > function is malloc-like if the return value is result of PHI and one > of the arguments of PHI is 0. > For example: > > void g(unsigned n) > { > return (n) ? __builtin_malloc (n) : 0; > } > > The reason is that the following check: > if (TREE_CODE (arg) != SSA_NAME) > DUMP_AND_RETURN ("phi arg is not SSA_NAME.") > > fails for arg with constant value 0 and malloc_candidate_p returns false. > The patch simply skips the arg if it equals null_pointer_node. > Does it look OK ? > > One concern I have is that with the patch, malloc_candidate_p will > return true if all the args to PHI are NULL: > retval = PHI<0, 0> > return retval > > However I expect that PHI with all 0 args would be constant folded to > 0 earlier, so this case shouldn't occur in practice ? > > Bootstrapped+tested on x86_64-unknown-linux-gnu. > Cross-testing on arm*-*-* and aarch64*-*-* in progress. A degenerate PHI should be folded/propagated, but you can't absolutely depend on it -- ie, you must handle it gracefully. I think the way to do that here is verify that at least one argument is an SSA_NAME. jeff
On 3 January 2018 at 18:58, Jan Hubicka <hubicka@ucw.cz> wrote: > >> diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c >> index 09ca3590039..0406d5588d2 100644 >> --- a/gcc/ipa-pure-const.c >> +++ b/gcc/ipa-pure-const.c >> @@ -910,7 +910,8 @@ malloc_candidate_p (function *fun, bool ipa) >> #define DUMP_AND_RETURN(reason) \ >> { \ >> if (dump_file && (dump_flags & TDF_DETAILS)) \ >> - fprintf (dump_file, "%s", (reason)); \ >> + fprintf (dump_file, "\n%s is not a malloc candidate, reason: %s\n", \ >> + (node->name()), (reason)); \ >> return false; \ >> } >> >> @@ -961,11 +962,14 @@ malloc_candidate_p (function *fun, bool ipa) >> for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i) >> { >> tree arg = gimple_phi_arg_def (phi, i); >> + if (arg == null_pointer_node) >> + continue; > > I think you want operand_equal_p here and also check for flag_delete_null_pointer_checks > because otherwise 0 can be legal memory block addrss. >> + >> if (TREE_CODE (arg) != SSA_NAME) >> - DUMP_AND_RETURN("phi arg is not SSA_NAME.") >> - if (!(arg == null_pointer_node || check_retval_uses (arg, phi))) >> - DUMP_AND_RETURN("phi arg has uses outside phi" >> - " and comparisons against 0.") >> + DUMP_AND_RETURN ("phi arg is not SSA_NAME."); >> + if (!check_retval_uses (arg, phi)) >> + DUMP_AND_RETURN ("phi arg has uses outside phi" >> + " and comparisons against 0.") > > So the case of phi(0,0) is not really correctness issue just missed optimization? > I would still add code to handle it (it is easy). Yes, I suppose it won't affect correctness, since it would be marked as TOP and after propagation it would be lowered to BOTTOM, but I added the check anyway. > > Do you also handle a case where parameter of phi is another phi? Indeed, it doesn't handle multiple-phi's as in the following test-case, good catch! void *g (int cond1, int cond2, int cond3) { void *ret; void *a; void *b; if (cond1) a = __builtin_malloc (10); else a = __builtin_malloc (20); if (cond2) b = __builtin_malloc (30); else b = __builtin_malloc (40); if (cond3) ret = a; else ret = b; return ret; } local-pure-const dump shows the function not being marked as malloc while it should be. I will address this issue in a follow up patch. This patch check for flag_delete_null_pointer_checks and wheteher all phi args are 0 in the attached patch. Validation in progress. Does it look OK ? As Jakub pointed out for the case: void *f() { return __builtin_malloc (0); } The malloc propagation would set f() to malloc. However AFAIU, malloc(0) returns NULL (?) and the function shouldn't be marked as malloc ? Thanks, Prathamesh > > Honza >> >> gimple *arg_def = SSA_NAME_DEF_STMT (arg); >> gcall *call_stmt = dyn_cast<gcall *> (arg_def); >> diff --git a/gcc/testsuite/gcc.dg/ipa/pr83648.c b/gcc/testsuite/gcc.dg/ipa/pr83648.c >> new file mode 100644 >> index 00000000000..03b45de671b >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/ipa/pr83648.c >> @@ -0,0 +1,9 @@ >> +/* { dg-do compile } */ >> +/* { dg-options "-O2 -fdump-tree-local-pure-const-details" } */ >> + >> +void *g(unsigned n) >> +{ >> + return n ? __builtin_malloc (n) : 0; >> +} >> + >> +/* { dg-final { scan-tree-dump "Function found to be malloc: g" "local-pure-const1" } } */ > diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c index 09ca3590039..dd15a499f6b 100644 --- a/gcc/ipa-pure-const.c +++ b/gcc/ipa-pure-const.c @@ -910,11 +910,13 @@ malloc_candidate_p (function *fun, bool ipa) #define DUMP_AND_RETURN(reason) \ { \ if (dump_file && (dump_flags & TDF_DETAILS)) \ - fprintf (dump_file, "%s", (reason)); \ + fprintf (dump_file, "\n%s is not a malloc candidate, reason: %s\n", \ + (node->name()), (reason)); \ return false; \ } - if (EDGE_COUNT (exit_block->preds) == 0) + if (EDGE_COUNT (exit_block->preds) == 0 + || !flag_delete_null_pointer_checks) return false; FOR_EACH_EDGE (e, ei, exit_block->preds) @@ -958,34 +960,45 @@ malloc_candidate_p (function *fun, bool ipa) } else if (gphi *phi = dyn_cast<gphi *> (def)) - for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i) - { - tree arg = gimple_phi_arg_def (phi, i); - if (TREE_CODE (arg) != SSA_NAME) - DUMP_AND_RETURN("phi arg is not SSA_NAME.") - if (!(arg == null_pointer_node || check_retval_uses (arg, phi))) - DUMP_AND_RETURN("phi arg has uses outside phi" - " and comparisons against 0.") - - gimple *arg_def = SSA_NAME_DEF_STMT (arg); - gcall *call_stmt = dyn_cast<gcall *> (arg_def); - if (!call_stmt) - return false; - tree callee_decl = gimple_call_fndecl (call_stmt); - if (!callee_decl) - return false; - if (!ipa && !DECL_IS_MALLOC (callee_decl)) - DUMP_AND_RETURN("callee_decl does not have malloc attribute for" - " non-ipa mode.") - - cgraph_edge *cs = node->get_edge (call_stmt); - if (cs) - { - ipa_call_summary *es = ipa_call_summaries->get (cs); - gcc_assert (es); - es->is_return_callee_uncaptured = true; - } - } + { + bool nonzero_arg = false; + + for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i) + { + tree arg = gimple_phi_arg_def (phi, i); + if (integer_zerop (arg)) + continue; + + if (TREE_CODE (arg) != SSA_NAME) + DUMP_AND_RETURN ("phi arg is not SSA_NAME."); + if (!check_retval_uses (arg, phi)) + DUMP_AND_RETURN ("phi arg has uses outside phi" + " and comparisons against 0.") + + nonzero_arg = true; + gimple *arg_def = SSA_NAME_DEF_STMT (arg); + gcall *call_stmt = dyn_cast<gcall *> (arg_def); + if (!call_stmt) + return false; + tree callee_decl = gimple_call_fndecl (call_stmt); + if (!callee_decl) + return false; + if (!ipa && !DECL_IS_MALLOC (callee_decl)) + DUMP_AND_RETURN("callee_decl does not have malloc attribute for" + " non-ipa mode.") + + cgraph_edge *cs = node->get_edge (call_stmt); + if (cs) + { + ipa_call_summary *es = ipa_call_summaries->get (cs); + gcc_assert (es); + es->is_return_callee_uncaptured = true; + } + } + + if (!nonzero_arg) + DUMP_AND_RETURN ("all aregs to phi are 0."); + } else DUMP_AND_RETURN("def_stmt of return value is not a call or phi-stmt.") diff --git a/gcc/testsuite/gcc.dg/ipa/pr83648.c b/gcc/testsuite/gcc.dg/ipa/pr83648.c new file mode 100644 index 00000000000..03b45de671b --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr83648.c @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-local-pure-const-details" } */ + +void *g(unsigned n) +{ + return n ? __builtin_malloc (n) : 0; +} + +/* { dg-final { scan-tree-dump "Function found to be malloc: g" "local-pure-const1" } } */
On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote: > > As Jakub pointed out for the case: > void *f() > { > return __builtin_malloc (0); > } > > The malloc propagation would set f() to malloc. > However AFAIU, malloc(0) returns NULL (?) and the function shouldn't > be marked as malloc ? This seems like a pretty significant concern. Given: return n ? 0 : __builtin_malloc (n); Is the function malloc-like enough to allow it to be marked? If not, then ISTM we have to be very conservative in what we mark. foo (n, m) { return n ? 0 : __builtin_malloc (m); } Is that malloc-like enough to mark? Jeff
On 11 January 2018 at 04:50, Jeff Law <law@redhat.com> wrote: > On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote: >> >> As Jakub pointed out for the case: >> void *f() >> { >> return __builtin_malloc (0); >> } >> >> The malloc propagation would set f() to malloc. >> However AFAIU, malloc(0) returns NULL (?) and the function shouldn't >> be marked as malloc ? > This seems like a pretty significant concern. Given: > > > return n ? 0 : __builtin_malloc (n); > > Is the function malloc-like enough to allow it to be marked? > > If not, then ISTM we have to be very conservative in what we mark. > > foo (n, m) > { > return n ? 0 : __builtin_malloc (m); > } > > Is that malloc-like enough to mark? Not sure. Should I make it more conservative by marking it as malloc only if the argument to __builtin_malloc is constant or it's value-range is known not to include 0? And similarly for __builtin_calloc ? Thanks, Prathamesh > Jeff
On 11 January 2018 at 10:34, Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> wrote: > On 11 January 2018 at 04:50, Jeff Law <law@redhat.com> wrote: >> On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote: >>> >>> As Jakub pointed out for the case: >>> void *f() >>> { >>> return __builtin_malloc (0); >>> } >>> >>> The malloc propagation would set f() to malloc. >>> However AFAIU, malloc(0) returns NULL (?) and the function shouldn't >>> be marked as malloc ? >> This seems like a pretty significant concern. Given: >> >> >> return n ? 0 : __builtin_malloc (n); >> >> Is the function malloc-like enough to allow it to be marked? >> >> If not, then ISTM we have to be very conservative in what we mark. >> >> foo (n, m) >> { >> return n ? 0 : __builtin_malloc (m); >> } >> >> Is that malloc-like enough to mark? > Not sure. Should I make it more conservative by marking it as malloc > only if the argument to __builtin_malloc > is constant or it's value-range is known not to include 0? And > similarly for __builtin_calloc ? But I suppose this constraint will make malloc propagation almost useless :( > > Thanks, > Prathamesh >> Jeff
On Tue, 9 Jan 2018, Prathamesh Kulkarni wrote: > As Jakub pointed out for the case: > void *f() > { > return __builtin_malloc (0); > } > > The malloc propagation would set f() to malloc. > However AFAIU, malloc(0) returns NULL (?) and the function shouldn't > be marked as malloc ? Why not? Even for void*f(){return 0;} are any of the properties of the malloc attribute violated? It seems to me that if you reject malloc(0), then even the standard malloc function should be rejected as well... -- Marc Glisse
On Thu, 11 Jan 2018, Marc Glisse wrote: > On Tue, 9 Jan 2018, Prathamesh Kulkarni wrote: > > > As Jakub pointed out for the case: > > void *f() > > { > > return __builtin_malloc (0); > > } > > > > The malloc propagation would set f() to malloc. > > However AFAIU, malloc(0) returns NULL (?) and the function shouldn't > > be marked as malloc ? > > Why not? Even for > void*f(){return 0;} > are any of the properties of the malloc attribute violated? It seems to > me that if you reject malloc(0), then even the standard malloc function > should be rejected as well... The constraint for the malloc attribute is that it returns a unique pointer for each invocation that cannot alias with any other pointer in the program, or NULL. So yes, { return 0; } qualifies as 'malloc' (but I wouldn't mark that on its own ;)). Also int x; void *f(size_t n) { if (x) return malloc (n); return 0; } does. The only constraint is really that the value feeding the return statement is either literal zero or produced by a call with the malloc attribute. So even void *__attribute__((malloc)) bar(); void *f(size_t n) { if (n == 2) return bar (); return malloc (n); } is malloc. Richard.
On 01/10/2018 04:20 PM, Jeff Law wrote: > On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote: >> >> As Jakub pointed out for the case: >> void *f() >> { >> return __builtin_malloc (0); >> } >> >> The malloc propagation would set f() to malloc. >> However AFAIU, malloc(0) returns NULL (?) and the function shouldn't >> be marked as malloc ? > This seems like a pretty significant concern. Given: > > > return n ? 0 : __builtin_malloc (n); > > Is the function malloc-like enough to allow it to be marked? A call to malloc(0) is specified to return either NULL or a pointer to a distinct object of zero size. As useless as it is, the function above satisfies the requirement. That said, suggesting to mark it as such wouldn't be terribly helpful (a different warning pointing out that it's useless and so probably buggy might be). > > If not, then ISTM we have to be very conservative in what we mark. > > foo (n, m) > { > return n ? 0 : __builtin_malloc (m); > } > > Is that malloc-like enough to mark? I think it would be strictly correct as well but again probably not very useful. In fact, even void* f (size_t n) { return 0; } is malloc-like, but having -Wsuggest-attribute point out that it be decorated with the malloc attribute probably wouldn't make the option very popular ;) Martin
On 01/11/2018 11:56 AM, Martin Sebor wrote: > On 01/10/2018 04:20 PM, Jeff Law wrote: >> On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote: >>> >>> As Jakub pointed out for the case: >>> void *f() >>> { >>> return __builtin_malloc (0); >>> } >>> >>> The malloc propagation would set f() to malloc. >>> However AFAIU, malloc(0) returns NULL (?) and the function shouldn't >>> be marked as malloc ? >> This seems like a pretty significant concern. Given: >> >> >> return n ? 0 : __builtin_malloc (n); >> >> Is the function malloc-like enough to allow it to be marked? > > A call to malloc(0) is specified to return either NULL or > a pointer to a distinct object of zero size. As useless as > it is, the function above satisfies the requirement. That > said, suggesting to mark it as such wouldn't be terribly > helpful (a different warning pointing out that it's useless > and so probably buggy might be). I'm mostly concerned with making sure it is not harmful to mark. If it were harmful to mark then the bar for marking gets much higher, possibly to the point where the analysis really isn't worth it. I doubt either case is worth the effort to try and detect for a warning though. Jeff
On 01/11/2018 12:22 PM, Jeff Law wrote: > On 01/11/2018 11:56 AM, Martin Sebor wrote: >> On 01/10/2018 04:20 PM, Jeff Law wrote: >>> On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote: >>>> >>>> As Jakub pointed out for the case: >>>> void *f() >>>> { >>>> return __builtin_malloc (0); >>>> } >>>> >>>> The malloc propagation would set f() to malloc. >>>> However AFAIU, malloc(0) returns NULL (?) and the function shouldn't >>>> be marked as malloc ? >>> This seems like a pretty significant concern. Given: >>> >>> >>> return n ? 0 : __builtin_malloc (n); >>> >>> Is the function malloc-like enough to allow it to be marked? >> >> A call to malloc(0) is specified to return either NULL or >> a pointer to a distinct object of zero size. As useless as >> it is, the function above satisfies the requirement. That >> said, suggesting to mark it as such wouldn't be terribly >> helpful (a different warning pointing out that it's useless >> and so probably buggy might be). > I'm mostly concerned with making sure it is not harmful to mark. If it > were harmful to mark then the bar for marking gets much higher, possibly > to the point where the analysis really isn't worth it. > > I doubt either case is worth the effort to try and detect for a warning > though. As long as the marked definition still satisfies the assumptions GCC makes about the function it won't be harmful. I don't know all the nuances of pointer aliasing in GCC that might rely on it but assuming they faithfully reflect the standard requirements it will be safe. The other aspect of the question is under what the conditions is suggesting the attribute meaningful. Without spending too much time on it, I think the condition should be that the function must return a pointer obtained from a call to an allocation function that depends on one or more of its arguments, either directly or indirectly, or NULL. Does that make sense or can you or someone think of some realistic use cases where this would be too broad? Martin
> > As long as the marked definition still satisfies the assumptions > GCC makes about the function it won't be harmful. I don't know > all the nuances of pointer aliasing in GCC that might rely on it > but assuming they faithfully reflect the standard requirements > it will be safe. > > The other aspect of the question is under what the conditions > is suggesting the attribute meaningful. Without spending too > much time on it, I think the condition should be that the > function must return a pointer obtained from a call to > an allocation function that depends on one or more of its > arguments, either directly or indirectly, or NULL. Does that > make sense or can you or someone think of some realistic use > cases where this would be too broad? I also think marking functions returning NULL as malloc should be OK correctness wise. I would not require the call to alloc function to depend on argument of the caller - it seems perfectly OK to me to just call malloc with constant argument, for instance. Honza > > Martin
On 01/11/2018 01:32 PM, Jan Hubicka wrote: >> >> As long as the marked definition still satisfies the assumptions >> GCC makes about the function it won't be harmful. I don't know >> all the nuances of pointer aliasing in GCC that might rely on it >> but assuming they faithfully reflect the standard requirements >> it will be safe. >> >> The other aspect of the question is under what the conditions >> is suggesting the attribute meaningful. Without spending too >> much time on it, I think the condition should be that the >> function must return a pointer obtained from a call to >> an allocation function that depends on one or more of its >> arguments, either directly or indirectly, or NULL. Does that >> make sense or can you or someone think of some realistic use >> cases where this would be too broad? > > I also think marking functions returning NULL as malloc should be > OK correctness wise. > I would not require the call to alloc function to depend on argument > of the caller - it seems perfectly OK to me to just call malloc with > constant argument, for instance. You're right, constant calls should be included as well. I was trying to exclude the pathological cases brought up in this thread but maybe they're not worth worrying about. Martin
On 01/10/2018 10:04 PM, Prathamesh Kulkarni wrote: > On 11 January 2018 at 04:50, Jeff Law <law@redhat.com> wrote: >> On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote: >>> >>> As Jakub pointed out for the case: >>> void *f() >>> { >>> return __builtin_malloc (0); >>> } >>> >>> The malloc propagation would set f() to malloc. >>> However AFAIU, malloc(0) returns NULL (?) and the function shouldn't >>> be marked as malloc ? >> This seems like a pretty significant concern. Given: >> >> >> return n ? 0 : __builtin_malloc (n); >> >> Is the function malloc-like enough to allow it to be marked? >> >> If not, then ISTM we have to be very conservative in what we mark. >> >> foo (n, m) >> { >> return n ? 0 : __builtin_malloc (m); >> } >> >> Is that malloc-like enough to mark? > Not sure. Should I make it more conservative by marking it as malloc > only if the argument to __builtin_malloc > is constant or it's value-range is known not to include 0? And > similarly for __builtin_calloc ? It looks like the consensus is we don't need to worry about the cases above. So unless Jakub chimes in with a solid reason, don't worry about them. Jeff
On 01/11/2018 01:27 AM, Richard Biener wrote: > On Thu, 11 Jan 2018, Marc Glisse wrote: > >> On Tue, 9 Jan 2018, Prathamesh Kulkarni wrote: >> >>> As Jakub pointed out for the case: >>> void *f() >>> { >>> return __builtin_malloc (0); >>> } >>> >>> The malloc propagation would set f() to malloc. >>> However AFAIU, malloc(0) returns NULL (?) and the function shouldn't >>> be marked as malloc ? >> >> Why not? Even for >> void*f(){return 0;} >> are any of the properties of the malloc attribute violated? It seems to >> me that if you reject malloc(0), then even the standard malloc function >> should be rejected as well... > > The constraint for the malloc attribute is that it returns a > unique pointer for each invocation that cannot alias with any > other pointer in the program, or NULL. So yes, { return 0; } > qualifies as 'malloc' (but I wouldn't mark that on its own ;)). It'd certainly be strange to see something like that marked as malloc. At some point the result of an allocation has to feed into the return value. > > Also > > int x; > > void *f(size_t n) > { > if (x) > return malloc (n); > return 0; > } > > does. The only constraint is really that the value feeding > the return statement is either literal zero or produced > by a call with the malloc attribute. So even Right. > > void *__attribute__((malloc)) bar(); > > void *f(size_t n) > { > if (n == 2) > return bar (); > return malloc (n); > } > > is malloc. Also agreed. jeff
On 12 January 2018 at 05:02, Jeff Law <law@redhat.com> wrote: > On 01/10/2018 10:04 PM, Prathamesh Kulkarni wrote: >> On 11 January 2018 at 04:50, Jeff Law <law@redhat.com> wrote: >>> On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote: >>>> >>>> As Jakub pointed out for the case: >>>> void *f() >>>> { >>>> return __builtin_malloc (0); >>>> } >>>> >>>> The malloc propagation would set f() to malloc. >>>> However AFAIU, malloc(0) returns NULL (?) and the function shouldn't >>>> be marked as malloc ? >>> This seems like a pretty significant concern. Given: >>> >>> >>> return n ? 0 : __builtin_malloc (n); >>> >>> Is the function malloc-like enough to allow it to be marked? >>> >>> If not, then ISTM we have to be very conservative in what we mark. >>> >>> foo (n, m) >>> { >>> return n ? 0 : __builtin_malloc (m); >>> } >>> >>> Is that malloc-like enough to mark? >> Not sure. Should I make it more conservative by marking it as malloc >> only if the argument to __builtin_malloc >> is constant or it's value-range is known not to include 0? And >> similarly for __builtin_calloc ? > It looks like the consensus is we don't need to worry about the cases > above. So unless Jakub chimes in with a solid reason, don't worry about > them. Thanks everyone for the clarification. The attached patch skips on 0 phi arg, and returns false if -fno-delete-null-pointer-checks is passed. With the patch, malloc_candidate_p returns true for return 0; or ret = phi<0, 0> return ret which I believe is OK as far as correctness is concerned. However as Martin points out suggesting malloc attribute for return 0 case is not ideal. I suppose we can track the return 0 (or when value range of return value is known not to include 0) corner case and avoid suggesting malloc for those ? Validation in progress. Is this patch OK for next stage-1 ? Thanks, Prathamesh > > Jeff diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c index 09ca3590039..3cf71d4c653 100644 --- a/gcc/ipa-pure-const.c +++ b/gcc/ipa-pure-const.c @@ -910,11 +910,13 @@ malloc_candidate_p (function *fun, bool ipa) #define DUMP_AND_RETURN(reason) \ { \ if (dump_file && (dump_flags & TDF_DETAILS)) \ - fprintf (dump_file, "%s", (reason)); \ + fprintf (dump_file, "\n%s is not a malloc candidate, reason: %s\n", \ + (node->name()), (reason)); \ return false; \ } - if (EDGE_COUNT (exit_block->preds) == 0) + if (EDGE_COUNT (exit_block->preds) == 0 + || !flag_delete_null_pointer_checks) return false; FOR_EACH_EDGE (e, ei, exit_block->preds) @@ -929,6 +931,9 @@ malloc_candidate_p (function *fun, bool ipa) if (!retval) DUMP_AND_RETURN("No return value.") + if (integer_zerop (retval)) + continue; + if (TREE_CODE (retval) != SSA_NAME || TREE_CODE (TREE_TYPE (retval)) != POINTER_TYPE) DUMP_AND_RETURN("Return value is not SSA_NAME or not a pointer type.") @@ -961,11 +966,14 @@ malloc_candidate_p (function *fun, bool ipa) for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i) { tree arg = gimple_phi_arg_def (phi, i); + if (integer_zerop (arg)) + continue; + if (TREE_CODE (arg) != SSA_NAME) - DUMP_AND_RETURN("phi arg is not SSA_NAME.") - if (!(arg == null_pointer_node || check_retval_uses (arg, phi))) - DUMP_AND_RETURN("phi arg has uses outside phi" - " and comparisons against 0.") + DUMP_AND_RETURN ("phi arg is not SSA_NAME."); + if (!check_retval_uses (arg, phi)) + DUMP_AND_RETURN ("phi arg has uses outside phi" + " and comparisons against 0.") gimple *arg_def = SSA_NAME_DEF_STMT (arg); gcall *call_stmt = dyn_cast<gcall *> (arg_def); diff --git a/gcc/testsuite/gcc.dg/ipa/pr83648-2.c b/gcc/testsuite/gcc.dg/ipa/pr83648-2.c new file mode 100644 index 00000000000..ffa7e462abe --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr83648-2.c @@ -0,0 +1,15 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fno-delete-null-pointer-checks -fdump-tree-local-pure-const-details" } */ + +void *g(unsigned n) +{ + return n ? __builtin_malloc (n) : 0; +} + +void *h() +{ + return 0; +} + +/* { dg-final { scan-tree-dump-not "Function found to be malloc: g" "local-pure-const1" } } */ +/* { dg-final { scan-tree-dump-not "Function found to be malloc: h" "local-pure-const1" } } */ diff --git a/gcc/testsuite/gcc.dg/ipa/pr83648.c b/gcc/testsuite/gcc.dg/ipa/pr83648.c new file mode 100644 index 00000000000..febfd7d9319 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr83648.c @@ -0,0 +1,15 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-local-pure-const-details" } */ + +void *g(unsigned n) +{ + return n ? __builtin_malloc (n) : 0; +} + +void *h() +{ + return 0; +} + +/* { dg-final { scan-tree-dump "Function found to be malloc: g" "local-pure-const1" } } */ +/* { dg-final { scan-tree-dump "Function found to be malloc: h" "local-pure-const1" } } */
On Fri, 12 Jan 2018, Prathamesh Kulkarni wrote: > On 12 January 2018 at 05:02, Jeff Law <law@redhat.com> wrote: > > On 01/10/2018 10:04 PM, Prathamesh Kulkarni wrote: > >> On 11 January 2018 at 04:50, Jeff Law <law@redhat.com> wrote: > >>> On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote: > >>>> > >>>> As Jakub pointed out for the case: > >>>> void *f() > >>>> { > >>>> return __builtin_malloc (0); > >>>> } > >>>> > >>>> The malloc propagation would set f() to malloc. > >>>> However AFAIU, malloc(0) returns NULL (?) and the function shouldn't > >>>> be marked as malloc ? > >>> This seems like a pretty significant concern. Given: > >>> > >>> > >>> return n ? 0 : __builtin_malloc (n); > >>> > >>> Is the function malloc-like enough to allow it to be marked? > >>> > >>> If not, then ISTM we have to be very conservative in what we mark. > >>> > >>> foo (n, m) > >>> { > >>> return n ? 0 : __builtin_malloc (m); > >>> } > >>> > >>> Is that malloc-like enough to mark? > >> Not sure. Should I make it more conservative by marking it as malloc > >> only if the argument to __builtin_malloc > >> is constant or it's value-range is known not to include 0? And > >> similarly for __builtin_calloc ? > > It looks like the consensus is we don't need to worry about the cases > > above. So unless Jakub chimes in with a solid reason, don't worry about > > them. > Thanks everyone for the clarification. The attached patch skips on 0 phi arg, > and returns false if -fno-delete-null-pointer-checks is passed. > > With the patch, malloc_candidate_p returns true for > return 0; > or > ret = phi<0, 0> > return ret > > which I believe is OK as far as correctness is concerned. > However as Martin points out suggesting malloc attribute for return 0 > case is not ideal. > I suppose we can track the return 0 (or when value range of return > value is known not to include 0) > corner case and avoid suggesting malloc for those ? > > Validation in progress. > Is this patch OK for next stage-1 ? Ok. Thanks, Richard.
On 12 January 2018 at 18:26, Richard Biener <rguenther@suse.de> wrote: > On Fri, 12 Jan 2018, Prathamesh Kulkarni wrote: > >> On 12 January 2018 at 05:02, Jeff Law <law@redhat.com> wrote: >> > On 01/10/2018 10:04 PM, Prathamesh Kulkarni wrote: >> >> On 11 January 2018 at 04:50, Jeff Law <law@redhat.com> wrote: >> >>> On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote: >> >>>> >> >>>> As Jakub pointed out for the case: >> >>>> void *f() >> >>>> { >> >>>> return __builtin_malloc (0); >> >>>> } >> >>>> >> >>>> The malloc propagation would set f() to malloc. >> >>>> However AFAIU, malloc(0) returns NULL (?) and the function shouldn't >> >>>> be marked as malloc ? >> >>> This seems like a pretty significant concern. Given: >> >>> >> >>> >> >>> return n ? 0 : __builtin_malloc (n); >> >>> >> >>> Is the function malloc-like enough to allow it to be marked? >> >>> >> >>> If not, then ISTM we have to be very conservative in what we mark. >> >>> >> >>> foo (n, m) >> >>> { >> >>> return n ? 0 : __builtin_malloc (m); >> >>> } >> >>> >> >>> Is that malloc-like enough to mark? >> >> Not sure. Should I make it more conservative by marking it as malloc >> >> only if the argument to __builtin_malloc >> >> is constant or it's value-range is known not to include 0? And >> >> similarly for __builtin_calloc ? >> > It looks like the consensus is we don't need to worry about the cases >> > above. So unless Jakub chimes in with a solid reason, don't worry about >> > them. >> Thanks everyone for the clarification. The attached patch skips on 0 phi arg, >> and returns false if -fno-delete-null-pointer-checks is passed. >> >> With the patch, malloc_candidate_p returns true for >> return 0; >> or >> ret = phi<0, 0> >> return ret >> >> which I believe is OK as far as correctness is concerned. >> However as Martin points out suggesting malloc attribute for return 0 >> case is not ideal. >> I suppose we can track the return 0 (or when value range of return >> value is known not to include 0) >> corner case and avoid suggesting malloc for those ? >> >> Validation in progress. >> Is this patch OK for next stage-1 ? > > Ok. I have committed this as r260250 after bootstrap+test on x86_64 on top of trunk. With the patch, we now emit a suggestion for malloc attribute for a function returning NULL, which may not be ideal. I was wondering for which cases should we avoid suggesting malloc attribute with -Wsuggest-attribute ? 1] Return value is NULL. 2] Return value is phi result, and all args of phi are 0. 3] Any other cases ? Thanks, Prathamesh > > Thanks, > Richard.
On Tue, 15 May 2018, Prathamesh Kulkarni wrote: > On 12 January 2018 at 18:26, Richard Biener <rguenther@suse.de> wrote: > > On Fri, 12 Jan 2018, Prathamesh Kulkarni wrote: > > > >> On 12 January 2018 at 05:02, Jeff Law <law@redhat.com> wrote: > >> > On 01/10/2018 10:04 PM, Prathamesh Kulkarni wrote: > >> >> On 11 January 2018 at 04:50, Jeff Law <law@redhat.com> wrote: > >> >>> On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote: > >> >>>> > >> >>>> As Jakub pointed out for the case: > >> >>>> void *f() > >> >>>> { > >> >>>> return __builtin_malloc (0); > >> >>>> } > >> >>>> > >> >>>> The malloc propagation would set f() to malloc. > >> >>>> However AFAIU, malloc(0) returns NULL (?) and the function shouldn't > >> >>>> be marked as malloc ? > >> >>> This seems like a pretty significant concern. Given: > >> >>> > >> >>> > >> >>> return n ? 0 : __builtin_malloc (n); > >> >>> > >> >>> Is the function malloc-like enough to allow it to be marked? > >> >>> > >> >>> If not, then ISTM we have to be very conservative in what we mark. > >> >>> > >> >>> foo (n, m) > >> >>> { > >> >>> return n ? 0 : __builtin_malloc (m); > >> >>> } > >> >>> > >> >>> Is that malloc-like enough to mark? > >> >> Not sure. Should I make it more conservative by marking it as malloc > >> >> only if the argument to __builtin_malloc > >> >> is constant or it's value-range is known not to include 0? And > >> >> similarly for __builtin_calloc ? > >> > It looks like the consensus is we don't need to worry about the cases > >> > above. So unless Jakub chimes in with a solid reason, don't worry about > >> > them. > >> Thanks everyone for the clarification. The attached patch skips on 0 phi arg, > >> and returns false if -fno-delete-null-pointer-checks is passed. > >> > >> With the patch, malloc_candidate_p returns true for > >> return 0; > >> or > >> ret = phi<0, 0> > >> return ret > >> > >> which I believe is OK as far as correctness is concerned. > >> However as Martin points out suggesting malloc attribute for return 0 > >> case is not ideal. > >> I suppose we can track the return 0 (or when value range of return > >> value is known not to include 0) > >> corner case and avoid suggesting malloc for those ? > >> > >> Validation in progress. > >> Is this patch OK for next stage-1 ? > > > > Ok. > I have committed this as r260250 after bootstrap+test on x86_64 on top of trunk. > With the patch, we now emit a suggestion for malloc attribute for a > function returning NULL, > which may not be ideal. I was wondering for which cases should we > avoid suggesting malloc attribute with -Wsuggest-attribute ? > > 1] Return value is NULL. Yes. > 2] Return value is phi result, and all args of phi are 0. In which case constant propagation should have eliminated the PHI. > 3] Any other cases ? Can't think of any. Please create testcases for all cases you fend off. Richard.
On 15 May 2018 at 12:20, Richard Biener <rguenther@suse.de> wrote: > On Tue, 15 May 2018, Prathamesh Kulkarni wrote: > >> On 12 January 2018 at 18:26, Richard Biener <rguenther@suse.de> wrote: >> > On Fri, 12 Jan 2018, Prathamesh Kulkarni wrote: >> > >> >> On 12 January 2018 at 05:02, Jeff Law <law@redhat.com> wrote: >> >> > On 01/10/2018 10:04 PM, Prathamesh Kulkarni wrote: >> >> >> On 11 January 2018 at 04:50, Jeff Law <law@redhat.com> wrote: >> >> >>> On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote: >> >> >>>> >> >> >>>> As Jakub pointed out for the case: >> >> >>>> void *f() >> >> >>>> { >> >> >>>> return __builtin_malloc (0); >> >> >>>> } >> >> >>>> >> >> >>>> The malloc propagation would set f() to malloc. >> >> >>>> However AFAIU, malloc(0) returns NULL (?) and the function shouldn't >> >> >>>> be marked as malloc ? >> >> >>> This seems like a pretty significant concern. Given: >> >> >>> >> >> >>> >> >> >>> return n ? 0 : __builtin_malloc (n); >> >> >>> >> >> >>> Is the function malloc-like enough to allow it to be marked? >> >> >>> >> >> >>> If not, then ISTM we have to be very conservative in what we mark. >> >> >>> >> >> >>> foo (n, m) >> >> >>> { >> >> >>> return n ? 0 : __builtin_malloc (m); >> >> >>> } >> >> >>> >> >> >>> Is that malloc-like enough to mark? >> >> >> Not sure. Should I make it more conservative by marking it as malloc >> >> >> only if the argument to __builtin_malloc >> >> >> is constant or it's value-range is known not to include 0? And >> >> >> similarly for __builtin_calloc ? >> >> > It looks like the consensus is we don't need to worry about the cases >> >> > above. So unless Jakub chimes in with a solid reason, don't worry about >> >> > them. >> >> Thanks everyone for the clarification. The attached patch skips on 0 phi arg, >> >> and returns false if -fno-delete-null-pointer-checks is passed. >> >> >> >> With the patch, malloc_candidate_p returns true for >> >> return 0; >> >> or >> >> ret = phi<0, 0> >> >> return ret >> >> >> >> which I believe is OK as far as correctness is concerned. >> >> However as Martin points out suggesting malloc attribute for return 0 >> >> case is not ideal. >> >> I suppose we can track the return 0 (or when value range of return >> >> value is known not to include 0) >> >> corner case and avoid suggesting malloc for those ? >> >> >> >> Validation in progress. >> >> Is this patch OK for next stage-1 ? >> > >> > Ok. >> I have committed this as r260250 after bootstrap+test on x86_64 on top of trunk. >> With the patch, we now emit a suggestion for malloc attribute for a >> function returning NULL, >> which may not be ideal. I was wondering for which cases should we >> avoid suggesting malloc attribute with -Wsuggest-attribute ? >> >> 1] Return value is NULL. > > Yes. > >> 2] Return value is phi result, and all args of phi are 0. > > In which case constant propagation should have eliminated the PHI. > >> 3] Any other cases ? > > Can't think of any. Please create testcases for all cases you > fend off. Hi, Does the attached patch look OK ? It simply checks in warn_function_malloc if function returns NULL and chooses not to warn in that case. Thanks, Prathamesh > > Richard. diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c index 567b615fb60..23e6b19a3c4 100644 --- a/gcc/ipa-pure-const.c +++ b/gcc/ipa-pure-const.c @@ -246,6 +246,21 @@ warn_function_const (tree decl, bool known_finite) static void warn_function_malloc (tree decl) { + function *fun = DECL_STRUCT_FUNCTION (decl); + + basic_block exit_bb = EXIT_BLOCK_PTR_FOR_FN (fun); + if (single_pred_p (exit_bb)) + { + basic_block ret_bb = single_pred (exit_bb); + gimple_stmt_iterator gsi = gsi_last_bb (ret_bb); + greturn *ret_stmt = dyn_cast<greturn *> (gsi_stmt (gsi)); + gcc_assert (ret_stmt); + tree retval = gimple_return_retval (ret_stmt); + gcc_assert (retval && (TREE_CODE (TREE_TYPE (retval)) == POINTER_TYPE)); + if (integer_zerop (retval)) + return; + } + static hash_set<tree> *warned_about; warned_about = suggest_attribute (OPT_Wsuggest_attribute_malloc, decl, diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr83648-3.c b/gcc/testsuite/gcc.dg/tree-ssa/pr83648-3.c new file mode 100644 index 00000000000..564216ceae9 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr83648-3.c @@ -0,0 +1,19 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -Wsuggest-attribute=malloc" } */ + +__attribute__((noinline)) +void *g(unsigned n) /* { dg-bogus "function might be candidate for 'malloc' attribute" } */ +{ + return 0; +} + +void *h(unsigned n) /* { dg-bogus "function might be candidate for 'malloc' attribute" } */ +{ + int f(int); + + if (f (n)) + return 0; + + f (n); + return 0; +}
On Thu, May 17, 2018 at 1:25 PM Prathamesh Kulkarni < prathamesh.kulkarni@linaro.org> wrote: > On 15 May 2018 at 12:20, Richard Biener <rguenther@suse.de> wrote: > > On Tue, 15 May 2018, Prathamesh Kulkarni wrote: > > > >> On 12 January 2018 at 18:26, Richard Biener <rguenther@suse.de> wrote: > >> > On Fri, 12 Jan 2018, Prathamesh Kulkarni wrote: > >> > > >> >> On 12 January 2018 at 05:02, Jeff Law <law@redhat.com> wrote: > >> >> > On 01/10/2018 10:04 PM, Prathamesh Kulkarni wrote: > >> >> >> On 11 January 2018 at 04:50, Jeff Law <law@redhat.com> wrote: > >> >> >>> On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote: > >> >> >>>> > >> >> >>>> As Jakub pointed out for the case: > >> >> >>>> void *f() > >> >> >>>> { > >> >> >>>> return __builtin_malloc (0); > >> >> >>>> } > >> >> >>>> > >> >> >>>> The malloc propagation would set f() to malloc. > >> >> >>>> However AFAIU, malloc(0) returns NULL (?) and the function shouldn't > >> >> >>>> be marked as malloc ? > >> >> >>> This seems like a pretty significant concern. Given: > >> >> >>> > >> >> >>> > >> >> >>> return n ? 0 : __builtin_malloc (n); > >> >> >>> > >> >> >>> Is the function malloc-like enough to allow it to be marked? > >> >> >>> > >> >> >>> If not, then ISTM we have to be very conservative in what we mark. > >> >> >>> > >> >> >>> foo (n, m) > >> >> >>> { > >> >> >>> return n ? 0 : __builtin_malloc (m); > >> >> >>> } > >> >> >>> > >> >> >>> Is that malloc-like enough to mark? > >> >> >> Not sure. Should I make it more conservative by marking it as malloc > >> >> >> only if the argument to __builtin_malloc > >> >> >> is constant or it's value-range is known not to include 0? And > >> >> >> similarly for __builtin_calloc ? > >> >> > It looks like the consensus is we don't need to worry about the cases > >> >> > above. So unless Jakub chimes in with a solid reason, don't worry about > >> >> > them. > >> >> Thanks everyone for the clarification. The attached patch skips on 0 phi arg, > >> >> and returns false if -fno-delete-null-pointer-checks is passed. > >> >> > >> >> With the patch, malloc_candidate_p returns true for > >> >> return 0; > >> >> or > >> >> ret = phi<0, 0> > >> >> return ret > >> >> > >> >> which I believe is OK as far as correctness is concerned. > >> >> However as Martin points out suggesting malloc attribute for return 0 > >> >> case is not ideal. > >> >> I suppose we can track the return 0 (or when value range of return > >> >> value is known not to include 0) > >> >> corner case and avoid suggesting malloc for those ? > >> >> > >> >> Validation in progress. > >> >> Is this patch OK for next stage-1 ? > >> > > >> > Ok. > >> I have committed this as r260250 after bootstrap+test on x86_64 on top of trunk. > >> With the patch, we now emit a suggestion for malloc attribute for a > >> function returning NULL, > >> which may not be ideal. I was wondering for which cases should we > >> avoid suggesting malloc attribute with -Wsuggest-attribute ? > >> > >> 1] Return value is NULL. > > > > Yes. > > > >> 2] Return value is phi result, and all args of phi are 0. > > > > In which case constant propagation should have eliminated the PHI. > > > >> 3] Any other cases ? > > > > Can't think of any. Please create testcases for all cases you > > fend off. > Hi, > Does the attached patch look OK ? > It simply checks in warn_function_malloc if function returns NULL and > chooses not to warn in that case. I think a better approach is to not add the pointless attribute. Richard. > Thanks, > Prathamesh > > > > Richard.
On Mon, May 14, 2018 at 11:11 PM, Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> wrote: > On 12 January 2018 at 18:26, Richard Biener <rguenther@suse.de> wrote: >> On Fri, 12 Jan 2018, Prathamesh Kulkarni wrote: >> >>> On 12 January 2018 at 05:02, Jeff Law <law@redhat.com> wrote: >>> > On 01/10/2018 10:04 PM, Prathamesh Kulkarni wrote: >>> >> On 11 January 2018 at 04:50, Jeff Law <law@redhat.com> wrote: >>> >>> On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote: >>> >>>> >>> >>>> As Jakub pointed out for the case: >>> >>>> void *f() >>> >>>> { >>> >>>> return __builtin_malloc (0); >>> >>>> } >>> >>>> >>> >>>> The malloc propagation would set f() to malloc. >>> >>>> However AFAIU, malloc(0) returns NULL (?) and the function shouldn't >>> >>>> be marked as malloc ? >>> >>> This seems like a pretty significant concern. Given: >>> >>> >>> >>> >>> >>> return n ? 0 : __builtin_malloc (n); >>> >>> >>> >>> Is the function malloc-like enough to allow it to be marked? >>> >>> >>> >>> If not, then ISTM we have to be very conservative in what we mark. >>> >>> >>> >>> foo (n, m) >>> >>> { >>> >>> return n ? 0 : __builtin_malloc (m); >>> >>> } >>> >>> >>> >>> Is that malloc-like enough to mark? >>> >> Not sure. Should I make it more conservative by marking it as malloc >>> >> only if the argument to __builtin_malloc >>> >> is constant or it's value-range is known not to include 0? And >>> >> similarly for __builtin_calloc ? >>> > It looks like the consensus is we don't need to worry about the cases >>> > above. So unless Jakub chimes in with a solid reason, don't worry about >>> > them. >>> Thanks everyone for the clarification. The attached patch skips on 0 phi arg, >>> and returns false if -fno-delete-null-pointer-checks is passed. >>> >>> With the patch, malloc_candidate_p returns true for >>> return 0; >>> or >>> ret = phi<0, 0> >>> return ret >>> >>> which I believe is OK as far as correctness is concerned. >>> However as Martin points out suggesting malloc attribute for return 0 >>> case is not ideal. >>> I suppose we can track the return 0 (or when value range of return >>> value is known not to include 0) >>> corner case and avoid suggesting malloc for those ? >>> >>> Validation in progress. >>> Is this patch OK for next stage-1 ? >> >> Ok. > I have committed this as r260250 after bootstrap+test on x86_64 on top of trunk. r260250 caused: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85820 -- H.J.
diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c index 09ca3590039..0406d5588d2 100644 --- a/gcc/ipa-pure-const.c +++ b/gcc/ipa-pure-const.c @@ -910,7 +910,8 @@ malloc_candidate_p (function *fun, bool ipa) #define DUMP_AND_RETURN(reason) \ { \ if (dump_file && (dump_flags & TDF_DETAILS)) \ - fprintf (dump_file, "%s", (reason)); \ + fprintf (dump_file, "\n%s is not a malloc candidate, reason: %s\n", \ + (node->name()), (reason)); \ return false; \ } @@ -961,11 +962,14 @@ malloc_candidate_p (function *fun, bool ipa) for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i) { tree arg = gimple_phi_arg_def (phi, i); + if (arg == null_pointer_node) + continue; + if (TREE_CODE (arg) != SSA_NAME) - DUMP_AND_RETURN("phi arg is not SSA_NAME.") - if (!(arg == null_pointer_node || check_retval_uses (arg, phi))) - DUMP_AND_RETURN("phi arg has uses outside phi" - " and comparisons against 0.") + DUMP_AND_RETURN ("phi arg is not SSA_NAME."); + if (!check_retval_uses (arg, phi)) + DUMP_AND_RETURN ("phi arg has uses outside phi" + " and comparisons against 0.") gimple *arg_def = SSA_NAME_DEF_STMT (arg); gcall *call_stmt = dyn_cast<gcall *> (arg_def); diff --git a/gcc/testsuite/gcc.dg/ipa/pr83648.c b/gcc/testsuite/gcc.dg/ipa/pr83648.c new file mode 100644 index 00000000000..03b45de671b --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr83648.c @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-local-pure-const-details" } */ + +void *g(unsigned n) +{ + return n ? __builtin_malloc (n) : 0; +} + +/* { dg-final { scan-tree-dump "Function found to be malloc: g" "local-pure-const1" } } */
Hi, malloc_candidate_p() in ipa-pure-const misses detecting that a function is malloc-like if the return value is result of PHI and one of the arguments of PHI is 0. For example: void g(unsigned n) { return (n) ? __builtin_malloc (n) : 0; } The reason is that the following check: if (TREE_CODE (arg) != SSA_NAME) DUMP_AND_RETURN ("phi arg is not SSA_NAME.") fails for arg with constant value 0 and malloc_candidate_p returns false. The patch simply skips the arg if it equals null_pointer_node. Does it look OK ? One concern I have is that with the patch, malloc_candidate_p will return true if all the args to PHI are NULL: retval = PHI<0, 0> return retval However I expect that PHI with all 0 args would be constant folded to 0 earlier, so this case shouldn't occur in practice ? Bootstrapped+tested on x86_64-unknown-linux-gnu. Cross-testing on arm*-*-* and aarch64*-*-* in progress. Thanks, Prathamesh