PR83648

Message ID CAAgBjMnap+eF165TdbZrpAmU2=stko03z0OOzsHwd_Bca+MN9g@mail.gmail.com
State New
Headers show
Series
  • PR83648
Related show

Commit Message

Prathamesh Kulkarni Jan. 3, 2018, 6:03 a.m.
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

Comments

Richard Biener Jan. 3, 2018, 9:05 a.m. | #1
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
Jakub Jelinek Jan. 3, 2018, 9:10 a.m. | #2
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
Jan Hubicka Jan. 3, 2018, 11:35 a.m. | #3
> 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
Jan Hubicka Jan. 3, 2018, 1:28 p.m. | #4
> 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" } } */
Jeff Law Jan. 3, 2018, 7:06 p.m. | #5
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
Prathamesh Kulkarni Jan. 9, 2018, 12:57 p.m. | #6
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" } } */
Jeff Law Jan. 10, 2018, 11:20 p.m. | #7
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
Prathamesh Kulkarni Jan. 11, 2018, 5:04 a.m. | #8
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
Prathamesh Kulkarni Jan. 11, 2018, 6:29 a.m. | #9
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
Marc Glisse Jan. 11, 2018, 7:12 a.m. | #10
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
Richard Biener Jan. 11, 2018, 8:27 a.m. | #11
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.
Martin Sebor Jan. 11, 2018, 6:56 p.m. | #12
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
Jeff Law Jan. 11, 2018, 7:22 p.m. | #13
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
Martin Sebor Jan. 11, 2018, 8:05 p.m. | #14
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
Jan Hubicka Jan. 11, 2018, 8:32 p.m. | #15
> 

> 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
Martin Sebor Jan. 11, 2018, 10:12 p.m. | #16
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
Jeff Law Jan. 11, 2018, 11:32 p.m. | #17
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
Jeff Law Jan. 11, 2018, 11:33 p.m. | #18
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
Prathamesh Kulkarni Jan. 12, 2018, 6:41 a.m. | #19
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" } } */
Richard Biener Jan. 12, 2018, 12:56 p.m. | #20
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.
Prathamesh Kulkarni May 15, 2018, 6:11 a.m. | #21
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.
Richard Biener May 15, 2018, 6:50 a.m. | #22
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.
Prathamesh Kulkarni May 17, 2018, 11:25 a.m. | #23
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;
+}
Richard Biener May 17, 2018, 11:29 a.m. | #24
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.
H.J. Lu May 17, 2018, 4:20 p.m. | #25
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.

Patch

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" } } */