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 ?
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..56e54511300 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_RETURNS_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)
/*---------------------------------------------------------------------------
