[RFC] [c-family] PR92867 - Add returns_arg attribute

Tue Feb 4 09:24:00 GMT 2020

On Mon, 3 Feb 2020 at 14:56, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
>
> On Mon, 3 Feb 2020 at 14:41, Prathamesh Kulkarni
> <prathamesh.kulkarni@linaro.org> wrote:
> >
> > On Thu, 30 Jan 2020 at 19:17, Richard Biener <richard.guenther@gmail.com> wrote:
> > >
> > > On Thu, Jan 30, 2020 at 11:49 AM Prathamesh Kulkarni
> > > <prathamesh.kulkarni@linaro.org> wrote:
> > > >
> > > > On Wed, 29 Jan 2020 at 14:38, Richard Biener <richard.guenther@gmail.com> wrote:
> > > > >
> > > > > On Tue, Jan 28, 2020 at 1:02 PM Jakub Jelinek <jakub@redhat.com> wrote:
> > > > > >
> > > > > > On Tue, Jan 28, 2020 at 05:09:36PM +0530, Prathamesh Kulkarni wrote:
> > > > > > > On Tue, 28 Jan 2020 at 17:00, Jakub Jelinek <jakub@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Tue, Jan 28, 2020 at 04:56:59PM +0530, Prathamesh Kulkarni wrote:
> > > > > > > > > Thanks for the suggestions. In the attached patch I bumped up value of
> > > > > > > > > ERF_RETURNS_ARG_MASK
> > > > > > > > > to UINT_MAX >> 2, and use highest two bits for ERF_NOALIAS and ERF_RETURNS_ARG.
> > > > > > > > > And use fn spec "Z<argnum>" to store the argument number in fn-spec format.
> > > > > > > > > Does that look OK ?
> > > > > > > >
> > > > > > > > No.
> > > > > > > >
> > > > > > > > +#define ERF_RETURN_ARG_MASK    (UINT_MAX >> 2)
> > > > > > > >
> > > > > > > >  /* Nonzero if the return value is equal to the argument number
> > > > > > > >     flags & ERF_RETURN_ARG_MASK.  */
> > > > > > > > -#define ERF_RETURNS_ARG                (1 << 2)
> > > > > > > > +#define ERF_RETURNS_ARG                (1 << (BITS_PER_WORD - 2))
> > > > > > > >
> > > > > > > > How is size of host int related to BITS_PER_WORD?  Not to mention that
> > > > > > > > if BITS_PER_WORD is 64 and host int is 32-bit, 1 << (64 - 2) is UB.
> > > > > > > Oops sorry. I should have used HOST_BITS_PER_INT.
> > > > > > > I assume that'd be correct ?
> > > > > >
> > > > > > It still wouldn't, 1 << (HOST_BITS_PER_INT - 1) is negative number, you'd
> > > > > > need either 1U and verify all ERF_* flags uses, or avoid using the sign bit.
> > > > > > The patch has other issues, you don't verify that the argnum fits into
> > > > > > the bits (doesn't overflow into the other ERF_* bits), in
> > > > > > +  char *s = (char *) xmalloc (sizeof (char) * BITS_PER_WORD);
> > > > > > +  s[0] = 'Z';
> > > > > > +  sprintf (s + 1, "%lu", argnum);
> > > > > > 1) sizeof (char) is 1 by definition
> > > > > > 2) it is pointless to allocate it and then deallocate (just use automatic
> > > > > > array)
> > > > > > 3) it is unclear how is BITS_PER_WORD related to the length of decimal
> > > > > > encoded string + Z char + terminating '\0'.  The usual way is for unsigned
> > > > > > numbers to estimate number of digits by counting 3 digits per each 8 bits,
> > > > > > in your case of course + 2.
> > > > > >
> > > > > > More importantly, the "fn spec" attribute isn't used just in
> > > > > > gimple_call_return_flags, but also in e.g. gimple_call_arg_flags which
> > > > > > assumes that the return stuff in there is a single char and the reaming
> > > > > > chars are for argument descriptions, or in decl_return_flags which you
> > > > > > haven't modified.
> > > > > >
> > > > > > I must say I fail to see the point in trying to glue this together into the
> > > > > > "fn spec" argument so incompatibly, why can't we handle the fn spec with its
> > > > > > 1-4 returns_arg and returns_arg attribute with arbitrary position
> > > > > > side-by-side?
> > > > >
> > > > > Yeah, I wouldn't have added "fn spec" for "returns_arg" but kept the
> > > > > query interface thus access it via gimple_call_return_flags and use
> > > > > ERF_*.  For the flags adjustment just up the maximum argument
> > > > > to 1<<15 then the argument number is also nicely aligned, no need
> > > > > to do fancy limiting that depends on the host.  For too large
> > > > > argument numbers just warn the attribute is ignored.  I'd say even
> > > > > a max of 255 is sane just the existing limit is a bit too low.
> > > > Hi,
> > > > Thanks for the suggestions. In the attached patch, I use TREE_VALUE
> > > > (attr) to store/retrieve
> > > > arbitrary argument position, and have bumped ERF_RETURNS_ARG_MASK to 0x3fff.
> > > > Does it look OK ?
> > >
> > > +      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes,
> > > +                 "%qE attribute ignored on a function returning %qT.",
> > > +                 name, rettype);
> > >
> > > no punctuation in diagnostics (trailing '.'), also elsewhere in the patch.
> > >
> > > +  tree fndecl = gimple_call_fndecl (stmt);
> > > +  attr = lookup_attribute ("returns_arg", DECL_ATTRIBUTES (fndecl));
> > > +  if (attr)
> > > +    {
> > > +      unsigned argnum = tree_to_uhwi (TREE_VALUE (TREE_VALUE (attr)));
> > > +      return ERF_RETURNS_ARG | argnum;
> > >
> > > please verify argnum against ERF_ARG_MASK.
> > >
> > > +  tree attr = lookup_attribute ("returns_arg", DECL_ATTRIBUTES (decl));
> > > +  if (attr)
> > > +    TREE_VALUE (attr) = build_int_cst (unsigned_type_node, argnum);
> > > +  else
> > > +    DECL_ATTRIBUTES (decl)
> > > +      = tree_cons (get_identifier ("returns_arg"),
> > > +                  build_int_cst (unsigned_type_node, argnum),
> > > +                                 DECL_ATTRIBUTES (decl));
> > > +  return NULL_TREE;
> > >
> > > what's this for?  for *no_add_attrs = false you get the attribute
> > > added by the caller.
> > > Also other positional_argument callers overwrite TREE_VALUE with the result
> > > from the function.
> > Ah, thanks for pointing out!
> > Does attached version look OK ?
> Oops sorry, the previous patch contained typo ERF_RETURNS_ARG_MASK
> (should be ERF_RETURN_ARG_MASK).
> Fixed in this patch.
Validating previous patch reported a segfault because
gimple_call_fndecl returned NULL in gimple_call_return_flags.
Fixed in attached patch.
Does it look OK ?

Thanks,
Prathamesh
>
> Thanks,
> Prathamesh
> >
> > Thanks,
> > Prathamesh
> > >
> > > > Thanks,
> > > > Prathamesh
> > > > >
> > > > > Richard.
> > > > >
> > > > > >         Jakub
> > > > > >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: returns-arg-7.diff
Type: text/x-patch
Size: 6230 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20200204/fe8fdc41/attachment.bin>