On Mon, 3 Feb 2020 at 14:56, Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> wrote: > > On Mon, 3 Feb 2020 at 14:41, Prathamesh Kulkarni > <prathamesh.kulka...@linaro.org> wrote: > > > > On Thu, 30 Jan 2020 at 19:17, Richard Biener <richard.guent...@gmail.com> > > wrote: > > > > > > On Thu, Jan 30, 2020 at 11:49 AM Prathamesh Kulkarni > > > <prathamesh.kulka...@linaro.org> wrote: > > > > > > > > On Wed, 29 Jan 2020 at 14:38, Richard Biener > > > > <richard.guent...@gmail.com> wrote: > > > > > > > > > > On Tue, Jan 28, 2020 at 1:02 PM Jakub Jelinek <ja...@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 <ja...@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 > > > > > >
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) /*---------------------------------------------------------------------------