Re: [C PATCH, v2] Fix ICE with -g and -std=c23 related to incomplete types [PR114361]
On Mon, Apr 15, 2024 at 10:02:25AM +0200, Richard Biener wrote:
> > Though, haven't managed to reproduce it with -O2 -flto -std=c23
> > struct S;
> > typedef struct S **V[10];
> > V **foo (int x) { return 0; }
> > struct S { int s; };
> > either.
> > So, maybe let's drop the ipa-free-lang-data.cc part?
> > Seems fld_incomplete_type_of uses fld_type_variant which should
> > copy over TYPE_CANONICAL.
>
> If you have a testcase that still triggers it would be nice to see it.
I don't, that is why I'm now suggesting to just drop that hunk.
Jakub
Re: [C PATCH, v2] Fix ICE with -g and -std=c23 related to incomplete types [PR114361]
On Mon, 15 Apr 2024, Jakub Jelinek wrote:
> On Mon, Apr 15, 2024 at 09:38:29AM +0200, Jakub Jelinek wrote:
> > I had this spot instrumented to log the different cases (before adding the
> > code to fix up also pointer types in c_update_type_canonical) and the only
> > thing
> > that triggered was that the 2 TYPE_CANONICALs weren't equal if
> > TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (t)), the other was just in case.
> > gcc.c-torture/compile/20021205-1.c
> > gcc.c-torture/compile/20040214-2.c
> > gcc.c-torture/compile/20060109-1.c
> > gcc.c-torture/compile/pr113623.c
> > gcc.c-torture/compile/pr46866.c
> > gcc.c-torture/compile/pta-1.c
> > gcc.c-torture/execute/pr33870-1.c
> > gcc.c-torture/execute/pr33870.c
> > gcc.dg/torture/pr57478.c
> > tests were affected in make check-gcc.
> > I thought it would be a clear consequence of the choice we've discussed on
> > IRC, that build_pointer_type_for_mode and other tree.cc functions which
> > lookup/create derived types don't try to fill in TYPE_CANONICAL for
> > types derived from something which initially had TYPE_STRUCTURAL_EQUALITY_P
> > but later changed to non-TYPE_STRUCTURAL_EQUALITY_P. The patch updates
> > it solely for qualified types/related pointer types, but doesn't do that
> > for array types, pointer to array types, function types, ...
> > So, I think the assertion could still trigger if we have something like
> > -O2 -flto -std=c23
> > struct S;
> > typedef struct S *T;
> > typedef T U[10];
> > typedef U *V;
> > V foo (int x) { return 0; }
> > struct S { int s; };
> > (but doesn't, dunno what I'm missing; though here certainly V and U have
> > TYPE_STRUCTURAL_EQUALITY_P, even T has because it is a typedef, not
> > something actually normally returned by build_pointer_type).
>
> Though, haven't managed to reproduce it with -O2 -flto -std=c23
> struct S;
> typedef struct S **V[10];
> V **foo (int x) { return 0; }
> struct S { int s; };
> either.
> So, maybe let's drop the ipa-free-lang-data.cc part?
> Seems fld_incomplete_type_of uses fld_type_variant which should
> copy over TYPE_CANONICAL.
If you have a testcase that still triggers it would be nice to see it.
Richard.
Re: [C PATCH, v2] Fix ICE with -g and -std=c23 related to incomplete types [PR114361]
On Mon, Apr 15, 2024 at 09:38:29AM +0200, Jakub Jelinek wrote:
> I had this spot instrumented to log the different cases (before adding the
> code to fix up also pointer types in c_update_type_canonical) and the only
> thing
> that triggered was that the 2 TYPE_CANONICALs weren't equal if
> TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (t)), the other was just in case.
> gcc.c-torture/compile/20021205-1.c
> gcc.c-torture/compile/20040214-2.c
> gcc.c-torture/compile/20060109-1.c
> gcc.c-torture/compile/pr113623.c
> gcc.c-torture/compile/pr46866.c
> gcc.c-torture/compile/pta-1.c
> gcc.c-torture/execute/pr33870-1.c
> gcc.c-torture/execute/pr33870.c
> gcc.dg/torture/pr57478.c
> tests were affected in make check-gcc.
> I thought it would be a clear consequence of the choice we've discussed on
> IRC, that build_pointer_type_for_mode and other tree.cc functions which
> lookup/create derived types don't try to fill in TYPE_CANONICAL for
> types derived from something which initially had TYPE_STRUCTURAL_EQUALITY_P
> but later changed to non-TYPE_STRUCTURAL_EQUALITY_P. The patch updates
> it solely for qualified types/related pointer types, but doesn't do that
> for array types, pointer to array types, function types, ...
> So, I think the assertion could still trigger if we have something like
> -O2 -flto -std=c23
> struct S;
> typedef struct S *T;
> typedef T U[10];
> typedef U *V;
> V foo (int x) { return 0; }
> struct S { int s; };
> (but doesn't, dunno what I'm missing; though here certainly V and U have
> TYPE_STRUCTURAL_EQUALITY_P, even T has because it is a typedef, not
> something actually normally returned by build_pointer_type).
Though, haven't managed to reproduce it with -O2 -flto -std=c23
struct S;
typedef struct S **V[10];
V **foo (int x) { return 0; }
struct S { int s; };
either.
So, maybe let's drop the ipa-free-lang-data.cc part?
Seems fld_incomplete_type_of uses fld_type_variant which should
copy over TYPE_CANONICAL.
Jakub
Re: [C PATCH, v2] Fix ICE with -g and -std=c23 related to incomplete types [PR114361]
On Mon, Apr 15, 2024 at 08:55:56AM +0200, Richard Biener wrote:
> > diff --git a/gcc/ipa-free-lang-data.cc b/gcc/ipa-free-lang-data.cc
> > index 16ed55e2e5a..6f75444e513 100644
> > --- a/gcc/ipa-free-lang-data.cc
> > +++ b/gcc/ipa-free-lang-data.cc
> > @@ -255,7 +255,9 @@ fld_incomplete_type_of (tree t, class free_lang_data_d
> > *fld)
> > first = build_reference_type_for_mode (t2, TYPE_MODE (t),
> >TYPE_REF_CAN_ALIAS_ALL (t));
> > gcc_assert (TYPE_CANONICAL (t2) != t2
> > - && TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t)));
> > + && (TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))
> > + || TYPE_STRUCTURAL_EQUALITY_P (t2)
> > + || TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (t;
>
> I think we want the same canonical types for t2 and TREE_TYPE (t), why
> doesn't fld_incomplete_type_of make it so?
I had this spot instrumented to log the different cases (before adding the
code to fix up also pointer types in c_update_type_canonical) and the only thing
that triggered was that the 2 TYPE_CANONICALs weren't equal if
TYPE_STRUCTURAL_EQUALITY_P (TREE_TYPE (t)), the other was just in case.
gcc.c-torture/compile/20021205-1.c
gcc.c-torture/compile/20040214-2.c
gcc.c-torture/compile/20060109-1.c
gcc.c-torture/compile/pr113623.c
gcc.c-torture/compile/pr46866.c
gcc.c-torture/compile/pta-1.c
gcc.c-torture/execute/pr33870-1.c
gcc.c-torture/execute/pr33870.c
gcc.dg/torture/pr57478.c
tests were affected in make check-gcc.
I thought it would be a clear consequence of the choice we've discussed on
IRC, that build_pointer_type_for_mode and other tree.cc functions which
lookup/create derived types don't try to fill in TYPE_CANONICAL for
types derived from something which initially had TYPE_STRUCTURAL_EQUALITY_P
but later changed to non-TYPE_STRUCTURAL_EQUALITY_P. The patch updates
it solely for qualified types/related pointer types, but doesn't do that
for array types, pointer to array types, function types, ...
So, I think the assertion could still trigger if we have something like
-O2 -flto -std=c23
struct S;
typedef struct S *T;
typedef T U[10];
typedef U *V;
V foo (int x) { return 0; }
struct S { int s; };
(but doesn't, dunno what I'm missing; though here certainly V and U have
TYPE_STRUCTURAL_EQUALITY_P, even T has because it is a typedef, not
something actually normally returned by build_pointer_type).
Jakub
Re: [C PATCH, v2] Fix ICE with -g and -std=c23 related to incomplete types [PR114361]
On Sun, Apr 14, 2024 at 02:30:27PM +0200, Martin Uecker wrote:
> 2024-04-12 Martin Uecker
> Jakub Jelinek
>
> PR lto/114574
> PR c/114361
> gcc/
> * ipa-free-lang-data.cc (fld_incomplete_type_of): Allow
> either of the types in the assert to have TYPE_STRUCTURAL_EQUALITY_P.
> gcc/c/
> * c-decl.cc (shadow_tag_warned): For flag_isoc23 and code not
> ENUMERAL_TYPE use SET_TYPE_STRUCTURAL_EQUALITY.
> (parser_xref_tag): Likewise.
> (start_struct): For flag_isoc23 use SET_TYPE_STRUCTURAL_EQUALITY.
> (c_update_type_canonical): New function.
> (finish_struct): Put NULL as second == operand rather than first.
> Assert TYPE_STRUCTURAL_EQUALITY_P. Call c_update_type_canonical.
> * c-typeck.cc (composite_type_internal): Use
> SET_TYPE_STRUCTURAL_EQUALITY. Formatting fix.
> gcc/testsuite/
> * gcc.dg/pr114574-1.c: New test.
> * gcc.dg/pr114574-2.c: New test.
> * gcc.dg/pr114361.c: New test.
> * gcc.dg/c23-tag-incomplete-1.c: New test.
> * gcc.dg/c23-tag-incomplete-2.c: New test.
Just a nit I haven't seen before (not a review, can't do that when
I've participated on the patch):
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/c23-tag-incomplete-1.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile }
> + * { dg-options "-std=c23 -g" } */
> +
> +struct a;
> +typedef struct a b;
> +
> +void g() {
> +struct a { b* x; };
> +}
> +
> +struct a { b* x; };
> +
> +
> +
Please avoid the trailing whitespace in the testcases. They should end with
a single newline, not 3-4 newlines.
Jakub
Re: [C PATCH, v2] Fix ICE with -g and -std=c23 related to incomplete types [PR114361]
On Sun, 14 Apr 2024, Martin Uecker wrote:
>
> I had to revert the old patch because it broke LTO which lead
> to PR114574. We now set TYPE_STRUCTURAL_EQUALITY and properly
> update TYPE_CANONICAL for such types and also for pointers
> to such types via a new function c_update_type_canonical
> (thanks to Jakob).
>
>
> Bootstrapped and regession tested on x86_64.
>
>
>
> Fix ICE with -g and -std=c23 related to incomplete types [PR114361]
>
> We did not update TYPE_CANONICAL for incomplete variants when
> completing a structure. We now set TYPE_STRUCTURAL_EQUALITY for
> incomplete structure and union types and then update TYPE_CANONICAL
> later. See PR114574 for discussion.
>
>
> 2024-04-12 Martin Uecker
> Jakub Jelinek
>
> PR lto/114574
> PR c/114361
> gcc/
> * ipa-free-lang-data.cc (fld_incomplete_type_of): Allow
> either of the types in the assert to have TYPE_STRUCTURAL_EQUALITY_P.
> gcc/c/
> * c-decl.cc (shadow_tag_warned): For flag_isoc23 and code not
> ENUMERAL_TYPE use SET_TYPE_STRUCTURAL_EQUALITY.
> (parser_xref_tag): Likewise.
> (start_struct): For flag_isoc23 use SET_TYPE_STRUCTURAL_EQUALITY.
> (c_update_type_canonical): New function.
> (finish_struct): Put NULL as second == operand rather than first.
> Assert TYPE_STRUCTURAL_EQUALITY_P. Call c_update_type_canonical.
> * c-typeck.cc (composite_type_internal): Use
> SET_TYPE_STRUCTURAL_EQUALITY. Formatting fix.
> gcc/testsuite/
> * gcc.dg/pr114574-1.c: New test.
> * gcc.dg/pr114574-2.c: New test.
> * gcc.dg/pr114361.c: New test.
> * gcc.dg/c23-tag-incomplete-1.c: New test.
> * gcc.dg/c23-tag-incomplete-2.c: New test.
> ---
> gcc/c/c-decl.cc | 47 -
> gcc/c/c-typeck.cc | 4 +-
> gcc/ipa-free-lang-data.cc | 4 +-
> gcc/testsuite/gcc.dg/c23-tag-incomplete-1.c | 14 ++
> gcc/testsuite/gcc.dg/c23-tag-incomplete-2.c | 13 ++
> gcc/testsuite/gcc.dg/pr114361.c | 11 +
> gcc/testsuite/gcc.dg/pr114574-1.c | 10 +
> gcc/testsuite/gcc.dg/pr114574-2.c | 10 +
> 8 files changed, 110 insertions(+), 3 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/c23-tag-incomplete-1.c
> create mode 100644 gcc/testsuite/gcc.dg/c23-tag-incomplete-2.c
> create mode 100644 gcc/testsuite/gcc.dg/pr114361.c
> create mode 100644 gcc/testsuite/gcc.dg/pr114574-1.c
> create mode 100644 gcc/testsuite/gcc.dg/pr114574-2.c
>
> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
> index 345090dae38..54917297b6a 100644
> --- a/gcc/c/c-decl.cc
> +++ b/gcc/c/c-decl.cc
> @@ -5051,6 +5051,8 @@ shadow_tag_warned (const struct c_declspecs *declspecs,
> int warned)
> if (t == NULL_TREE)
> {
> t = make_node (code);
> + if (flag_isoc23 && code != ENUMERAL_TYPE)
> + SET_TYPE_STRUCTURAL_EQUALITY (t);
> pushtag (input_location, name, t);
> }
> }
> @@ -8809,6 +8811,8 @@ parser_xref_tag (location_t loc, enum tree_code code,
> tree name,
> the forward-reference will be altered into a real type. */
>
>ref = make_node (code);
> + if (flag_isoc23 && code != ENUMERAL_TYPE)
> +SET_TYPE_STRUCTURAL_EQUALITY (ref);
>if (code == ENUMERAL_TYPE)
> {
>/* Give the type a default layout like unsigned int
> @@ -8910,6 +8914,8 @@ start_struct (location_t loc, enum tree_code code, tree
> name,
>if (ref == NULL_TREE || TREE_CODE (ref) != code)
> {
>ref = make_node (code);
> + if (flag_isoc23)
> + SET_TYPE_STRUCTURAL_EQUALITY (ref);
>pushtag (loc, name, ref);
> }
>
> @@ -9347,6 +9353,43 @@ is_flexible_array_member_p (bool is_last_field,
>return false;
> }
>
> +/* Recompute TYPE_CANONICAL for qualified versions of the type and
> + related pointer types after an aggregate type has been finalized.
> + Will not update array types, pointers to array types, function
> + types and other derived types created while the type was still
> + incomplete, those will remain TYPE_STRUCTURAL_EQUALITY_P. */
> +
> +static void
> +c_update_type_canonical (tree t)
> +{
> + for (tree x = TYPE_MAIN_VARIANT (t); x; x = TYPE_NEXT_VARIANT (x))
> +{
> + if (x != t
> + && TYPE_STRUCTURAL_EQUALITY_P (x)
> + && check_qualified_type (x, t, TYPE_QUALS (x)))
> + {
> + if (TYPE_CANONICAL (t) != t)
> + TYPE_CANONICAL (x)
> + = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x));
> + else
> + TYPE_CANONICAL (x) = x;
> + }
> + else if (x != t)
> + continue;
> + for (tree p = TYPE_POINTER_TO (x); p; p = TYPE_NEXT_PTR_TO (p))
> + {
> + if (!TYPE_STRUCTURAL_EQUALITY_P (p))
> + continue;
> + if (TYPE_CANONICAL (x) != x || TYPE_REF_CAN_ALIAS_ALL (p))
> +
Re: [C PATCH, v2] Fix ICE with -g and -std=c23 related to incomplete types [PR114361]
(new email for Joseph)
Am Sonntag, dem 14.04.2024 um 14:30 +0200 schrieb Martin Uecker:
> I had to revert the old patch because it broke LTO which lead
> to PR114574. We now set TYPE_STRUCTURAL_EQUALITY and properly
> update TYPE_CANONICAL for such types and also for pointers
> to such types via a new function c_update_type_canonical
> (thanks to Jakob).
>
>
> Bootstrapped and regession tested on x86_64.
>
>
>
> Fix ICE with -g and -std=c23 related to incomplete types [PR114361]
>
> We did not update TYPE_CANONICAL for incomplete variants when
> completing a structure. We now set TYPE_STRUCTURAL_EQUALITY for
> incomplete structure and union types and then update TYPE_CANONICAL
> later. See PR114574 for discussion.
>
>
> 2024-04-12 Martin Uecker
> Jakub Jelinek
>
> PR lto/114574
> PR c/114361
> gcc/
> * ipa-free-lang-data.cc (fld_incomplete_type_of): Allow
> either of the types in the assert to have TYPE_STRUCTURAL_EQUALITY_P.
> gcc/c/
> * c-decl.cc (shadow_tag_warned): For flag_isoc23 and code not
> ENUMERAL_TYPE use SET_TYPE_STRUCTURAL_EQUALITY.
> (parser_xref_tag): Likewise.
> (start_struct): For flag_isoc23 use SET_TYPE_STRUCTURAL_EQUALITY.
> (c_update_type_canonical): New function.
> (finish_struct): Put NULL as second == operand rather than first.
> Assert TYPE_STRUCTURAL_EQUALITY_P. Call c_update_type_canonical.
> * c-typeck.cc (composite_type_internal): Use
> SET_TYPE_STRUCTURAL_EQUALITY. Formatting fix.
> gcc/testsuite/
> * gcc.dg/pr114574-1.c: New test.
> * gcc.dg/pr114574-2.c: New test.
> * gcc.dg/pr114361.c: New test.
> * gcc.dg/c23-tag-incomplete-1.c: New test.
> * gcc.dg/c23-tag-incomplete-2.c: New test.
> ---
> gcc/c/c-decl.cc | 47 -
> gcc/c/c-typeck.cc | 4 +-
> gcc/ipa-free-lang-data.cc | 4 +-
> gcc/testsuite/gcc.dg/c23-tag-incomplete-1.c | 14 ++
> gcc/testsuite/gcc.dg/c23-tag-incomplete-2.c | 13 ++
> gcc/testsuite/gcc.dg/pr114361.c | 11 +
> gcc/testsuite/gcc.dg/pr114574-1.c | 10 +
> gcc/testsuite/gcc.dg/pr114574-2.c | 10 +
> 8 files changed, 110 insertions(+), 3 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/c23-tag-incomplete-1.c
> create mode 100644 gcc/testsuite/gcc.dg/c23-tag-incomplete-2.c
> create mode 100644 gcc/testsuite/gcc.dg/pr114361.c
> create mode 100644 gcc/testsuite/gcc.dg/pr114574-1.c
> create mode 100644 gcc/testsuite/gcc.dg/pr114574-2.c
>
> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
> index 345090dae38..54917297b6a 100644
> --- a/gcc/c/c-decl.cc
> +++ b/gcc/c/c-decl.cc
> @@ -5051,6 +5051,8 @@ shadow_tag_warned (const struct c_declspecs *declspecs,
> int warned)
> if (t == NULL_TREE)
> {
> t = make_node (code);
> + if (flag_isoc23 && code != ENUMERAL_TYPE)
> + SET_TYPE_STRUCTURAL_EQUALITY (t);
> pushtag (input_location, name, t);
> }
> }
> @@ -8809,6 +8811,8 @@ parser_xref_tag (location_t loc, enum tree_code code,
> tree name,
> the forward-reference will be altered into a real type. */
>
>ref = make_node (code);
> + if (flag_isoc23 && code != ENUMERAL_TYPE)
> +SET_TYPE_STRUCTURAL_EQUALITY (ref);
>if (code == ENUMERAL_TYPE)
> {
>/* Give the type a default layout like unsigned int
> @@ -8910,6 +8914,8 @@ start_struct (location_t loc, enum tree_code code, tree
> name,
>if (ref == NULL_TREE || TREE_CODE (ref) != code)
> {
>ref = make_node (code);
> + if (flag_isoc23)
> + SET_TYPE_STRUCTURAL_EQUALITY (ref);
>pushtag (loc, name, ref);
> }
>
> @@ -9347,6 +9353,43 @@ is_flexible_array_member_p (bool is_last_field,
>return false;
> }
>
> +/* Recompute TYPE_CANONICAL for qualified versions of the type and
> + related pointer types after an aggregate type has been finalized.
> + Will not update array types, pointers to array types, function
> + types and other derived types created while the type was still
> + incomplete, those will remain TYPE_STRUCTURAL_EQUALITY_P. */
> +
> +static void
> +c_update_type_canonical (tree t)
> +{
> + for (tree x = TYPE_MAIN_VARIANT (t); x; x = TYPE_NEXT_VARIANT (x))
> +{
> + if (x != t
> + && TYPE_STRUCTURAL_EQUALITY_P (x)
> + && check_qualified_type (x, t, TYPE_QUALS (x)))
> + {
> + if (TYPE_CANONICAL (t) != t)
> + TYPE_CANONICAL (x)
> + = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x));
> + else
> + TYPE_CANONICAL (x) = x;
> + }
> + else if (x != t)
> + continue;
> + for (tree p = TYPE_POINTER_TO (x); p; p = TYPE_NEXT_PTR_TO (p))
> + {
> + if (!TYPE_STRUCTURAL_EQUALITY_P (p))
> + continue;
> + if (TYPE_CANONICAL (x) != x ||
