On 15 May 2018 at 12:20, Richard Biener <rguent...@suse.de> wrote:
> On Tue, 15 May 2018, Prathamesh Kulkarni wrote:
>
>> On 12 January 2018 at 18:26, Richard Biener <rguent...@suse.de> wrote:
>> > On Fri, 12 Jan 2018, Prathamesh Kulkarni wrote:
>> >
>> >> On 12 January 2018 at 05:02, Jeff Law <l...@redhat.com> wrote:
>> >> > On 01/10/2018 10:04 PM, Prathamesh Kulkarni wrote:
>> >> >> On 11 January 2018 at 04:50, Jeff Law <l...@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;
+}
