Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Wed, Feb 12, 2020 at 7:52 AM Prathamesh Kulkarni wrote: > > On Tue, 4 Feb 2020 at 14:54, Prathamesh Kulkarni > wrote: > > > > On Mon, 3 Feb 2020 at 14:56, Prathamesh Kulkarni > > wrote: > > > > > > On Mon, 3 Feb 2020 at 14:41, Prathamesh Kulkarni > > > wrote: > > > > > > > > On Thu, 30 Jan 2020 at 19:17, Richard Biener > > > > wrote: > > > > > > > > > > On Thu, Jan 30, 2020 at 11:49 AM Prathamesh Kulkarni > > > > > wrote: > > > > > > > > > > > > On Wed, 29 Jan 2020 at 14:38, Richard Biener > > > > > > wrote: > > > > > > > > > > > > > > On Tue, Jan 28, 2020 at 1:02 PM Jakub Jelinek > > > > > > > wrote: > > > > > > > > > > > > > > > > On Tue, Jan 28, 2020 at 05:09:36PM +0530, Prathamesh Kulkarni > > > > > > > > wrote: > > > > > > > > > On Tue, 28 Jan 2020 at 17:00, Jakub Jelinek > > > > > > > > > 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" 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
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Tue, 4 Feb 2020 at 14:54, Prathamesh Kulkarni wrote: > > On Mon, 3 Feb 2020 at 14:56, Prathamesh Kulkarni > wrote: > > > > On Mon, 3 Feb 2020 at 14:41, Prathamesh Kulkarni > > wrote: > > > > > > On Thu, 30 Jan 2020 at 19:17, Richard Biener > > > wrote: > > > > > > > > On Thu, Jan 30, 2020 at 11:49 AM Prathamesh Kulkarni > > > > wrote: > > > > > > > > > > On Wed, 29 Jan 2020 at 14:38, Richard Biener > > > > > wrote: > > > > > > > > > > > > On Tue, Jan 28, 2020 at 1:02 PM Jakub Jelinek > > > > > > wrote: > > > > > > > > > > > > > > On Tue, Jan 28, 2020 at 05:09:36PM +0530, Prathamesh Kulkarni > > > > > > > wrote: > > > > > > > > On Tue, 28 Jan 2020 at 17:00, Jakub Jelinek > > > > > > > > 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" 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
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Mon, 3 Feb 2020 at 14:56, Prathamesh Kulkarni
wrote:
>
> On Mon, 3 Feb 2020 at 14:41, Prathamesh Kulkarni
> wrote:
> >
> > On Thu, 30 Jan 2020 at 19:17, Richard Biener
> > wrote:
> > >
> > > On Thu, Jan 30, 2020 at 11:49 AM Prathamesh Kulkarni
> > > wrote:
> > > >
> > > > On Wed, 29 Jan 2020 at 14:38, Richard Biener
> > > > wrote:
> > > > >
> > > > > On Tue, Jan 28, 2020 at 1:02 PM Jakub Jelinek
> > > > > wrote:
> > > > > >
> > > > > > On Tue, Jan 28, 2020 at 05:09:36PM +0530, Prathamesh Kulkarni wrote:
> > > > > > > On Tue, 28 Jan 2020 at 17:00, Jakub Jelinek
> > > > > > > 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" 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;
> > >
> > >
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Mon, 3 Feb 2020 at 14:41, Prathamesh Kulkarni
wrote:
>
> On Thu, 30 Jan 2020 at 19:17, Richard Biener
> wrote:
> >
> > On Thu, Jan 30, 2020 at 11:49 AM Prathamesh Kulkarni
> > wrote:
> > >
> > > On Wed, 29 Jan 2020 at 14:38, Richard Biener
> > > wrote:
> > > >
> > > > On Tue, Jan 28, 2020 at 1:02 PM Jakub Jelinek wrote:
> > > > >
> > > > > On Tue, Jan 28, 2020 at 05:09:36PM +0530, Prathamesh Kulkarni wrote:
> > > > > > On Tue, 28 Jan 2020 at 17:00, Jakub Jelinek
> > > > > > 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" 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
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Thu, 30 Jan 2020 at 19:17, Richard Biener wrote:
>
> On Thu, Jan 30, 2020 at 11:49 AM Prathamesh Kulkarni
> wrote:
> >
> > On Wed, 29 Jan 2020 at 14:38, Richard Biener
> > wrote:
> > >
> > > On Tue, Jan 28, 2020 at 1:02 PM Jakub Jelinek wrote:
> > > >
> > > > On Tue, Jan 28, 2020 at 05:09:36PM +0530, Prathamesh Kulkarni wrote:
> > > > > On Tue, 28 Jan 2020 at 17:00, Jakub Jelinek 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" 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!
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Thu, Jan 30, 2020 at 11:49 AM Prathamesh Kulkarni
wrote:
>
> On Wed, 29 Jan 2020 at 14:38, Richard Biener
> wrote:
> >
> > On Tue, Jan 28, 2020 at 1:02 PM Jakub Jelinek wrote:
> > >
> > > On Tue, Jan 28, 2020 at 05:09:36PM +0530, Prathamesh Kulkarni wrote:
> > > > On Tue, 28 Jan 2020 at 17:00, Jakub Jelinek 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" 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.
> Thanks,
> Prathamesh
> >
> > Richard.
> >
> > > Jakub
> > >
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Wed, 29 Jan 2020 at 14:38, Richard Biener wrote:
>
> On Tue, Jan 28, 2020 at 1:02 PM Jakub Jelinek wrote:
> >
> > On Tue, Jan 28, 2020 at 05:09:36PM +0530, Prathamesh Kulkarni wrote:
> > > On Tue, 28 Jan 2020 at 17:00, Jakub Jelinek 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" 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 ?
Thanks,
Prathamesh
>
> Richard.
>
> > Jakub
> >
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index dc9579c5c60..c6d5bbd1d7a 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -150,6 +150,7 @@ static tree handle_designated_init_attribute (tree *, tree, tree, int, bool *);
static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
int, bool *);
static tree handle_copy_attribute (tree *, tree, tree, int, bool *);
+static tree handle_returns_arg_attribute (tree *, tree, tree, int, bool *);
/* Helper to define attribute exclusions. */
#define ATTR_EXCL(name, function, type, variable) \
@@ -484,6 +485,8 @@ const struct attribute_spec c_common_attribute_table[] =
handle_noinit_attribute, attr_noinit_exclusions },
{ "access", 1, 3, false, true, true, false,
handle_access_attribute, NULL },
+ { "returns_arg", 1, 1, true, false, false, false,
+ handle_returns_arg_attribute, NULL },
{ NULL, 0, 0, false, false, false, false, NULL, NULL }
};
@@ -4603,6 +4606,60 @@ handle_patchable_function_entry_attribute (tree *, tree name, tree args,
return NULL_TREE;
}
+/* Handle a "returns_arg" attribute; arguments as in
+ struct attribute_spec.handler. */
+
+static tree
+handle_returns_arg_attribute (tree *node, tree name, tree args,
+ int, bool *no_add_attrs)
+{
+ tree decl = *node;
+ tree rettype = TREE_TYPE (decl);
+
+ if (TREE_CODE (rettype) == METHOD_TYPE
+ || TREE_CODE (rettype) == FUNCTION_TYPE)
+rettype = TREE_TYPE (rettype);
+
+ if (VOID_TYPE_P (rettype))
+{
+ warning_at
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Tue, Jan 28, 2020 at 1:02 PM Jakub Jelinek wrote: > > On Tue, Jan 28, 2020 at 05:09:36PM +0530, Prathamesh Kulkarni wrote: > > On Tue, 28 Jan 2020 at 17:00, Jakub Jelinek 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" 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. Richard. > Jakub >
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Tue, Jan 28, 2020 at 05:09:36PM +0530, Prathamesh Kulkarni wrote: > On Tue, 28 Jan 2020 at 17:00, Jakub Jelinek 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" 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? Jakub
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Tue, 28 Jan 2020 at 17:00, Jakub Jelinek 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" 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 ? Thanks, Prathamesh > > Jakub >
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
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" 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. Jakub
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Mon, 27 Jan 2020 at 17:36, Richard Biener wrote:
>
> On Fri, Jan 24, 2020 at 11:53 PM Joseph Myers wrote:
> >
> > On Fri, 24 Jan 2020, Prathamesh Kulkarni wrote:
> >
> > > The middle-end representation issue of ERF_RETURNS_ARG still remains,
> > > which restricts the attribute till first four args. The patch simply
> > > emits sorry(), for arguments beyond first four..
> >
> > I think this should be fixed (e.g. make the middle-end check for the
> > attribute, or something like that).
>
> Since it's pure optimization you can also simply chose to ignore this without
> notice.
>
> Note ERF_RETURN_ARG_MASK is limited to the number of available
> bits in an int and practically the only current setter was via "fn spec"
> which uses a single digit [1-9] to denote the argument (so limiting to
> three is indeed an odd choice but matches builtins using this at the moment).
>
> Feel free to up ERF_RETURN_ARG_MASK (but then you need to adjust
> the ERF_ flag defines).
Hi,
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" to store the argument number in fn-spec format.
Does that look OK ?
In gimple_call_return_flags, I didn't remove the existing fn spec
"0-3" in this patch, since RET1 (and possibly others?) depend on it.
I will remove that and adjust other cases to use fn-spec "Z"
if that's OK, in follow-up patch.
Thanks,
Prathamesh
>
> > The language semantics of the
> > attribute should not be driven by such internal implementation details;
> > rather, implementation details should be determined by the language
> > semantics to be implemented.
> >
> > The sorry () has coding style issues. Diagnostics should not end with '.'
> > or '\n', should use full words (attribute not attr, arguments not args)
> > and programming language text in them should be surrounded by %<%> (so
> > %).
> >
> > --
> > Joseph S. Myers
> > jos...@codesourcery.com
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index dc9579c5c60..2ed41ed136d 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -150,6 +150,7 @@ static tree handle_designated_init_attribute (tree *, tree, tree, int, bool *);
static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
int, bool *);
static tree handle_copy_attribute (tree *, tree, tree, int, bool *);
+static tree handle_returns_arg_attribute (tree *, tree, tree, int, bool *);
/* Helper to define attribute exclusions. */
#define ATTR_EXCL(name, function, type, variable) \
@@ -484,6 +485,8 @@ const struct attribute_spec c_common_attribute_table[] =
handle_noinit_attribute, attr_noinit_exclusions },
{ "access", 1, 3, false, true, true, false,
handle_access_attribute, NULL },
+ { "returns_arg", 1, 1, true, false, false, false,
+ handle_returns_arg_attribute, NULL },
{ NULL, 0, 0, false, false, false, false, NULL, NULL }
};
@@ -4603,6 +4606,53 @@ handle_patchable_function_entry_attribute (tree *, tree name, tree args,
return NULL_TREE;
}
+/* Handle a "returns_arg" attribute; arguments as in
+ struct attribute_spec.handler. */
+
+static tree
+handle_returns_arg_attribute (tree *node, tree name, tree args,
+ int flags, bool *no_add_attrs)
+{
+ tree decl = *node;
+ tree rettype = TREE_TYPE (decl);
+
+ if (TREE_CODE (rettype) == METHOD_TYPE
+ || TREE_CODE (rettype) == FUNCTION_TYPE)
+rettype = TREE_TYPE (rettype);
+
+ if (VOID_TYPE_P (rettype))
+{
+ warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes,
+ "%qE attribute ignored on a function returning %qT.",
+ name, rettype);
+ *no_add_attrs = true;
+ return NULL_TREE;
+}
+
+ if (!positional_argument (TREE_TYPE (decl), name, TREE_VALUE (args),
+ TREE_CODE (rettype)))
+{
+ *no_add_attrs = true;
+ return NULL_TREE;
+}
+
+ *no_add_attrs = false;
+ gcc_assert (args);
+ tree val = TREE_VALUE (args);
+ unsigned HOST_WIDE_INT argnum = tree_to_uhwi (val);
+ char *s = (char *) xmalloc (sizeof (char) * BITS_PER_WORD);
+ s[0] = 'Z';
+ sprintf (s + 1, "%lu", argnum);
+
+ tree attr = tree_cons (get_identifier ("fn spec"),
+ build_tree_list (NULL_TREE,
+ build_string (strlen (s), s)),
+ NULL_TREE);
+ decl_attributes (node, attr, flags);
+ free (s);
+ return NULL_TREE;
+}
+
/* Attempt to partially validate a single attribute ATTR as if
it were to be applied to an entity OPER. */
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index ec99c38a607..3531e0c8292 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -3566,6 +3566,19 @@ diagnosed. Because a pure function cannot have any observable side
effects it does not make sense for such a function to return @code{void}.
Declaring such a function is diagnosed.
+@item returns_arg (@var{position})
+@cindex
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Fri, Jan 24, 2020 at 11:53 PM Joseph Myers wrote: > > On Fri, 24 Jan 2020, Prathamesh Kulkarni wrote: > > > The middle-end representation issue of ERF_RETURNS_ARG still remains, > > which restricts the attribute till first four args. The patch simply > > emits sorry(), for arguments beyond first four.. > > I think this should be fixed (e.g. make the middle-end check for the > attribute, or something like that). Since it's pure optimization you can also simply chose to ignore this without notice. Note ERF_RETURN_ARG_MASK is limited to the number of available bits in an int and practically the only current setter was via "fn spec" which uses a single digit [1-9] to denote the argument (so limiting to three is indeed an odd choice but matches builtins using this at the moment). Feel free to up ERF_RETURN_ARG_MASK (but then you need to adjust the ERF_ flag defines). > The language semantics of the > attribute should not be driven by such internal implementation details; > rather, implementation details should be determined by the language > semantics to be implemented. > > The sorry () has coding style issues. Diagnostics should not end with '.' > or '\n', should use full words (attribute not attr, arguments not args) > and programming language text in them should be surrounded by %<%> (so > %). > > -- > Joseph S. Myers > jos...@codesourcery.com
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Fri, 24 Jan 2020, Prathamesh Kulkarni wrote: > The middle-end representation issue of ERF_RETURNS_ARG still remains, > which restricts the attribute till first four args. The patch simply > emits sorry(), for arguments beyond first four.. I think this should be fixed (e.g. make the middle-end check for the attribute, or something like that). The language semantics of the attribute should not be driven by such internal implementation details; rather, implementation details should be determined by the language semantics to be implemented. The sorry () has coding style issues. Diagnostics should not end with '.' or '\n', should use full words (attribute not attr, arguments not args) and programming language text in them should be surrounded by %<%> (so %). -- Joseph S. Myers jos...@codesourcery.com
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Tue, 21 Jan 2020 at 04:35, Joseph Myers wrote:
>
> On Mon, 20 Jan 2020, Prathamesh Kulkarni wrote:
>
> > Hi,
> > This patch attempts to add returns_arg attribute for c-family
> > languages. For C++ methods, first arg is assumed to be this pointer,
>
> This is missing .texi documentation explaining the attribute and the cases
> for which it would be useful.
>
> A restriction to the first 4 arguments is not a good design of a language
> feature, whatever implementation issues there may be.
>
> Do you intend to update builtins.def in a followup patch for the various
> built-in functions (e.g. memcpy) for which such an attribute would be
> applicable?
>
> When extracting an integer value from an INTEGER_CST provided in user
> source code, you should always use tree_to_uhwi / tree_to_shwi as
> appropriate, after checking the relevant tree_fits_*, rather than using
> TREE_INT_CST_LOW directly, to avoid mishandling arguments that have a
> small number in the low part of the INTEGER_CST but have bits set in the
> high part as well. Any direct use of TREE_INT_CST_LOW should have a
> specific justification for why it is correct to discard the high part of
> the integer. See c-attribs.c:positional_argument, and try to use that
> function if possible rather than reimplementing bits of it, so that
> handling of attribute arguments giving the position of a function argument
> can be as consistent as possible between different attributes.
>
> There are coding style issues, e.g. diagnostics should not end with '.'
> and lines should be broken before not after an operator.
Hi Joseph,
Thanks for the suggestions. Using positional_argument helped to
simplify the patch,
and also catches the case when return-type and arg-type differ.
Does it look OK ?
I will update builtins.def in follow-up patch.
The middle-end representation issue of ERF_RETURNS_ARG still remains,
which restricts the attribute till first four args. The patch simply
emits sorry(), for arguments beyond first four..
I will try to address this in follow up patch.
Thanks,
Prathamesh
>
> --
> Joseph S. Myers
> jos...@codesourcery.com
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index dc9579c5c60..baed1b811ba 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -150,6 +150,7 @@ static tree handle_designated_init_attribute (tree *, tree, tree, int, bool *);
static tree handle_patchable_function_entry_attribute (tree *, tree, tree,
int, bool *);
static tree handle_copy_attribute (tree *, tree, tree, int, bool *);
+static tree handle_returns_arg_attribute (tree *, tree, tree, int, bool *);
/* Helper to define attribute exclusions. */
#define ATTR_EXCL(name, function, type, variable) \
@@ -484,6 +485,8 @@ const struct attribute_spec c_common_attribute_table[] =
handle_noinit_attribute, attr_noinit_exclusions },
{ "access", 1, 3, false, true, true, false,
handle_access_attribute, NULL },
+ { "returns_arg", 1, 1, true, false, false, false,
+ handle_returns_arg_attribute, NULL },
{ NULL, 0, 0, false, false, false, false, NULL, NULL }
};
@@ -4603,6 +4606,55 @@ handle_patchable_function_entry_attribute (tree *, tree name, tree args,
return NULL_TREE;
}
+/* Handle a "returns_arg" attribute; arguments as in
+ struct attribute_spec.handler. */
+
+static tree
+handle_returns_arg_attribute (tree *node, tree name, tree args,
+ int flags, bool *no_add_attrs)
+{
+ tree decl = *node;
+ tree rettype = TREE_TYPE (decl);
+
+ if (TREE_CODE (rettype) == METHOD_TYPE
+ || TREE_CODE (rettype) == FUNCTION_TYPE)
+rettype = TREE_TYPE (rettype);
+
+ if (VOID_TYPE_P (rettype))
+{
+ warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes,
+ "%qE attribute ignored on a function returning %qT.",
+ name, rettype);
+ *no_add_attrs = true;
+ return NULL_TREE;
+}
+
+ if (!positional_argument (TREE_TYPE (decl), name, TREE_VALUE (args),
+ TREE_CODE (rettype)))
+{
+ *no_add_attrs = true;
+ return NULL_TREE;
+}
+
+ *no_add_attrs = false;
+ gcc_assert (args);
+ tree val = TREE_VALUE (args);
+ unsigned HOST_WIDE_INT argnum = tree_to_uhwi (val);
+
+ if (argnum >= 4)
+sorry ("returns_arg attr can only be applied to first four args.\n");
+
+ char s[2];
+ s[0] = argnum + '0';
+ s[1] = '\0';
+
+ tree attr = tree_cons (get_identifier ("fn spec"),
+ build_tree_list (NULL_TREE, build_string (1, s)),
+ NULL_TREE);
+ decl_attributes (node, attr, flags);
+ return NULL_TREE;
+}
+
/* Attempt to partially validate a single attribute ATTR as if
it were to be applied to an entity OPER. */
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index ec99c38a607..3531e0c8292 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -3566,6 +3566,19 @@ diagnosed. Because a pure function cannot have any observable side
effects it does not make sense for such a function to
Re: [RFC] [c-family] PR92867 - Add returns_arg attribute
On Mon, 20 Jan 2020, Prathamesh Kulkarni wrote: > Hi, > This patch attempts to add returns_arg attribute for c-family > languages. For C++ methods, first arg is assumed to be this pointer, This is missing .texi documentation explaining the attribute and the cases for which it would be useful. A restriction to the first 4 arguments is not a good design of a language feature, whatever implementation issues there may be. Do you intend to update builtins.def in a followup patch for the various built-in functions (e.g. memcpy) for which such an attribute would be applicable? When extracting an integer value from an INTEGER_CST provided in user source code, you should always use tree_to_uhwi / tree_to_shwi as appropriate, after checking the relevant tree_fits_*, rather than using TREE_INT_CST_LOW directly, to avoid mishandling arguments that have a small number in the low part of the INTEGER_CST but have bits set in the high part as well. Any direct use of TREE_INT_CST_LOW should have a specific justification for why it is correct to discard the high part of the integer. See c-attribs.c:positional_argument, and try to use that function if possible rather than reimplementing bits of it, so that handling of attribute arguments giving the position of a function argument can be as consistent as possible between different attributes. There are coding style issues, e.g. diagnostics should not end with '.' and lines should be broken before not after an operator. -- Joseph S. Myers jos...@codesourcery.com
