diff mbox

[C] Make attributes accept enum values (PR c/50459)

Message ID 20140414151045.GA20332@redhat.com
State New
Headers show

Commit Message

Marek Polacek April 14, 2014, 3:10 p.m. UTC 
Currently it is not possible to use an enum value (or const int in
C++) as an attribute argument (where compiler wants INTEGER_CST).  This
is because the values are represented by CONST_DECL nodes -- and
we don't deal with them.  I think it's viable to accept them too,
and that is what this patch does along with some clean ups.

Regtested/bootstrapped on x86_64-linux, ok for trunk?

2014-04-14  Marek Polacek  <polacek@redhat.com>

	PR c/50459
c-family/
	* c-common.c (get_attrib_value): New function.
	(get_priority): Call get_attrib_value.
	(check_user_alignment): Likewise.
	(handle_alloc_size_attribute): Likewise.
	(handle_alloc_align_attribute): Likewise.
	(handle_assume_aligned_attribute): Likewise.
	(handle_vector_size_attribute): Likewise.
	(handle_sentinel_attribute): Likewise.
testsuite/
	* c-c++-common/pr50459.c: New test.
	* g++.dg/ext/pr50459.C: New test.


	Marek

Comments

Marc Glisse April 14, 2014, 3:26 p.m. UTC | #1 
On Mon, 14 Apr 2014, Marek Polacek wrote:

> Currently it is not possible to use an enum value (or const int in
> C++) as an attribute argument (where compiler wants INTEGER_CST).  This
> is because the values are represented by CONST_DECL nodes -- and
> we don't deal with them.  I think it's viable to accept them too,
> and that is what this patch does along with some clean ups.

Thanks.

> +/* Perform default promotions or extract the integer constant from
> +   an enum value.  This function is used for getting a value from
> +   an attribute argument.  */
> +
> +static tree
> +get_attrib_value (tree val)
> +{
> +  if (val
> +      && TREE_CODE (val) != IDENTIFIER_NODE
> +      && TREE_CODE (val) != FUNCTION_DECL)
> +    val = default_conversion (val);
> +  else if (TREE_CODE (val) == IDENTIFIER_NODE)
> +    {
> +      tree t = lookup_name (val);
> +      if (t && TREE_CODE (t) == CONST_DECL)
> +	return DECL_INITIAL (t);
> +    }
> +  return val;
> +}

If val is NULL, it seems you still end up looking at its TREE_CODE?
(don't know if that can happen)

In C++, default_conversion probably handles IDENTIFIER_NODE just fine, but 
having different code for the 2 languages would not be nice, right?
Jason Merrill April 15, 2014, 1:59 p.m. UTC | #2 
On 04/14/2014 11:10 AM, Marek Polacek wrote:
> +  else if (TREE_CODE (val) == IDENTIFIER_NODE)
> +    {
> +      tree t = lookup_name (val);
> +      if (t && TREE_CODE (t) == CONST_DECL)
> +	return DECL_INITIAL (t);
> +    }

I'm uncomfortable with this; we should have looked up any attributes in 
the parser.  Does the testsuite hit this code?

Jason
Marek Polacek April 15, 2014, 7:56 p.m. UTC | #3 
On Tue, Apr 15, 2014 at 09:59:10AM -0400, Jason Merrill wrote:
> On 04/14/2014 11:10 AM, Marek Polacek wrote:
> >+  else if (TREE_CODE (val) == IDENTIFIER_NODE)
> >+    {
> >+      tree t = lookup_name (val);
> >+      if (t && TREE_CODE (t) == CONST_DECL)
> >+	return DECL_INITIAL (t);
> >+    }
> 
> I'm uncomfortable with this; we should have looked up any attributes
> in the parser.  Does the testsuite hit this code?

Thanks for looking at it.

So the newer version of the patch contains:

+      else if (TREE_CODE (val) == IDENTIFIER_NODE)
+	{
+	  tree t = lookup_name (val);
+	  if (t && TREE_CODE (t) == CONST_DECL)
+	    val = default_conversion (t);
+	}

The testsuite doesn't hit this code with C++, but does hit this code
with C.  The thing is, if we have e.g.
enum { A = 128 };
void *fn1 (void) __attribute__((assume_aligned (A)));
then handle_assume_aligned_attribute walks the attribute arguments
and gets the argument via TREE_VALUE.  If this argument is an enum
value, then for C the argument is identifier_node that contains const_decl,
but for C++ the argument is directly const_decl.  That means for C++ in
get_attrib_value we just call default_conversion as before, but for C we
call lookup_name firstly.
Does this answer your question?

	Marek
Jason Merrill April 16, 2014, 5:40 p.m. UTC | #4 
On 04/15/2014 03:56 PM, Marek Polacek wrote:
> The testsuite doesn't hit this code with C++, but does hit this code
> with C.  The thing is, if we have e.g.
> enum { A = 128 };
> void *fn1 (void) __attribute__((assume_aligned (A)));
> then handle_assume_aligned_attribute walks the attribute arguments
> and gets the argument via TREE_VALUE.  If this argument is an enum
> value, then for C the argument is identifier_node that contains const_decl,

Ah.  Then I think the C parser should be fixed to check 
attribute_takes_identifier_p and look up the argument if false.

Jason
diff mbox

Patch

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index 1d56bc0..3c66d49 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -6448,6 +6448,26 @@  attribute_takes_identifier_p (const_tree attr_id)
     return targetm.attribute_takes_identifier_p (attr_id);
 }
 
+/* Perform default promotions or extract the integer constant from
+   an enum value.  This function is used for getting a value from
+   an attribute argument.  */
+
+static tree
+get_attrib_value (tree val)
+{
+  if (val
+      && TREE_CODE (val) != IDENTIFIER_NODE
+      && TREE_CODE (val) != FUNCTION_DECL)
+    val = default_conversion (val);
+  else if (TREE_CODE (val) == IDENTIFIER_NODE)
+    {
+      tree t = lookup_name (val);
+      if (t && TREE_CODE (t) == CONST_DECL)
+	return DECL_INITIAL (t);
+    }
+  return val;
+}
+
 /* Attribute handlers common to C front ends.  */
 
 /* Handle a "packed" attribute; arguments as in
@@ -7030,11 +7050,10 @@  get_priority (tree args, bool is_destructor)
     }
 
   arg = TREE_VALUE (args);
-  if (TREE_CODE (arg) == IDENTIFIER_NODE)
-    goto invalid;
   if (arg == error_mark_node)
     return DEFAULT_INIT_PRIORITY;
-  arg = default_conversion (arg);
+  arg = get_attrib_value (arg);
+
   if (!tree_fits_shwi_p (arg)
       || !INTEGRAL_TYPE_P (TREE_TYPE (arg)))
     goto invalid;
@@ -7418,6 +7437,8 @@  check_user_alignment (const_tree align, bool allow_zero)
 {
   int i;
 
+  align = get_attrib_value (CONST_CAST_TREE (align));
+
   if (TREE_CODE (align) != INTEGER_CST
       || !INTEGRAL_TYPE_P (TREE_TYPE (align)))
     {
@@ -8037,11 +8058,7 @@  handle_alloc_size_attribute (tree *node, tree ARG_UNUSED (name), tree args,
   unsigned arg_count = type_num_arguments (*node);
   for (; args; args = TREE_CHAIN (args))
     {
-      tree position = TREE_VALUE (args);
-      if (position && TREE_CODE (position) != IDENTIFIER_NODE
-	  && TREE_CODE (position) != FUNCTION_DECL)
-	position = default_conversion (position);
-
+      tree position = get_attrib_value (TREE_VALUE (args));
       if (!tree_fits_uhwi_p (position)
 	  || !arg_count
 	  || !IN_RANGE (tree_to_uhwi (position), 1, arg_count))
@@ -8063,10 +8080,7 @@  handle_alloc_align_attribute (tree *node, tree, tree args, int,
 			      bool *no_add_attrs)
 {
   unsigned arg_count = type_num_arguments (*node);
-  tree position = TREE_VALUE (args);
-  if (position && TREE_CODE (position) != IDENTIFIER_NODE)
-    position = default_conversion (position);
-
+  tree position = get_attrib_value (TREE_VALUE (args));
   if (!tree_fits_uhwi_p (position)
       || !arg_count
       || !IN_RANGE (tree_to_uhwi (position), 1, arg_count))
@@ -8088,11 +8102,7 @@  handle_assume_aligned_attribute (tree *, tree, tree args, int,
 {
   for (; args; args = TREE_CHAIN (args))
     {
-      tree position = TREE_VALUE (args);
-      if (position && TREE_CODE (position) != IDENTIFIER_NODE
-	  && TREE_CODE (position) != FUNCTION_DECL)
-	position = default_conversion (position);
-
+      tree position = get_attrib_value (TREE_VALUE (args));
       if (TREE_CODE (position) != INTEGER_CST)
 	{
 	  warning (OPT_Wattributes,
@@ -8532,10 +8542,7 @@  handle_vector_size_attribute (tree *node, tree name, tree args,
 
   *no_add_attrs = true;
 
-  size = TREE_VALUE (args);
-  if (size && TREE_CODE (size) != IDENTIFIER_NODE)
-    size = default_conversion (size);
-
+  size = get_attrib_value (TREE_VALUE (args));
   if (!tree_fits_uhwi_p (size))
     {
       warning (OPT_Wattributes, "%qE attribute ignored", name);
@@ -8943,8 +8950,7 @@  handle_sentinel_attribute (tree *node, tree name, tree args,
 
   if (args)
     {
-      tree position = TREE_VALUE (args);
-
+      tree position = get_attrib_value (TREE_VALUE (args));
       if (TREE_CODE (position) != INTEGER_CST)
 	{
 	  warning (OPT_Wattributes,
diff --git gcc/testsuite/c-c++-common/pr50459.c gcc/testsuite/c-c++-common/pr50459.c
index e69de29..f837b63 100644
--- gcc/testsuite/c-c++-common/pr50459.c
+++ gcc/testsuite/c-c++-common/pr50459.c
@@ -0,0 +1,14 @@ 
+/* PR c/50459 */
+/* { dg-do compile } */
+/* { dg-options "-Wall -Wextra" } */
+
+enum { A = 128, B = 1 };
+void *fn1 (void) __attribute__((assume_aligned (A)));
+void *fn2 (void) __attribute__((assume_aligned (A, 4)));
+void fn3 (void) __attribute__((constructor (A)));
+void fn4 (void) __attribute__((destructor (A)));
+void *fn5 (int) __attribute__((alloc_size (B)));
+void *fn6 (int) __attribute__((alloc_align (B)));
+void fn7 (const char *, ...) __attribute__ ((sentinel (B)));
+int __attribute__((vector_size (A))) a;
+int __attribute__((aligned (A))) foo;
diff --git gcc/testsuite/g++.dg/ext/pr50459.C gcc/testsuite/g++.dg/ext/pr50459.C
index e69de29..8acc299 100644
--- gcc/testsuite/g++.dg/ext/pr50459.C
+++ gcc/testsuite/g++.dg/ext/pr50459.C
@@ -0,0 +1,14 @@ 
+// PR c/50459
+// { dg-do compile }
+// { dg-options "-Wall -Wextra" }
+
+const int A = 128, B = 1;
+void *fn1 (void) __attribute__((assume_aligned (A)));
+void *fn2 (void) __attribute__((assume_aligned (A, 4)));
+void fn3 (void) __attribute__((constructor (A)));
+void fn4 (void) __attribute__((destructor (A)));
+void *fn5 (int) __attribute__((alloc_size (B)));
+void *fn6 (int) __attribute__((alloc_align (B)));
+void fn7 (const char *, ...) __attribute__ ((sentinel (B)));
+int __attribute__((vector_size (A))) a;
+int __attribute__((aligned (A))) foo;