diff mbox series

avoid modifying type in place (PR 97206)

Message ID 62c2168a-86c7-01aa-0691-e706c9368562@gmail.com
State New
Headers show
Series avoid modifying type in place (PR 97206) | expand

Commit Message

Martin Sebor Sept. 29, 2020, 9:40 p.m. UTC
To render the bounds as well as the static specifier in array
and VLA function parameters the  new -Warray-parameter and
-Wvla-parameter warning builds a "synthetic" array type that
corresponds to the form of the parameter, sets its qualifiers
to match those of the pointer, and passes it to the prett-
printer for formatting.  The in-place modification of the type
causes problems when the type is subsequently [re]used for one
that doesn't have the same qualifiers set.  The attached fix
replaces the in-place modification with a call to 
build_type_attribute_qual_variant.

Tested on x86_64-linux.

I will commit this patch later this week unless I hear concerns
or suggestions for changes.

Martin

Comments

Jakub Jelinek Sept. 30, 2020, 9:57 a.m. UTC | #1
On Tue, Sep 29, 2020 at 03:40:40PM -0600, Martin Sebor via Gcc-patches wrote:
> I will commit this patch later this week unless I hear concerns
> or suggestions for changes.

That is not how the patch review process works.

> +	  arat = tree_cons (get_identifier ("array"), flag, NULL_TREE);

Better
	  arat = build_tree_list (get_identifier ("array"), flag);
then, tree_cons is when you have a meaningful TREE_CHAIN you want to supply
too.
>  	}
>  
> -      TYPE_ATOMIC (artype) = TYPE_ATOMIC (type);
> -      TYPE_READONLY (artype) = TYPE_READONLY (type);
> -      TYPE_RESTRICT (artype) = TYPE_RESTRICT (type);
> -      TYPE_VOLATILE (artype) = TYPE_VOLATILE (type);
> -      type = artype;
> +      const int quals = TYPE_QUALS (type);
> +      type = build_array_type (eltype, index_type);
> +      type = build_type_attribute_qual_variant (type, arat, quals);
>      }
>  
>    /* Format the type using the current pretty printer.  The generic tree
> @@ -2309,10 +2304,6 @@ attr_access::array_as_string (tree type) const
>    typstr = pp_formatted_text (pp);
>    delete pp;
>  
> -  if (this->str)
> -    /* Remove the attribute that wasn't installed by decl_attributes.  */
> -    TYPE_ATTRIBUTES (type) = NULL_TREE;
> -
>    return typstr;
>  }

Otherwise LGTM.

	Jakub
Martin Sebor Sept. 30, 2020, 7:20 p.m. UTC | #2
On 9/30/20 3:57 AM, Jakub Jelinek wrote:
> On Tue, Sep 29, 2020 at 03:40:40PM -0600, Martin Sebor via Gcc-patches wrote:
>> I will commit this patch later this week unless I hear concerns
>> or suggestions for changes.
> 
> That is not how the patch review process works.

The review process hasn't been working well for me, but thankfully,
the commit policy lets me make these types of "obvious" fixes on
my own, without waiting for approval.  But if I could get simple
changes reviewed in a few days instead of having to ping them for
weeks there would be no reason for me to take advantage of this
latitude (and for us to rehash this topic yet again).
>> +	  arat = tree_cons (get_identifier ("array"), flag, NULL_TREE);
> 
> Better
> 	  arat = build_tree_list (get_identifier ("array"), flag);
> then, tree_cons is when you have a meaningful TREE_CHAIN you want to supply
> too.

Okay.  I checked to make sure they both do the same thing and
create a tree with the size and committed the updated patch in
r11-3570.

Martin

>>   	}
>>   
>> -      TYPE_ATOMIC (artype) = TYPE_ATOMIC (type);
>> -      TYPE_READONLY (artype) = TYPE_READONLY (type);
>> -      TYPE_RESTRICT (artype) = TYPE_RESTRICT (type);
>> -      TYPE_VOLATILE (artype) = TYPE_VOLATILE (type);
>> -      type = artype;
>> +      const int quals = TYPE_QUALS (type);
>> +      type = build_array_type (eltype, index_type);
>> +      type = build_type_attribute_qual_variant (type, arat, quals);
>>       }
>>   
>>     /* Format the type using the current pretty printer.  The generic tree
>> @@ -2309,10 +2304,6 @@ attr_access::array_as_string (tree type) const
>>     typstr = pp_formatted_text (pp);
>>     delete pp;
>>   
>> -  if (this->str)
>> -    /* Remove the attribute that wasn't installed by decl_attributes.  */
>> -    TYPE_ATTRIBUTES (type) = NULL_TREE;
>> -
>>     return typstr;
>>   }
> 
> Otherwise LGTM.
> 
> 	Jakub
>
Richard Biener Oct. 1, 2020, 6:32 a.m. UTC | #3
On Wed, Sep 30, 2020 at 9:20 PM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On 9/30/20 3:57 AM, Jakub Jelinek wrote:
> > On Tue, Sep 29, 2020 at 03:40:40PM -0600, Martin Sebor via Gcc-patches wrote:
> >> I will commit this patch later this week unless I hear concerns
> >> or suggestions for changes.
> >
> > That is not how the patch review process works.
>
> The review process hasn't been working well for me, but thankfully,
> the commit policy lets me make these types of "obvious" fixes on
> my own, without waiting for approval.

Guess it would help if you'd simply say

"I will commit this patch as obvious later this week."

which makes clear you view the change as obvious.

Richard.

>  But if I could get simple
> changes reviewed in a few days instead of having to ping them for
> weeks there would be no reason for me to take advantage of this
> latitude (and for us to rehash this topic yet again).
> >> +      arat = tree_cons (get_identifier ("array"), flag, NULL_TREE);
> >
> > Better
> >         arat = build_tree_list (get_identifier ("array"), flag);
> > then, tree_cons is when you have a meaningful TREE_CHAIN you want to supply
> > too.
>
> Okay.  I checked to make sure they both do the same thing and
> create a tree with the size and committed the updated patch in
> r11-3570.
>
> Martin
>
> >>      }
> >>
> >> -      TYPE_ATOMIC (artype) = TYPE_ATOMIC (type);
> >> -      TYPE_READONLY (artype) = TYPE_READONLY (type);
> >> -      TYPE_RESTRICT (artype) = TYPE_RESTRICT (type);
> >> -      TYPE_VOLATILE (artype) = TYPE_VOLATILE (type);
> >> -      type = artype;
> >> +      const int quals = TYPE_QUALS (type);
> >> +      type = build_array_type (eltype, index_type);
> >> +      type = build_type_attribute_qual_variant (type, arat, quals);
> >>       }
> >>
> >>     /* Format the type using the current pretty printer.  The generic tree
> >> @@ -2309,10 +2304,6 @@ attr_access::array_as_string (tree type) const
> >>     typstr = pp_formatted_text (pp);
> >>     delete pp;
> >>
> >> -  if (this->str)
> >> -    /* Remove the attribute that wasn't installed by decl_attributes.  */
> >> -    TYPE_ATTRIBUTES (type) = NULL_TREE;
> >> -
> >>     return typstr;
> >>   }
> >
> > Otherwise LGTM.
> >
> >       Jakub
> >
>
diff mbox series

Patch

PR c/97206 - ICE in composite_type on declarations of a similar array types

gcc/ChangeLog:

	PR c/97206
	* attribs.c (attr_access::array_as_string): Avoid modifying a shared
	type in place and use build_type_attribute_qual_variant instead.
gcc/testsuite/ChangeLog:

	PR c/97206
	* gcc.dg/Warray-parameter-7.c: New test.
	* gcc.dg/Warray-parameter-8.c: New test.
	* gcc.dg/Wvla-parameter-5.c: New test.

diff --git a/gcc/attribs.c b/gcc/attribs.c
index abc75368e6c..923e2e142bb 100644
--- a/gcc/attribs.c
+++ b/gcc/attribs.c
@@ -2256,15 +2256,14 @@  attr_access::array_as_string (tree type) const
 
   if (this->str)
     {
-      /* For array parameters (but not pointers) create an array type
-	 that corresponds to the form of the parameter including its
+      /* For array parameters (but not pointers) create a temporary array
+	 type that corresponds to the form of the parameter including its
 	 qualifiers even though they apply to the pointer, not the array
 	 type.  */
       const bool vla_p = minsize == HOST_WIDE_INT_M1U;
       tree eltype = TREE_TYPE (type);
-      tree artype;
-
       tree index_type = NULL_TREE;
+
       if (minsize == HOST_WIDE_INT_M1U)
 	{
 	  /* Determine if this is a VLA (an array whose most significant
@@ -2278,28 +2277,24 @@  attr_access::array_as_string (tree type) const
       else  if (minsize)
 	index_type = build_index_type (size_int (minsize - 1));
 
-      artype = build_array_type (eltype, index_type);
-
+      tree arat = NULL_TREE;
       if (static_p || vla_p)
 	{
 	  tree flag = static_p ? integer_one_node : NULL_TREE;
 	  /* Hack: there's no language-independent way to encode
 	     the "static" specifier or the "*" notation in an array type.
-	     Temporarily add an attribute to have the pretty printer add
-	     "static" or "*", and remove it later.  The static notation
-	     is only valid in the most significant bound but [*] can be
-	     used for any bound.  Because [*] is represented the same as
-	     [0] this hack only works for the most significant bound like
-	     static and the others are rendered as [0].  */
-	  tree at = tree_cons (get_identifier ("array"), flag, NULL_TREE);
-	  TYPE_ATTRIBUTES (artype) = at;
+	     Add a "fake" attribute to have the pretty printer add "static"
+	     or "*", and remove it later.  The static notation is only
+	     valid in the most significant bound but [*] can be used for
+	     any bound.  Because [*] is represented the same as [0] this
+	     hack only works for the most significant bound like static
+	     and the others are rendered as [0].  */
+	  arat = tree_cons (get_identifier ("array"), flag, NULL_TREE);
 	}
 
-      TYPE_ATOMIC (artype) = TYPE_ATOMIC (type);
-      TYPE_READONLY (artype) = TYPE_READONLY (type);
-      TYPE_RESTRICT (artype) = TYPE_RESTRICT (type);
-      TYPE_VOLATILE (artype) = TYPE_VOLATILE (type);
-      type = artype;
+      const int quals = TYPE_QUALS (type);
+      type = build_array_type (eltype, index_type);
+      type = build_type_attribute_qual_variant (type, arat, quals);
     }
 
   /* Format the type using the current pretty printer.  The generic tree
@@ -2309,10 +2304,6 @@  attr_access::array_as_string (tree type) const
   typstr = pp_formatted_text (pp);
   delete pp;
 
-  if (this->str)
-    /* Remove the attribute that wasn't installed by decl_attributes.  */
-    TYPE_ATTRIBUTES (type) = NULL_TREE;
-
   return typstr;
 }
 
diff --git a/gcc/testsuite/gcc.dg/Warray-parameter-7.c b/gcc/testsuite/gcc.dg/Warray-parameter-7.c
new file mode 100644
index 00000000000..4863045be78
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Warray-parameter-7.c
@@ -0,0 +1,25 @@ 
+/* PR c/97206 - ICE in composite_type on declarations of a similar array types
+   { dg-do compile }
+   { dg-options "-Wall" } */
+
+__attribute__((__access__(__write_only__, 1))) void
+f1 (char* restrict);
+
+void f1 (char*);
+
+char a1[];
+char a1[] = { };
+
+
+void f2 (char[restrict]);
+void f2 (char*);
+
+char a2[];
+char a2[] = { };
+
+
+void f3 (char*);
+void f3 (char[const]);
+
+extern const char a3[];
+extern const char a3[1];
diff --git a/gcc/testsuite/gcc.dg/Warray-parameter-8.c b/gcc/testsuite/gcc.dg/Warray-parameter-8.c
new file mode 100644
index 00000000000..b152702b847
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Warray-parameter-8.c
@@ -0,0 +1,36 @@ 
+/* Verify that combinations of array type qualifiers render correctly.
+   { dg-do compile }
+   { dg-options "-Warray-parameter" } */
+
+void fatm (int[_Atomic 1]);       // { dg-message "previously declared as 'int\\\[_Atomic 1]" }
+void fatm (int[_Atomic 2]);       // { dg-warning "argument 1 of type 'int\\\[_Atomic 2]' with mismatched bound" }
+
+
+void fcst (int[const 2]);         // { dg-message "previously declared as 'int\\\[const 2]" }
+void fcst (int[const 3]);         // { dg-warning "argument 1 of type 'int\\\[const 3]' with mismatched bound" }
+
+
+void frst (int[restrict 3]);      // { dg-message "previously declared as 'int\\\[restrict 3]" }
+void frst (int[restrict 4]);      // { dg-warning "argument 1 of type 'int\\\[restrict 4]' with mismatched bound" }
+
+void fvol (int[volatile 4]);      // { dg-message "previously declared as 'int\\\[volatile 4]" }
+void fvol (int[volatile 5]);      // { dg-warning "argument 1 of type 'int\\\[volatile 5]' with mismatched bound" }
+
+
+void fcr (int[const restrict 1]);   // { dg-message "previously declared as 'int\\\[\(const restrict|restrict const\) 1]" }
+void fcr (int[restrict volatile 2]); // { dg-warning "argument 1 of type 'int\\\[\(restrict volatile|volatile restrict\) 2]' with mismatched bound" }
+void fcr (int[const restrict volatile 3]);  // { dg-warning "argument 1 of type 'int\\\[const volatile restrict 3]' with mismatched bound" }
+
+
+extern int n;
+
+void fcx_n (int [const 1][n]);      // { dg-message "previously declared as 'int\\\[const 1]\\\[n]'" "note" }
+void fcx_n (int [restrict 2][n]);   // { dg-warning "argument 1 of type 'int\\\[restrict 2]\\\[n]' with mismatched bound" }
+
+
+extern int n1, n2;
+
+/* The mismatch in the array bound should be diagnosed but the mismatch
+   in the VLA should not be without -Wvla-parameter.  */
+void fc3_n1 (int [const 3][n1]);   // { dg-message "previously declared as 'int\\\[const 3]\\\[n1]'" "note" }
+void fc3_n1 (int [const 5][n2]);   // { dg-warning "argument 1 of type 'int\\\[const 5]\\\[n2]' with mismatched bound" }
diff --git a/gcc/testsuite/gcc.dg/Wvla-parameter-5.c b/gcc/testsuite/gcc.dg/Wvla-parameter-5.c
new file mode 100644
index 00000000000..16b40d9539a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wvla-parameter-5.c
@@ -0,0 +1,22 @@ 
+/* Verify that combinations of array type qualifiers render correctly.
+   { dg-do compile }
+   { dg-options "-Wvla-parameter" } */
+
+extern int n1, n2;
+
+void fcx_n1 (int [const][n1]);     // { dg-message "previously declared as 'int\\\[const]\\\[n1]' with bound 'n1'" "note" }
+void fcx_n1 (int [const][n2]);     // { dg-warning "argument 1 of type 'int\\\[const]\\\[n2]' declared with mismatched bound 'n2'" }
+
+/* The mismatch in the array bound should not be diagnosed without
+   -Warray-parameter but the mismatch in the VLA should still be
+   diagnosed.  */
+void fc3_n1 (int [const 3][n1]);   // { dg-message "previously declared as 'int\\\[const 3]\\\[n1]' with bound 'n1'" "note" }
+void fc3_n1 (int [const 5][n2]);   // { dg-warning "argument 1 of type 'int\\\[const 5]\\\[n2]' declared with mismatched bound 'n2'" }
+
+
+void frx_n1 (int [restrict][n1]);  // { dg-message "previously declared as 'int\\\[restrict]\\\[n1]' with bound 'n1'" "note" }
+void frx_n1 (int [restrict][n2]);  // { dg-warning "argument 1 of type 'int\\\[restrict]\\\[n2]' declared with mismatched bound 'n2'" }
+
+
+void fvx_n2 (int [volatile][n2]);  // { dg-message "previously declared as 'int\\\[volatile]\\\[n2]' with bound 'n2'" "note" }
+void fvx_n2 (int [volatile][n1]);  // { dg-warning "argument 1 of type 'int\\\[volatile]\\\[n1]' declared with mismatched bound 'n1'" }