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.
Richard.
