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

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 934 bytes --]

Hi,
This patch attempts to add returns_arg attribute for c-family
languages. For C++ methods, first arg is assumed to be this pointer,
similar to alloc_size.
I have a couple of doubts:

(a) I am not sure why DECL_ARGUMENTS (decl) in
handle_returns_arg_attribute returns NULL ? My intent was to check
that the return-type and argument-types are compatible.

(b) AFAIU the bottom two bits of call return flags are used for storing
args from 0 - 3 and the arg number can be obtained by
masking with ERF_RETURN_ARG_MASK.
So, does that mean we can only allow first 4 arguments to be annotated
with returns_arg attribute ?
In the patch, I use fn_spec ('argnum'), which gimple_call_return_flags
uses to mark the corresponding arg with ERF_RETURN_ARG:

(c) How to write dejaGNU test to check if fn_spec ('argnum') has been correctly
applied to the corresponding parameter ? In the patch, I am just
testing for validation.

Thanks,
Prathamesh

[-- Attachment #2: returns-arg-1.diff --]
[-- Type: application/x-patch, Size: 4240 bytes --]

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

[Joseph Myers]

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
joseph@codesourcery.com

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 2145 bytes --]

On Tue, 21 Jan 2020 at 04:35, Joseph Myers <joseph@codesourcery.com> 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
> joseph@codesourcery.com

[-- Attachment #2: returns-arg-2.diff --]
[-- Type: application/x-patch, Size: 4697 bytes --]

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

[Joseph Myers]

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 
%<returns_arg%>).

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Richard Biener]

On Fri, Jan 24, 2020 at 11:53 PM Joseph Myers <joseph@codesourcery.com> 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
> %<returns_arg%>).
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]

On Mon, 27 Jan 2020 at 17:36, Richard Biener <richard.guenther@gmail.com> wrote:
>
> On Fri, Jan 24, 2020 at 11:53 PM Joseph Myers <joseph@codesourcery.com> 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<argnum>" 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<argnum>"
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
> > %<returns_arg%>).
> >
> > --
> > Joseph S. Myers
> > joseph@codesourcery.com

[-- Attachment #2: returns-arg-3.diff --]
[-- Type: application/x-patch, Size: 6292 bytes --]

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

[Jakub Jelinek]

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.

	Jakub

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

[Prathamesh Kulkarni]

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 ?

Thanks,
Prathamesh
>
>         Jakub
>

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

[Jakub Jelinek]

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?

	Jakub

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

[Richard Biener]

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.

Richard.

>         Jakub
>

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 3419 bytes --]

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 ?

Thanks,
Prathamesh
>
> Richard.
>
> >         Jakub
> >

[-- Attachment #2: returns-arg-4.diff --]
[-- Type: application/x-patch, Size: 6484 bytes --]

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

[Richard Biener]

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.

> Thanks,
> Prathamesh
> >
> > Richard.
> >
> > >         Jakub
> > >

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 5140 bytes --]

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 ?

Thanks,
Prathamesh
>
> > Thanks,
> > Prathamesh
> > >
> > > Richard.
> > >
> > > >         Jakub
> > > >

[-- Attachment #2: returns-arg-5.diff --]
[-- Type: application/x-patch, Size: 6209 bytes --]

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 5599 bytes --]

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.

Thanks,
Prathamesh
>
> Thanks,
> Prathamesh
> >
> > > Thanks,
> > > Prathamesh
> > > >
> > > > Richard.
> > > >
> > > > >         Jakub
> > > > >

[-- Attachment #2: returns-arg-6.diff --]
[-- Type: text/x-patch, Size: 6208 bytes --]

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index dc9579c5c60..1bc249d470a 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,52 @@ 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 (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;
+    }
+
+  gcc_assert (args);
+  tree val = TREE_VALUE (args);
+  unsigned argnum = tree_to_uhwi (val);
+
+  if (argnum > ERF_RETURN_ARG_MASK)
+    {
+      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes,
+		  "%qE attribute ignored for argument %u", name, argnum);
+      *no_add_attrs = true;
+      return NULL_TREE;
+    }
+
+  *no_add_attrs = false;
+  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 @code{returns_arg} function attribute
+The @code{returns_arg}, when applied to an argument states that
+the function returns that argument umodified.
+For instance, the declaration:
+
+@smallexample
+int foo(int x) __attribute__((returns_arg(1)));
+@end smallexample
+
+@noindent
+lets the compiler know that foo returns x.
+
 @item returns_nonnull
 @cindex @code{returns_nonnull} function attribute
 The @code{returns_nonnull} attribute specifies that the function
diff --git a/gcc/gimple.c b/gcc/gimple.c
index 324e706d508..b44db71a9fe 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -1559,6 +1559,15 @@ gimple_call_return_flags (const gcall *stmt)
   if (gimple_call_flags (stmt) & ECF_MALLOC)
     return ERF_NOALIAS;
 
+  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)));
+      gcc_assert (argnum <= ERF_RETURN_ARG_MASK);
+      return ERF_RETURNS_ARG | argnum;
+    }
+
   attr = gimple_call_fnspec (stmt);
   if (!attr || TREE_STRING_LENGTH (attr) < 1)
     return 0;
diff --git a/gcc/testsuite/g++.dg/Wattributes-6.C b/gcc/testsuite/g++.dg/Wattributes-6.C
new file mode 100644
index 00000000000..fcf660a4684
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wattributes-6.C
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* Check that 'this' is counted as first arg to the attribute.  */
+
+struct X
+{
+  X *f() __attribute__((returns_arg(1)));
+};
+
+int main()
+{
+  X x;
+  x.f();
+}
diff --git a/gcc/testsuite/gcc.dg/Wattributes-11.c b/gcc/testsuite/gcc.dg/Wattributes-11.c
new file mode 100644
index 00000000000..81245b44cfa
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wattributes-11.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-Wattributes" } */
+
+int f1 (int) __attribute__((returns_arg)); /* { dg-error "wrong number of arguments specified for 'returns_arg' attribute" } */
+
+void f2 (int) __attribute__((returns_arg (1))); /* { dg-warning "'returns_arg' attribute ignored on a function returning 'void'" } */
+
+int f3 (int) __attribute__((returns_arg ("foo"))); /* { dg-warning "'returns_arg' attribute argument has type 'char\\\[4\\\]'" } */
+
+int f4 (char *) __attribute__((returns_arg (1))); /* { dg-warning "'returns_arg' attribute argument value '1' refers to parameter type 'char \\*'" } */
diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index 765ea2a9542..c4d78b20e57 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -111,17 +111,16 @@ struct die_struct;
 #define EAF_UNUSED		(1 << 3)
 
 /* Call return flags.  */
-/* Mask for the argument number that is returned.  Lower two bits of
-   the return flags, encodes argument slots zero to three.  */
-#define ERF_RETURN_ARG_MASK	(3)
+/* Mask for the argument number that is returned.  */
+#define ERF_RETURN_ARG_MASK	(0x3fff)
 
 /* 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 << 15)
 
 /* Nonzero if the return value does not alias with anything.  Functions
    with the malloc attribute have this set on their return value.  */
-#define ERF_NOALIAS		(1 << 3)
+#define ERF_NOALIAS		(1 << 14)
 
 
 /*---------------------------------------------------------------------------

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 6114 bytes --]

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
> > > > > >

[-- Attachment #2: returns-arg-7.diff --]
[-- Type: text/x-patch, Size: 6230 bytes --]

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index dc9579c5c60..1bc249d470a 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,52 @@ 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 (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;
+    }
+
+  gcc_assert (args);
+  tree val = TREE_VALUE (args);
+  unsigned argnum = tree_to_uhwi (val);
+
+  if (argnum > ERF_RETURN_ARG_MASK)
+    {
+      warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes,
+		  "%qE attribute ignored for argument %u", name, argnum);
+      *no_add_attrs = true;
+      return NULL_TREE;
+    }
+
+  *no_add_attrs = false;
+  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 @code{returns_arg} function attribute
+The @code{returns_arg}, when applied to an argument states that
+the function returns that argument umodified.
+For instance, the declaration:
+
+@smallexample
+int foo(int x) __attribute__((returns_arg(1)));
+@end smallexample
+
+@noindent
+lets the compiler know that foo returns x.
+
 @item returns_nonnull
 @cindex @code{returns_nonnull} function attribute
 The @code{returns_nonnull} attribute specifies that the function
diff --git a/gcc/gimple.c b/gcc/gimple.c
index 324e706d508..66150e95dd5 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -1559,6 +1559,18 @@ gimple_call_return_flags (const gcall *stmt)
   if (gimple_call_flags (stmt) & ECF_MALLOC)
     return ERF_NOALIAS;
 
+  tree fndecl = gimple_call_fndecl (stmt);
+  if (fndecl)
+    {
+      attr = lookup_attribute ("returns_arg", DECL_ATTRIBUTES (fndecl));
+      if (attr)
+	{
+	  unsigned argnum = tree_to_uhwi (TREE_VALUE (TREE_VALUE (attr)));
+	  gcc_assert (argnum <= ERF_RETURN_ARG_MASK);
+	  return ERF_RETURNS_ARG | argnum;
+	}
+    }
+
   attr = gimple_call_fnspec (stmt);
   if (!attr || TREE_STRING_LENGTH (attr) < 1)
     return 0;
diff --git a/gcc/testsuite/g++.dg/Wattributes-6.C b/gcc/testsuite/g++.dg/Wattributes-6.C
new file mode 100644
index 00000000000..fcf660a4684
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wattributes-6.C
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* Check that 'this' is counted as first arg to the attribute.  */
+
+struct X
+{
+  X *f() __attribute__((returns_arg(1)));
+};
+
+int main()
+{
+  X x;
+  x.f();
+}
diff --git a/gcc/testsuite/gcc.dg/Wattributes-11.c b/gcc/testsuite/gcc.dg/Wattributes-11.c
new file mode 100644
index 00000000000..81245b44cfa
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wattributes-11.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-Wattributes" } */
+
+int f1 (int) __attribute__((returns_arg)); /* { dg-error "wrong number of arguments specified for 'returns_arg' attribute" } */
+
+void f2 (int) __attribute__((returns_arg (1))); /* { dg-warning "'returns_arg' attribute ignored on a function returning 'void'" } */
+
+int f3 (int) __attribute__((returns_arg ("foo"))); /* { dg-warning "'returns_arg' attribute argument has type 'char\\\[4\\\]'" } */
+
+int f4 (char *) __attribute__((returns_arg (1))); /* { dg-warning "'returns_arg' attribute argument value '1' refers to parameter type 'char \\*'" } */
diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index 765ea2a9542..c4d78b20e57 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -111,17 +111,16 @@ struct die_struct;
 #define EAF_UNUSED		(1 << 3)
 
 /* Call return flags.  */
-/* Mask for the argument number that is returned.  Lower two bits of
-   the return flags, encodes argument slots zero to three.  */
-#define ERF_RETURN_ARG_MASK	(3)
+/* Mask for the argument number that is returned.  */
+#define ERF_RETURN_ARG_MASK	(0x3fff)
 
 /* 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 << 15)
 
 /* Nonzero if the return value does not alias with anything.  Functions
    with the malloc attribute have this set on their return value.  */
-#define ERF_NOALIAS		(1 << 3)
+#define ERF_NOALIAS		(1 << 14)
 
 
 /*---------------------------------------------------------------------------

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

[Prathamesh Kulkarni]

On Tue, 4 Feb 2020 at 14:54, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
>
> 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 ?
ping https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00159.html

Thanks,
Prathamesh
>
> Thanks,
> Prathamesh
> >
> > Thanks,
> > Prathamesh
> > >
> > > Thanks,
> > > Prathamesh
> > > >
> > > > > Thanks,
> > > > > Prathamesh
> > > > > >
> > > > > > Richard.
> > > > > >
> > > > > > >         Jakub
> > > > > > >

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

[Richard Biener]

On Wed, Feb 12, 2020 at 7:52 AM Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
>
> On Tue, 4 Feb 2020 at 14:54, Prathamesh Kulkarni
> <prathamesh.kulkarni@linaro.org> wrote:
> >
> > 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 ?
> ping https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00159.html

Queued for review, it's stage1 material anyways.

Richard.

> Thanks,
> Prathamesh
> >
> > Thanks,
> > Prathamesh
> > >
> > > Thanks,
> > > Prathamesh
> > > >
> > > > Thanks,
> > > > Prathamesh
> > > > >
> > > > > > Thanks,
> > > > > > Prathamesh
> > > > > > >
> > > > > > > Richard.
> > > > > > >
> > > > > > > >         Jakub
> > > > > > > >