diff mbox series

[RFA] c-family: attribute ((aligned, mode)) [PR100545]

Message ID 20220427151957.795214-1-jason@redhat.com
State New
Headers show
Series [RFA] c-family: attribute ((aligned, mode)) [PR100545] | expand

Commit Message

Jason Merrill April 27, 2022, 3:19 p.m. UTC
The problem here was that handle_mode_attribute clobbered the changes of any
previous attribute, only copying type qualifiers to the new type.  And
common_handle_aligned_attribute had previously set up the typedef, so when
we later called set_underlying_type it saw DECL_ORIGINAL_TYPE set and just
returned, even though handle_mode_attribute had messed up the TREE_TYPE.
So, let's fix handle_mode_attribute to copy attributes, alignment, and
typedefness to the new type.

Tested x86_64-pc-linux-gnu, OK for trunk now or after 12.1?

	PR c/100545

gcc/c-family/ChangeLog:

	* c-attribs.cc (handle_mode_attribute): Copy attributes, aligned,
	and typedef.
	* c-common.cc (set_underlying_type): Add assert.

gcc/testsuite/ChangeLog:

	* c-c++-common/attr-mode-1.c: New test.
---
 gcc/c-family/c-attribs.cc                | 15 ++++++++++++++-
 gcc/c-family/c-common.cc                 |  7 ++++---
 gcc/testsuite/c-c++-common/attr-mode-1.c |  3 +++
 3 files changed, 21 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/attr-mode-1.c


base-commit: 6a460a2007dd9c527c5f9d5bbbedb852db7c1373

Comments

Marek Polacek April 27, 2022, 4:58 p.m. UTC | #1
On Wed, Apr 27, 2022 at 11:19:57AM -0400, Jason Merrill via Gcc-patches wrote:
> The problem here was that handle_mode_attribute clobbered the changes of any
> previous attribute, only copying type qualifiers to the new type.  And
> common_handle_aligned_attribute had previously set up the typedef, so when
> we later called set_underlying_type it saw DECL_ORIGINAL_TYPE set and just
> returned, even though handle_mode_attribute had messed up the TREE_TYPE.
> So, let's fix handle_mode_attribute to copy attributes, alignment, and
> typedefness to the new type.
> 
> Tested x86_64-pc-linux-gnu, OK for trunk now or after 12.1?

I think I'd slightly prefer 12.1, it doesn't seem to be a regression.
 
> 	PR c/100545
> 
> gcc/c-family/ChangeLog:
> 
> 	* c-attribs.cc (handle_mode_attribute): Copy attributes, aligned,
> 	and typedef.
> 	* c-common.cc (set_underlying_type): Add assert.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* c-c++-common/attr-mode-1.c: New test.
> ---
>  gcc/c-family/c-attribs.cc                | 15 ++++++++++++++-
>  gcc/c-family/c-common.cc                 |  7 ++++---
>  gcc/testsuite/c-c++-common/attr-mode-1.c |  3 +++
>  3 files changed, 21 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/c-c++-common/attr-mode-1.c
> 
> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> index 111a33f405a..26876d0f309 100644
> --- a/gcc/c-family/c-attribs.cc
> +++ b/gcc/c-family/c-attribs.cc
> @@ -2199,7 +2199,20 @@ handle_mode_attribute (tree *node, tree name, tree args,
>  	  return NULL_TREE;
>  	}
>  
> -      *node = build_qualified_type (typefm, TYPE_QUALS (type));
> +      /* Copy any quals and attributes to the new type.  */
> +      *node = build_type_attribute_qual_variant (typefm, TYPE_ATTRIBUTES (type),
> +						 TYPE_QUALS (type));
> +      if (TYPE_USER_ALIGN (type))
> +	*node = build_aligned_type (*node, TYPE_ALIGN (type));
> +      if (typedef_variant_p (type))
> +	{
> +	  /* Set up the typedef all over again.  */
> +	  tree decl = TYPE_NAME (type);
> +	  DECL_ORIGINAL_TYPE (decl) = NULL_TREE;
> +	  TREE_TYPE (decl) = *node;
> +	  set_underlying_type (decl);
> +	  *node = TREE_TYPE (decl);
> +	}
>      }
>  
>    return NULL_TREE;
> diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
> index bb0544eeaea..730faa9e87f 100644
> --- a/gcc/c-family/c-common.cc
> +++ b/gcc/c-family/c-common.cc
> @@ -8153,15 +8153,16 @@ check_missing_format_attribute (tree ltype, tree rtype)
>  void
>  set_underlying_type (tree x)
>  {
> -  if (x == error_mark_node)
> +  if (x == error_mark_node || TREE_TYPE (x) == error_mark_node)

Maybe error_operand_p?

>      return;
>    if (DECL_IS_UNDECLARED_BUILTIN (x) && TREE_CODE (TREE_TYPE (x)) != ARRAY_TYPE)
>      {
>        if (TYPE_NAME (TREE_TYPE (x)) == 0)
>  	TYPE_NAME (TREE_TYPE (x)) = x;
>      }
> -  else if (TREE_TYPE (x) != error_mark_node
> -	   && DECL_ORIGINAL_TYPE (x) == NULL_TREE)
> +  else if (DECL_ORIGINAL_TYPE (x))
> +    gcc_checking_assert (TYPE_NAME (TREE_TYPE (x)) == x);
> +  else
>      {
>        tree tt = TREE_TYPE (x);
>        DECL_ORIGINAL_TYPE (x) = tt;
> diff --git a/gcc/testsuite/c-c++-common/attr-mode-1.c b/gcc/testsuite/c-c++-common/attr-mode-1.c
> new file mode 100644
> index 00000000000..59b20cd99e8
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/attr-mode-1.c
> @@ -0,0 +1,3 @@
> +// PR c/100545
> +
> +typedef int fatp_t __attribute__((aligned, mode(SI)));

I think you need to add "dg-options -g", otherwise the bug doesn't
manifest.

Marek
Joseph Myers April 27, 2022, 5:02 p.m. UTC | #2
On Wed, 27 Apr 2022, Jason Merrill via Gcc-patches wrote:

> +      if (typedef_variant_p (type))
> +	{
> +	  /* Set up the typedef all over again.  */

This seems wrong when the typedef is just being used in another 
declaration with the mode attribute, as opposed to being defined using the 
mode attribute.  E.g. the following test is valid and accepted before the 
patch, but wrongly rejected after the patch because the typedef has had 
its type changed.

typedef int I;
int x;
I y __attribute__ ((mode(QI)));
extern I x;
Jason Merrill April 27, 2022, 11:48 p.m. UTC | #3
On 4/27/22 13:02, Joseph Myers wrote:
> On Wed, 27 Apr 2022, Jason Merrill via Gcc-patches wrote:
> 
>> +      if (typedef_variant_p (type))
>> +	{
>> +	  /* Set up the typedef all over again.  */
> 
> This seems wrong when the typedef is just being used in another
> declaration with the mode attribute, as opposed to being defined using the
> mode attribute.  E.g. the following test is valid and accepted before the
> patch, but wrongly rejected after the patch because the typedef has had
> its type changed.
> 
> typedef int I;
> int x;
> I y __attribute__ ((mode(QI)));
> extern I x;

Ah, good point.  Fixed thus:
Jason Merrill April 29, 2022, 7:20 p.m. UTC | #4
On 4/27/22 19:48, Jason Merrill wrote:
> On 4/27/22 13:02, Joseph Myers wrote:
>> On Wed, 27 Apr 2022, Jason Merrill via Gcc-patches wrote:
>>
>>> +      if (typedef_variant_p (type))
>>> +    {
>>> +      /* Set up the typedef all over again.  */
>>
>> This seems wrong when the typedef is just being used in another
>> declaration with the mode attribute, as opposed to being defined using 
>> the
>> mode attribute.  E.g. the following test is valid and accepted before the
>> patch, but wrongly rejected after the patch because the typedef has had
>> its type changed.
>>
>> typedef int I;
>> int x;
>> I y __attribute__ ((mode(QI)));
>> extern I x;
> 
> Ah, good point.  Fixed thus:

Marek pointed out elsewhere that the first testcase should have

// { dg-additional-options -g }

OK for 13 with that change?
Joseph Myers April 29, 2022, 7:46 p.m. UTC | #5
On Fri, 29 Apr 2022, Jason Merrill via Gcc-patches wrote:

> Marek pointed out elsewhere that the first testcase should have
> 
> // { dg-additional-options -g }
> 
> OK for 13 with that change?

OK.
diff mbox series

Patch

diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
index 111a33f405a..26876d0f309 100644
--- a/gcc/c-family/c-attribs.cc
+++ b/gcc/c-family/c-attribs.cc
@@ -2199,7 +2199,20 @@  handle_mode_attribute (tree *node, tree name, tree args,
 	  return NULL_TREE;
 	}
 
-      *node = build_qualified_type (typefm, TYPE_QUALS (type));
+      /* Copy any quals and attributes to the new type.  */
+      *node = build_type_attribute_qual_variant (typefm, TYPE_ATTRIBUTES (type),
+						 TYPE_QUALS (type));
+      if (TYPE_USER_ALIGN (type))
+	*node = build_aligned_type (*node, TYPE_ALIGN (type));
+      if (typedef_variant_p (type))
+	{
+	  /* Set up the typedef all over again.  */
+	  tree decl = TYPE_NAME (type);
+	  DECL_ORIGINAL_TYPE (decl) = NULL_TREE;
+	  TREE_TYPE (decl) = *node;
+	  set_underlying_type (decl);
+	  *node = TREE_TYPE (decl);
+	}
     }
 
   return NULL_TREE;
diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
index bb0544eeaea..730faa9e87f 100644
--- a/gcc/c-family/c-common.cc
+++ b/gcc/c-family/c-common.cc
@@ -8153,15 +8153,16 @@  check_missing_format_attribute (tree ltype, tree rtype)
 void
 set_underlying_type (tree x)
 {
-  if (x == error_mark_node)
+  if (x == error_mark_node || TREE_TYPE (x) == error_mark_node)
     return;
   if (DECL_IS_UNDECLARED_BUILTIN (x) && TREE_CODE (TREE_TYPE (x)) != ARRAY_TYPE)
     {
       if (TYPE_NAME (TREE_TYPE (x)) == 0)
 	TYPE_NAME (TREE_TYPE (x)) = x;
     }
-  else if (TREE_TYPE (x) != error_mark_node
-	   && DECL_ORIGINAL_TYPE (x) == NULL_TREE)
+  else if (DECL_ORIGINAL_TYPE (x))
+    gcc_checking_assert (TYPE_NAME (TREE_TYPE (x)) == x);
+  else
     {
       tree tt = TREE_TYPE (x);
       DECL_ORIGINAL_TYPE (x) = tt;
diff --git a/gcc/testsuite/c-c++-common/attr-mode-1.c b/gcc/testsuite/c-c++-common/attr-mode-1.c
new file mode 100644
index 00000000000..59b20cd99e8
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/attr-mode-1.c
@@ -0,0 +1,3 @@ 
+// PR c/100545
+
+typedef int fatp_t __attribute__((aligned, mode(SI)));