This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: PR83648
- From: Richard Biener <rguenther at suse dot de>
- To: Prathamesh Kulkarni <prathamesh dot kulkarni at linaro dot org>
- Cc: Jeff Law <law at redhat dot com>, Jan Hubicka <hubicka at ucw dot cz>, gcc Patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 15 May 2018 08:50:11 +0200 (CEST)
- Subject: Re: PR83648
- References: <CAAgBjMnap+eF165TdbZrpAmU2=stko03z0OOzsHwd_Bca+MN9g@mail.gmail.com> <20180103132859.GC39213@kam.mff.cuni.cz> <CAAgBjMksL=HYaQyA2qNbf3Q=hF+CwAu2Rw+ZyJm4TvzRL3Fr9A@mail.gmail.com> <9c2a52f5-b54f-3b29-3aa6-bfb595e2b788@redhat.com> <CAAgBjMnTXXU6gVxe2n=HMMgKb_hA3NbGL7A4_KovZypSrtBhOg@mail.gmail.com> <f31b4d23-a6b5-9f85-1fe3-f18ffb68b3fb@redhat.com> <CAAgBjM=g-dBoo_9iNnhbO2=NsH62ePLGgdKSw8in23B60gAP6w@mail.gmail.com> <alpine.LSU.2.20.1801121356410.32271@zhemvz.fhfr.qr> <CAAgBjMkheoypOx7e6kx3GXo-x5YvmLPfEB=XsvEtvA6K7g6xxw@mail.gmail.com>
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.