Re: Fix PR78154

[Richard Biener <rguenther at suse dot de>]

On Tue, 22 Nov 2016, Prathamesh Kulkarni wrote:

> On 21 November 2016 at 15:34, Richard Biener <rguenther@suse.de> wrote:
> > On Fri, 18 Nov 2016, Prathamesh Kulkarni wrote:
> >
> >> On 17 November 2016 at 15:24, Richard Biener <rguenther@suse.de> wrote:
> >> > On Thu, 17 Nov 2016, Prathamesh Kulkarni wrote:
> >> >
> >> >> On 17 November 2016 at 14:21, Richard Biener <rguenther@suse.de> wrote:
> >> >> > On Thu, 17 Nov 2016, Prathamesh Kulkarni wrote:
> >> >> >
> >> >> >> Hi Richard,
> >> >> >> Following your suggestion in PR78154, the patch checks if stmt
> >> >> >> contains call to memmove (and friends) in gimple_stmt_nonzero_warnv_p
> >> >> >> and returns true in that case.
> >> >> >>
> >> >> >> Bootstrapped+tested on x86_64-unknown-linux-gnu.
> >> >> >> Cross-testing on arm*-*-*, aarch64*-*-* in progress.
> >> >> >> Would it be OK to commit this patch in stage-3 ?
> >> >> >
> >> >> > As people noted we have returns_nonnull for this and that is already
> >> >> > checked.  So please make sure the builtins get this attribute instead.
> >> >> OK thanks, I will add the returns_nonnull attribute to the required
> >> >> string builtins.
> >> >> I noticed some of the string builtins don't have RET1 in builtins.def:
> >> >> strcat, strncpy, strncat have ATTR_NOTHROW_NONNULL_LEAF.
> >> >> Should they instead be having ATTR_RET1_NOTHROW_NONNULL_LEAF similar
> >> >> to entries for memmove, strcpy ?
> >> >
> >> > Yes, I think so.
> >> Hi,
> >> In the attached patch I added returns_nonnull attribute to
> >> ATTR_RET1_NOTHROW_NONNULL_LEAF,
> >> and changed few builtins like strcat, strncpy, strncat and
> >> corresponding _chk builtins to use ATTR_RET1_NOTHROW_NONNULL_LEAF.
> >> Does the patch look correct ?
> >
> > Hmm, given you only change ATTR_RET1_NOTHROW_NONNULL_LEAF means that
> > the gimple_stmt_nonzero_warnv_p code is incomplete -- it should
> > infer returns_nonnull itself from RET1 (which is fnspec("1") basically)
> > and the nonnull attribute on the argument.  So
> >
> >   unsigned rf = gimple_call_return_flags (stmt);
> >   if (rf & ERF_RETURNS_ARG)
> >    {
> >      tree arg = gimple_call_arg (stmt, rf & ERF_RETURN_ARG_MASK);
> >      if (range of arg is ! VARYING)
> >        use range of arg;
> >      else if (infer_nonnull_range_by_attribute (stmt, arg))
> >         ... nonnull ...
> >
> Hi,
> Thanks for the suggestions, modified gimple_stmt_nonzero_warnv_p
> accordingly in this version.
> For functions like stpcpy that return nonnull but not one of it's
> arguments, I added new enum ATTR_RETNONNULL_NOTHROW_LEAF.
> Is that OK ?
> Bootstrapped+tested on x86_64-unknown-linux-gnu.
> Cross-testing on arm*-*-*, aarch64*-*-* in progress.

+               value_range *vr = get_value_range (arg);
+               if ((vr && vr->type != VR_VARYING)
+                   || infer_nonnull_range_by_attribute (stmt, arg))
+                 return true;
+             }

actually that's not quite correct (failed to notice the function
doesn't return a range but whether the range is nonnull).  For
nonnull it's just

  if (infer_nonnull_range_by_attribute (stmt, arg))
    return true;

in the extract_range_basic call handling we could handle
ERF_RETURNS_ARG by returning the range of the argument (if not varying).

Thus the patch is ok with the above condition changed.  Please refer
to the recently opened PR from the ChangeLog.

Thanks,
Richard.