avoid passing expressions to decl_attributes [PR94346]

Message ID d25aa2ea-474e-f8d2-94e2-b86003de6861@gmail.com
State New
Headers show
Series
  • avoid passing expressions to decl_attributes [PR94346]
Related show

Commit Message

Feng Xue OS via Gcc-patches March 27, 2020, 1:55 a.m.
Attribute copy can be invoked with an expression argument to copy from
the expression's type.  However, it must avoid passing the expression
as the last (optional) argument to decl_attributes because the function
is only prepared to deal with DECLs and types.

The attached patch passes null as the last argument in these situations
instead.  In addition, it makes more robust the handling of expressions
of pointers to function type (a subtle bug here was exposed by testing
of additional cases beyond the one in the report).

Tested on x86_64-linux.  I plan to commit the patch in the next day or
so unless there are concerns/suggestions.

Martin

Comments

Feng Xue OS via Gcc-patches March 27, 2020, 9:55 a.m. | #1
On Thu, Mar 26, 2020 at 07:55:39PM -0600, Martin Sebor via Gcc-patches wrote:
> --- a/gcc/c-family/c-attribs.c

> +++ b/gcc/c-family/c-attribs.c

> @@ -2526,17 +2526,21 @@ handle_copy_attribute (tree *node, tree name, tree args,

>        && !FUNCTION_POINTER_TYPE_P (TREE_TYPE (ref)))

>      ref = TREE_TYPE (ref);

>  

> +  tree reftype = DECL_P (ref) || EXPR_P (ref) ? TREE_TYPE (ref) : ref;


Perhaps
  tree reftype = TYPE_P (ref) ? ref : TREE_TYPE (ref);
would be shorter and easier to read?

>    if (DECL_P (decl))

>      {

>        if ((VAR_P (decl)

>  	   && (TREE_CODE (ref) == FUNCTION_DECL

>  	       || (EXPR_P (ref)

> -		   && POINTER_TYPE_P (TREE_TYPE (ref))

> -		   && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (TREE_TYPE (ref))))))

> +		   && POINTER_TYPE_P (reftype)

> +		   && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (reftype)))))

>  	  || (TREE_CODE (decl) == FUNCTION_DECL

>  	      && (VAR_P (ref)

>  		  || (EXPR_P (ref)

> -		      && !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (ref))))))

> +		      && !FUNC_OR_METHOD_TYPE_P (reftype)

> +		      && (!POINTER_TYPE_P (reftype)

> +			  || !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (reftype)))))))

>  	{

>  	  /* It makes no sense to try to copy function attributes

>  	     to a variable, or variable attributes to a function.  */

> @@ -2606,15 +2610,11 @@ handle_copy_attribute (tree *node, tree name, tree args,

>  

>    /* Similarly, a function declared with attribute noreturn has it

>       attached on to it, but a C11 _Noreturn function does not.  */

> -  tree reftype = ref;

>    if (DECL_P (ref)

>        && TREE_THIS_VOLATILE (ref)

> -      && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (reftype)))

> +      && FUNC_OR_METHOD_TYPE_P (reftype))

>      TREE_THIS_VOLATILE (decl) = true;

>  

> -  if (DECL_P (ref) || EXPR_P (ref))

> -    reftype = TREE_TYPE (ref);

> -

>    if (POINTER_TYPE_P (reftype))

>      reftype = TREE_TYPE (reftype);

> 


The above changes are certainly a good cleanup.
 
> @@ -2623,9 +2623,10 @@ handle_copy_attribute (tree *node, tree name, tree args,

>  

>    tree attrs = TYPE_ATTRIBUTES (reftype);

>  

> -  /* Copy type attributes from REF to DECL.  */

> +  /* Copy type attributes from REF to DECL.  Pass in REF if it's a DECL

> +     or a type but not if it's an expression.  */

>    for (tree at = attrs; at; at = TREE_CHAIN (at))

> -    decl_attributes (node, at, flags, ref);

> +    decl_attributes (node, at, flags, EXPR_P (ref) ? NULL_TREE : ref);

>  

>    return NULL_TREE;

>  }


This one really depends on what exactly the copy attribute should do.
The current state where it tries to determine a decl from which to copy
attributes is something that is IMHO very hard to be documented:

  /* Consider address-of expressions in the attribute argument
     as requests to copy from the referenced entity.  */
  if (TREE_CODE (ref) == ADDR_EXPR)
    ref = TREE_OPERAND (ref, 0);

  do
    {
      /* Drill down into references to find the referenced decl.  */
      tree_code refcode = TREE_CODE (ref);
      if (refcode == ARRAY_REF
          || refcode == INDIRECT_REF)
        ref = TREE_OPERAND (ref, 0);
      else if (refcode == COMPONENT_REF)
        ref = TREE_OPERAND (ref, 1);
      else
        break;
    } while (!DECL_P (ref));

but my point in the PR was that if you do it this way, COMPOUND_EXPRs should
be taken into account as well.
Because, right now you e.g. diagnose
  copy (1)
but don't diagnose
  copy (bar (), 1)
which appart from some side-effects first is the same thing.
Similarly, if you have
  copy (&decl)
it will behave differently from
  copy (bar (), &decl)
or
  copy (&whatever.field)
from
  copy (bar (), &whatever.field)
Yes, the comma expression isn't lvalue in C (but is in C++), but it doesn't
seem the current code would differentiate between what is an lvalue or
rvalue.  But it is something that can come either from the user, or e.g.
from sanitizers if they need to evaluate something before evaluating some
expression, or from slight differentiations on how we deal with the
COMPOUND_EXPRs elsewhere in the FE (see the PR94339 change).
From the user POV, it would be better if there was separate syntax in which
case to extract the attributes from the type vs. when extract it from
much more limited set of expressions (basically, only decls or
COMPONENT_REFs) and error on anything else.

	Jakub
Feng Xue OS via Gcc-patches March 27, 2020, 8:41 p.m. | #2
On 3/27/20 3:55 AM, Jakub Jelinek wrote:
> On Thu, Mar 26, 2020 at 07:55:39PM -0600, Martin Sebor via Gcc-patches wrote:

>> --- a/gcc/c-family/c-attribs.c

>> +++ b/gcc/c-family/c-attribs.c

>> @@ -2526,17 +2526,21 @@ handle_copy_attribute (tree *node, tree name, tree args,

>>         && !FUNCTION_POINTER_TYPE_P (TREE_TYPE (ref)))

>>       ref = TREE_TYPE (ref);

>>   

>> +  tree reftype = DECL_P (ref) || EXPR_P (ref) ? TREE_TYPE (ref) : ref;

> 

> Perhaps

>    tree reftype = TYPE_P (ref) ? ref : TREE_TYPE (ref);

> would be shorter and easier to read?


Sure.

> 

>>     if (DECL_P (decl))

>>       {

>>         if ((VAR_P (decl)

>>   	   && (TREE_CODE (ref) == FUNCTION_DECL

>>   	       || (EXPR_P (ref)

>> -		   && POINTER_TYPE_P (TREE_TYPE (ref))

>> -		   && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (TREE_TYPE (ref))))))

>> +		   && POINTER_TYPE_P (reftype)

>> +		   && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (reftype)))))

>>   	  || (TREE_CODE (decl) == FUNCTION_DECL

>>   	      && (VAR_P (ref)

>>   		  || (EXPR_P (ref)

>> -		      && !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (ref))))))

>> +		      && !FUNC_OR_METHOD_TYPE_P (reftype)

>> +		      && (!POINTER_TYPE_P (reftype)

>> +			  || !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (reftype)))))))

>>   	{

>>   	  /* It makes no sense to try to copy function attributes

>>   	     to a variable, or variable attributes to a function.  */

>> @@ -2606,15 +2610,11 @@ handle_copy_attribute (tree *node, tree name, tree args,

>>   

>>     /* Similarly, a function declared with attribute noreturn has it

>>        attached on to it, but a C11 _Noreturn function does not.  */

>> -  tree reftype = ref;

>>     if (DECL_P (ref)

>>         && TREE_THIS_VOLATILE (ref)

>> -      && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (reftype)))

>> +      && FUNC_OR_METHOD_TYPE_P (reftype))

>>       TREE_THIS_VOLATILE (decl) = true;

>>   

>> -  if (DECL_P (ref) || EXPR_P (ref))

>> -    reftype = TREE_TYPE (ref);

>> -

>>     if (POINTER_TYPE_P (reftype))

>>       reftype = TREE_TYPE (reftype);

>>

> 

> The above changes are certainly a good cleanup.

>   

>> @@ -2623,9 +2623,10 @@ handle_copy_attribute (tree *node, tree name, tree args,

>>   

>>     tree attrs = TYPE_ATTRIBUTES (reftype);

>>   

>> -  /* Copy type attributes from REF to DECL.  */

>> +  /* Copy type attributes from REF to DECL.  Pass in REF if it's a DECL

>> +     or a type but not if it's an expression.  */

>>     for (tree at = attrs; at; at = TREE_CHAIN (at))

>> -    decl_attributes (node, at, flags, ref);

>> +    decl_attributes (node, at, flags, EXPR_P (ref) ? NULL_TREE : ref);

>>   

>>     return NULL_TREE;

>>   }

> 

> This one really depends on what exactly the copy attribute should do.


For most expressions it's meant to copy the attributes from their type.
The reason why expressions are accepted as arguments is to avoid having
to declare things only to copy from types.

> The current state where it tries to determine a decl from which to copy

> attributes is something that is IMHO very hard to be documented:

 >

>    /* Consider address-of expressions in the attribute argument

>       as requests to copy from the referenced entity.  */

>    if (TREE_CODE (ref) == ADDR_EXPR)

>      ref = TREE_OPERAND (ref, 0);

> 

>    do

>      {

>        /* Drill down into references to find the referenced decl.  */

>        tree_code refcode = TREE_CODE (ref);

>        if (refcode == ARRAY_REF

>            || refcode == INDIRECT_REF)

>          ref = TREE_OPERAND (ref, 0);

>        else if (refcode == COMPONENT_REF)

>          ref = TREE_OPERAND (ref, 1);

>        else

>          break;

>      } while (!DECL_P (ref));

> 

> but my point in the PR was that if you do it this way, COMPOUND_EXPRs should

> be taken into account as well.


A COMPOUND_EXPR is an oddball corner case I used in the tests only to
stress the implementation.

In contrast, I expected ARRAY_REF and especially COMPONENT_REF and
INDIRECT_REF to be far more likely to come up because they support
use cases that aren't possible otherwise.  For example, given:

   struct A {
     ...
     __attribute__ ((aligned (A) ... )) T a;
   };

a natural thing to want to do is to define another struct with a member
that has the same properties as a, including alignment.  The only way
to make that possible is by having attribute copy accept expressions
so that it can be used like this:

   struct B {
     __attribute__ ((copy (((struct A*)0)->a)) T a1;
   };

I can think of no similar use case for the comma expression.  At
the same time, I also can't think of any problems that extending
the special treatment to COMPOUND_EXPR would cause but I don't see
it as important.

> Because, right now you e.g. diagnose

>    copy (1)

> but don't diagnose

>    copy (bar (), 1)

> which appart from some side-effects first is the same thing.


I don't disagree that the handler could be more discerning in what
it accepts and what it diagnoses.  So could many other handlers.
I've been improving things here for a while and plan to continue
going forward.

> Similarly, if you have

>    copy (&decl)

> it will behave differently from

>    copy (bar (), &decl)

> or

>    copy (&whatever.field)

> from

>    copy (bar (), &whatever.field)

> Yes, the comma expression isn't lvalue in C (but is in C++), but it doesn't

> seem the current code would differentiate between what is an lvalue or

> rvalue.  But it is something that can come either from the user, or e.g.

> from sanitizers if they need to evaluate something before evaluating some

> expression, or from slight differentiations on how we deal with the

> COMPOUND_EXPRs elsewhere in the FE (see the PR94339 change).

>  From the user POV, it would be better if there was separate syntax in which

> case to extract the attributes from the type vs. when extract it from

> much more limited set of expressions (basically, only decls or

> COMPONENT_REFs) and error on anything else.


Maybe.  I don't see it that way but regardless, it's too late to
change.  The current interface was introduced in GCC 9 and changing
it could break code that relies on it.  Adding a new one, while
possible, doesn't seem necessary given the very limited audience
this is designed for.

If you think some of these misuses are especially important to diagnose
please open bugs for them.  I'll be looking into making improvements
in the area of attributes in stage 1 so I can see about improving this
one as well.

Until then, I have pushed just the fix for the ICE alone.

Martin

Patch

PR c++/94346 - [9/10 Regression] ICE due to handle_copy_attribute since r9-3982

gcc/c-family/ChangeLog:

	PR c++/94346
	* c-attribs.c (handle_copy_attribute): Avoid passing expressions
	to decl_attributes.  Make handling of different kinds of entities
	more robust.
	
gcc/c-c++-common/ChangeLog:

	PR c++/94346
	* c-c++-common/attr-copy.c: New test.

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index f30158a258b..93e740853a9 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -2526,17 +2526,21 @@  handle_copy_attribute (tree *node, tree name, tree args,
       && !FUNCTION_POINTER_TYPE_P (TREE_TYPE (ref)))
     ref = TREE_TYPE (ref);
 
+  tree reftype = DECL_P (ref) || EXPR_P (ref) ? TREE_TYPE (ref) : ref;
+
   if (DECL_P (decl))
     {
       if ((VAR_P (decl)
 	   && (TREE_CODE (ref) == FUNCTION_DECL
 	       || (EXPR_P (ref)
-		   && POINTER_TYPE_P (TREE_TYPE (ref))
-		   && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (TREE_TYPE (ref))))))
+		   && POINTER_TYPE_P (reftype)
+		   && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (reftype)))))
 	  || (TREE_CODE (decl) == FUNCTION_DECL
 	      && (VAR_P (ref)
 		  || (EXPR_P (ref)
-		      && !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (ref))))))
+		      && !FUNC_OR_METHOD_TYPE_P (reftype)
+		      && (!POINTER_TYPE_P (reftype)
+			  || !FUNC_OR_METHOD_TYPE_P (TREE_TYPE (reftype)))))))
 	{
 	  /* It makes no sense to try to copy function attributes
 	     to a variable, or variable attributes to a function.  */
@@ -2606,15 +2610,11 @@  handle_copy_attribute (tree *node, tree name, tree args,
 
   /* Similarly, a function declared with attribute noreturn has it
      attached on to it, but a C11 _Noreturn function does not.  */
-  tree reftype = ref;
   if (DECL_P (ref)
       && TREE_THIS_VOLATILE (ref)
-      && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (reftype)))
+      && FUNC_OR_METHOD_TYPE_P (reftype))
     TREE_THIS_VOLATILE (decl) = true;
 
-  if (DECL_P (ref) || EXPR_P (ref))
-    reftype = TREE_TYPE (ref);
-
   if (POINTER_TYPE_P (reftype))
     reftype = TREE_TYPE (reftype);
 
@@ -2623,9 +2623,10 @@  handle_copy_attribute (tree *node, tree name, tree args,
 
   tree attrs = TYPE_ATTRIBUTES (reftype);
 
-  /* Copy type attributes from REF to DECL.  */
+  /* Copy type attributes from REF to DECL.  Pass in REF if it's a DECL
+     or a type but not if it's an expression.  */
   for (tree at = attrs; at; at = TREE_CHAIN (at))
-    decl_attributes (node, at, flags, ref);
+    decl_attributes (node, at, flags, EXPR_P (ref) ? NULL_TREE : ref);
 
   return NULL_TREE;
 }
diff --git a/gcc/testsuite/c-c++-common/attr-copy.c b/gcc/testsuite/c-c++-common/attr-copy.c
new file mode 100644
index 00000000000..284088a8b97
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/attr-copy.c
@@ -0,0 +1,43 @@ 
+/* PR c++/94346 - ICE due to handle_copy_attribute
+   { dg-do compile }
+   { dg-options "-Wall" } */
+
+#define ATTR(...) __attribute__ ((__VA_ARGS__))
+
+#if __cplusplus > 199711L
+#  define SA(expr) static_assert (expr, #expr)
+#elif __cplusplus
+#  define SA(expr)							\
+  typedef __attribute__ ((unused)) char Assert[!(expr) ? -1 : 1]
+#else
+#  define SA(expr) _Static_assert (expr, #expr)
+#endif
+
+typedef struct ATTR (packed) A { ATTR (packed) unsigned bf: 1; } A;
+
+int bar (void);
+
+struct C
+{
+  char c;
+  ATTR (copy ((bar (), ((struct A *)(0))[0]))) int i;
+};
+
+/* Verify the attribute has been copied.  */
+SA (__builtin_offsetof (struct C, i) == 1);
+
+
+
+/* Verify attribute copy can copy from the type a comma expression.  */
+ATTR (alloc_size (1)) void* alloc1 (int);
+
+ATTR (copy ((bar (), alloc1))) void* alloc2 (int, int);
+
+ATTR (copy ((bar (), alloc1))) void alloc3 (int);  /* { dg-warning "'alloc_size' attribute ignored on a function returning 'void'" } */
+
+
+typedef ATTR (alloc_size (1)) void* F (int);
+
+ATTR (copy ((bar (), (F*)0))) void* alloc4 (int, int);
+
+ATTR (copy ((bar (), (F*)0))) void alloc5 (int, int);  /* { dg-warning "'alloc_size' attribute ignored on a function returning 'void'" } */