Re: Fix PR78154

[Prathamesh Kulkarni <prathamesh dot kulkarni at linaro dot org>]

On 22 November 2016 at 20:23, Richard Biener <rguenther@suse.de> wrote:
> 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.
Err sorry I think had looked at wrong results for bootstrap+test for
previous patch :/
It caused ICE for pr55890-3.c and regressed nonnull-3.c, which is fixed in this
version, and changed the above condition to only check
infer_nonnull_range_by_attribute().
Bootstrapped+tested on x86_64-unknown-linux-gnu.
Cross-tested on arm*-*-*, aarch64*-*-*.
OK to commit ?

Thanks,
Prathamesh
>
> Thanks,
> Richard.

Attachment: pr78154-7.txt
Description: Text document