diff mbox series

Update array address space in c_build_qualified_type

Message ID df9f403f4e3c1f9ad620e8d5e38adaa59be2332b.camel@microchip.com
State Accepted
Headers
Series Update array address space in c_build_qualified_type |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Li, Pan2 via Gcc-patches June 21, 2023, 5:57 a.m. UTC 
  Hi,

  When c-typeck.cc:c_build_qualified_type builds an array type
  from its element type, it does not copy the address space of
  the element type to the array type itself. This is unlike
  tree.cc:build_array_type_1, which explicitly does

TYPE_ADDR_SPACE (t) = TYPE_ADDR_SPACE (elt_type);

  This causes the ICE described in
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86869.

struct S {
  char y[2];
};

extern const __memx  struct S *s;

extern void bar(const __memx void*);

void foo(void) {
  bar(&s->y);
}

  build_component_ref calls c_build_qualified_type, passing in the
  array type and quals including the address space (ADDR_SPACE_MEMX
  in this case). Because of this missing address space copy, the
  returned array type remains in the generic address space.  Later
  down the line, expand_expr_addr_expr detects the mismatch in
  address space/mode and tries to convert, and that leads to the
  ICE described in the bug.

  This patch sets the address space of the array type to that of the
  element type.

  Regression tests for avr look ok. Ok for trunk?

Regards
Senthil

	PR 86869

gcc/c/ChangeLog:

	* c-typeck.cc (c_build_qualified_type): Set
	TYPE_ADDR_SPACE for ARRAY_TYPE.

gcc/testsuite/ChangeLog:

	* gcc.target/avr/pr86869.c: New test.

Comments

Richard Biener June 21, 2023, 8:15 a.m. UTC | #1 
On Wed, Jun 21, 2023 at 7:57 AM <SenthilKumar.Selvaraj@microchip.com> wrote:
>
> Hi,
>
>   When c-typeck.cc:c_build_qualified_type builds an array type
>   from its element type, it does not copy the address space of
>   the element type to the array type itself. This is unlike
>   tree.cc:build_array_type_1, which explicitly does
>
> TYPE_ADDR_SPACE (t) = TYPE_ADDR_SPACE (elt_type);
>
>   This causes the ICE described in
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86869.
>
> struct S {
>   char y[2];
> };
>
> extern const __memx  struct S *s;
>
> extern void bar(const __memx void*);
>
> void foo(void) {
>   bar(&s->y);
> }
>
>   build_component_ref calls c_build_qualified_type, passing in the
>   array type and quals including the address space (ADDR_SPACE_MEMX
>   in this case). Because of this missing address space copy, the
>   returned array type remains in the generic address space.  Later
>   down the line, expand_expr_addr_expr detects the mismatch in
>   address space/mode and tries to convert, and that leads to the
>   ICE described in the bug.
>
>   This patch sets the address space of the array type to that of the
>   element type.
>
>   Regression tests for avr look ok. Ok for trunk?

The patch looks OK to me but please let a C frontend maintainer
double-check (I've CCed Joseph for this).

Thanks,
Richard.

> Regards
> Senthil
>
>         PR 86869
>
> gcc/c/ChangeLog:
>
>         * c-typeck.cc (c_build_qualified_type): Set
>         TYPE_ADDR_SPACE for ARRAY_TYPE.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/avr/pr86869.c: New test.
>
> diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
> index 22e240a3c2a..d4ab1d1bd46 100644
> --- a/gcc/c/c-typeck.cc
> +++ b/gcc/c/c-typeck.cc
> @@ -16284,6 +16284,7 @@ c_build_qualified_type (tree type, int type_quals, tree orig_qual_type,
>
>           t = build_variant_type_copy (type);
>           TREE_TYPE (t) = element_type;
> +         TYPE_ADDR_SPACE (t) = TYPE_ADDR_SPACE (element_type);
>
>            if (TYPE_STRUCTURAL_EQUALITY_P (element_type)
>                || (domain && TYPE_STRUCTURAL_EQUALITY_P (domain)))
> diff --git a/gcc/testsuite/gcc.target/avr/pr86869.c b/gcc/testsuite/gcc.target/avr/pr86869.c
> new file mode 100644
> index 00000000000..54cd984276e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/avr/pr86869.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-options "-std=gnu99" } */
> +
> +extern void bar(const __memx void* p);
> +
> +struct S {
> +  char y[2];
> +};
> +extern const __memx struct S *s;
> +
> +void foo(void) {
> +  bar(&s->y);
> +}
Joseph Myers June 21, 2023, 6:37 p.m. UTC | #2 
On Wed, 21 Jun 2023, Richard Biener via Gcc-patches wrote:

> >   This patch sets the address space of the array type to that of the
> >   element type.
> >
> >   Regression tests for avr look ok. Ok for trunk?
> 
> The patch looks OK to me but please let a C frontend maintainer
> double-check (I've CCed Joseph for this).

The question would be whether there are any TYPE_QUALS uses in the C front 
end that behave incorrectly given TYPE_ADDR_SPACE (acting as qualifiers) 
being set on an array type - conceptually, before C2x, array types are 
unqualified, only the element types are qualified.

The fact that this changed in C2x gives a shortcut to doing that analysis 
- you don't need to check all TYPE_QUALS uses in the front end, only a 
limited number of places that might handle qualifiers on arrays that 
already have conditionals to do things differently in C2x mode.  But some 
sort of analysis of those places, to see how they'd react to an array type 
itself having TYPE_ADDR_SPACE set, would be helpful.  If you're lucky, all 
those places only care about TYPE_QUALS on the element type and not on the 
array type itself.
Li, Pan2 via Gcc-patches June 22, 2023, 9:55 a.m. UTC | #3 
On Wed, 2023-06-21 at 18:37 +0000, Joseph Myers wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, 21 Jun 2023, Richard Biener via Gcc-patches wrote:
> 
> > >   This patch sets the address space of the array type to that of the
> > >   element type.
> > > 
> > >   Regression tests for avr look ok. Ok for trunk?
> > 
> > The patch looks OK to me but please let a C frontend maintainer
> > double-check (I've CCed Joseph for this).
> 
> The question would be whether there are any TYPE_QUALS uses in the C front
> end that behave incorrectly given TYPE_ADDR_SPACE (acting as qualifiers)
> being set on an array type - conceptually, before C2x, array types are
> unqualified, only the element types are qualified.

Hmm, but tree.cc:build_array_type_1 sets the address space of the element
type to the array type, and the relevant commit's ChangeLog entry (from 2009)
says "Inherit array address space from element type."

On the avr target, for an array like

const __flash int arr[] = {1,2,3};

I can see that the array type gets the address space of the element type
already, even before this patch. Surely that must have caused any code that
doesn't expect address space in array type quals to be broken then? 

Regards
Senthil
diff mbox series

Patch

diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 22e240a3c2a..d4ab1d1bd46 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -16284,6 +16284,7 @@  c_build_qualified_type (tree type, int type_quals, tree orig_qual_type,
 
 	  t = build_variant_type_copy (type);
 	  TREE_TYPE (t) = element_type;
+	  TYPE_ADDR_SPACE (t) = TYPE_ADDR_SPACE (element_type);
 
           if (TYPE_STRUCTURAL_EQUALITY_P (element_type)
               || (domain && TYPE_STRUCTURAL_EQUALITY_P (domain)))
diff --git a/gcc/testsuite/gcc.target/avr/pr86869.c b/gcc/testsuite/gcc.target/avr/pr86869.c
new file mode 100644
index 00000000000..54cd984276e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/avr/pr86869.c
@@ -0,0 +1,13 @@ 
+/* { dg-do compile } */
+/* { dg-options "-std=gnu99" } */
+
+extern void bar(const __memx void* p);
+
+struct S {
+  char y[2];
+};
+extern const __memx struct S *s;
+
+void foo(void) {
+  bar(&s->y);
+}