On Thu, May 17, 2018 at 1:25 PM Prathamesh Kulkarni <
prathamesh.kulka...@linaro.org> wrote:
> 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.
I think a better approach is to not add the pointless attribute.
Richard.
> Thanks,
> Prathamesh
> >
> > Richard.
