* [RFC] [c-family] PR92867 - Add returns_arg attribute @ 2020-01-20 12:21 Prathamesh Kulkarni 2020-01-20 23:23 ` Joseph Myers 0 siblings, 1 reply; 17+ messages in thread From: Prathamesh Kulkarni @ 2020-01-20 12:21 UTC (permalink / raw) To: gcc Patches, Jakub Jelinek [-- 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 --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-01-20 12:21 [RFC] [c-family] PR92867 - Add returns_arg attribute Prathamesh Kulkarni @ 2020-01-20 23:23 ` Joseph Myers 2020-01-24 12:22 ` Prathamesh Kulkarni 0 siblings, 1 reply; 17+ messages in thread From: Joseph Myers @ 2020-01-20 23:23 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: gcc Patches, Jakub Jelinek 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-01-20 23:23 ` Joseph Myers @ 2020-01-24 12:22 ` Prathamesh Kulkarni 2020-01-25 2:40 ` Joseph Myers 0 siblings, 1 reply; 17+ messages in thread From: Prathamesh Kulkarni @ 2020-01-24 12:22 UTC (permalink / raw) To: Joseph Myers; +Cc: gcc Patches, Jakub Jelinek [-- 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 --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-01-24 12:22 ` Prathamesh Kulkarni @ 2020-01-25 2:40 ` Joseph Myers 2020-01-27 12:46 ` Richard Biener 0 siblings, 1 reply; 17+ messages in thread From: Joseph Myers @ 2020-01-25 2:40 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: gcc Patches, Jakub Jelinek 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-01-25 2:40 ` Joseph Myers @ 2020-01-27 12:46 ` Richard Biener 2020-01-28 11:47 ` Prathamesh Kulkarni 0 siblings, 1 reply; 17+ messages in thread From: Richard Biener @ 2020-01-27 12:46 UTC (permalink / raw) To: Joseph Myers; +Cc: Prathamesh Kulkarni, gcc Patches, Jakub Jelinek 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-01-27 12:46 ` Richard Biener @ 2020-01-28 11:47 ` Prathamesh Kulkarni 2020-01-28 11:53 ` Jakub Jelinek 0 siblings, 1 reply; 17+ messages in thread From: Prathamesh Kulkarni @ 2020-01-28 11:47 UTC (permalink / raw) To: Richard Biener; +Cc: Joseph Myers, gcc Patches, Jakub Jelinek [-- 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 --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-01-28 11:47 ` Prathamesh Kulkarni @ 2020-01-28 11:53 ` Jakub Jelinek 2020-01-28 12:02 ` Prathamesh Kulkarni 0 siblings, 1 reply; 17+ messages in thread From: Jakub Jelinek @ 2020-01-28 11:53 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: Richard Biener, Joseph Myers, gcc Patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-01-28 11:53 ` Jakub Jelinek @ 2020-01-28 12:02 ` Prathamesh Kulkarni 2020-01-28 12:25 ` Jakub Jelinek 0 siblings, 1 reply; 17+ messages in thread From: Prathamesh Kulkarni @ 2020-01-28 12:02 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Biener, Joseph Myers, gcc Patches 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-01-28 12:02 ` Prathamesh Kulkarni @ 2020-01-28 12:25 ` Jakub Jelinek 2020-01-29 10:06 ` Richard Biener 0 siblings, 1 reply; 17+ messages in thread From: Jakub Jelinek @ 2020-01-28 12:25 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: Richard Biener, Joseph Myers, gcc Patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-01-28 12:25 ` Jakub Jelinek @ 2020-01-29 10:06 ` Richard Biener 2020-01-30 12:16 ` Prathamesh Kulkarni 0 siblings, 1 reply; 17+ messages in thread From: Richard Biener @ 2020-01-29 10:06 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Prathamesh Kulkarni, Joseph Myers, gcc Patches 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-01-29 10:06 ` Richard Biener @ 2020-01-30 12:16 ` Prathamesh Kulkarni 2020-01-30 14:10 ` Richard Biener 0 siblings, 1 reply; 17+ messages in thread From: Prathamesh Kulkarni @ 2020-01-30 12:16 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, Joseph Myers, gcc Patches [-- 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 --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-01-30 12:16 ` Prathamesh Kulkarni @ 2020-01-30 14:10 ` Richard Biener 2020-02-03 9:12 ` Prathamesh Kulkarni 0 siblings, 1 reply; 17+ messages in thread From: Richard Biener @ 2020-01-30 14:10 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: Jakub Jelinek, Joseph Myers, gcc Patches 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 > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-01-30 14:10 ` Richard Biener @ 2020-02-03 9:12 ` Prathamesh Kulkarni 2020-02-03 9:27 ` Prathamesh Kulkarni 0 siblings, 1 reply; 17+ messages in thread From: Prathamesh Kulkarni @ 2020-02-03 9:12 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, Joseph Myers, gcc Patches [-- 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 --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-02-03 9:12 ` Prathamesh Kulkarni @ 2020-02-03 9:27 ` Prathamesh Kulkarni 2020-02-04 9:24 ` Prathamesh Kulkarni 0 siblings, 1 reply; 17+ messages in thread From: Prathamesh Kulkarni @ 2020-02-03 9:27 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, Joseph Myers, gcc Patches [-- 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) /*--------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-02-03 9:27 ` Prathamesh Kulkarni @ 2020-02-04 9:24 ` Prathamesh Kulkarni 2020-02-12 6:52 ` Prathamesh Kulkarni 0 siblings, 1 reply; 17+ messages in thread From: Prathamesh Kulkarni @ 2020-02-04 9:24 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, Joseph Myers, gcc Patches [-- 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) /*--------------------------------------------------------------------------- ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-02-04 9:24 ` Prathamesh Kulkarni @ 2020-02-12 6:52 ` Prathamesh Kulkarni 2020-02-12 8:35 ` Richard Biener 0 siblings, 1 reply; 17+ messages in thread From: Prathamesh Kulkarni @ 2020-02-12 6:52 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, Joseph Myers, gcc Patches 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 > > > > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] [c-family] PR92867 - Add returns_arg attribute 2020-02-12 6:52 ` Prathamesh Kulkarni @ 2020-02-12 8:35 ` Richard Biener 0 siblings, 0 replies; 17+ messages in thread From: Richard Biener @ 2020-02-12 8:35 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: Jakub Jelinek, Joseph Myers, gcc Patches 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 > > > > > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-02-12 8:35 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-20 12:21 [RFC] [c-family] PR92867 - Add returns_arg attribute Prathamesh Kulkarni 2020-01-20 23:23 ` Joseph Myers 2020-01-24 12:22 ` Prathamesh Kulkarni 2020-01-25 2:40 ` Joseph Myers 2020-01-27 12:46 ` Richard Biener 2020-01-28 11:47 ` Prathamesh Kulkarni 2020-01-28 11:53 ` Jakub Jelinek 2020-01-28 12:02 ` Prathamesh Kulkarni 2020-01-28 12:25 ` Jakub Jelinek 2020-01-29 10:06 ` Richard Biener 2020-01-30 12:16 ` Prathamesh Kulkarni 2020-01-30 14:10 ` Richard Biener 2020-02-03 9:12 ` Prathamesh Kulkarni 2020-02-03 9:27 ` Prathamesh Kulkarni 2020-02-04 9:24 ` Prathamesh Kulkarni 2020-02-12 6:52 ` Prathamesh Kulkarni 2020-02-12 8:35 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).