| Message ID | 740a0212f71eb64732ae481f77952b68ab2e7ca8.camel@tugraz.at |
|---|---|
| State | New |
| Headers | show |
| Series | [C] Fix ICE related to incomplete structures in C23 [PR114930,PR115502]. | expand |
> Am 18.06.2024 um 17:20 schrieb Martin Uecker <uecker@tugraz.at>: > > > As discussed this replaces the use of check_qualified_type with > a simple check for qualifiers as suggested by Jakub in > c_update_type_canonical. Note a canonical type should always be unqualified (for classical qualifiers, not address space or atomic qualification) Richard > Martin > > > Bootstrapped and regression tested on x86_64. > > > C23: Fix ICE related to incomplete structures [PR114930,PR115502]. > > The fix for PR114574 needs to be further revised because check_qualified_type > makes decision based on TYPE_NAME which can be incorrect for C when there > are TYPE_DECLS involved. > > gcc/c/: > * c-decl.c (c_update_type_canonical): Do not use check_qualified_type. > > gcc/testsuite/: > * gcc.dg/pr114930.c: New test. > * gcc.dg/pr115502.c: New test. > > diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc > index 01326570e2b..610061a07f8 100644 > --- a/gcc/c/c-decl.cc > +++ b/gcc/c/c-decl.cc > @@ -9374,7 +9374,7 @@ c_update_type_canonical (tree t) > if (TYPE_QUALS (x) == TYPE_QUALS (t)) > TYPE_CANONICAL (x) = TYPE_CANONICAL (t); > else if (TYPE_CANONICAL (t) != t > - || check_qualified_type (x, t, TYPE_QUALS (x))) > + || TYPE_QUALS (x) != TYPE_QUALS (TYPE_CANONICAL (t))) > TYPE_CANONICAL (x) > = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); > else > diff --git a/gcc/testsuite/gcc.dg/pr114930.c b/gcc/testsuite/gcc.dg/pr114930.c > new file mode 100644 > index 00000000000..5e982fb8929 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr114930.c > @@ -0,0 +1,9 @@ > +/* { dg-do compile } > + * { dg-options "-std=c23 -flto" } */ > + > +typedef struct WebPPicture WebPPicture; > +typedef int (*WebPProgressHook)(const WebPPicture *); > +WebPProgressHook progress_hook; > +struct WebPPicture { > +} WebPGetColorPalette(const struct WebPPicture *); > + > diff --git a/gcc/testsuite/gcc.dg/pr115502.c b/gcc/testsuite/gcc.dg/pr115502.c > new file mode 100644 > index 00000000000..02b52622c5a > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr115502.c > @@ -0,0 +1,9 @@ > +/* { dg-do compile } > + * { dg-options "-std=c23 -flto" } */ > + > +typedef struct _OSet OSet; > +typedef OSet AvlTree; > +void vgPlain_OSetGen_Lookup(const OSet *); > +struct _OSet {}; > +void vgPlain_OSetGen_Lookup(const AvlTree *); > + >
Am Dienstag, dem 18.06.2024 um 17:27 +0200 schrieb Richard Biener: > > > Am 18.06.2024 um 17:20 schrieb Martin Uecker <uecker@tugraz.at>: > > > > > > As discussed this replaces the use of check_qualified_type with > > a simple check for qualifiers as suggested by Jakub in > > c_update_type_canonical. > > Note a canonical type should always be unqualified (for > classical qualifiers, not address space or atomic qualification) The logic in build_qualified_type is the same as in this patch, it constructs TYPE_CANONICAL with qualifiers. Or what am I missing? Martin > > Richard > > > Martin > > > > > > Bootstrapped and regression tested on x86_64. > > > > > > C23: Fix ICE related to incomplete structures [PR114930,PR115502]. > > > > The fix for PR114574 needs to be further revised because check_qualified_type > > makes decision based on TYPE_NAME which can be incorrect for C when there > > are TYPE_DECLS involved. > > > > gcc/c/: > > * c-decl.c (c_update_type_canonical): Do not use check_qualified_type. > > > > gcc/testsuite/: > > * gcc.dg/pr114930.c: New test. > > * gcc.dg/pr115502.c: New test. > > > > diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc > > index 01326570e2b..610061a07f8 100644 > > --- a/gcc/c/c-decl.cc > > +++ b/gcc/c/c-decl.cc > > @@ -9374,7 +9374,7 @@ c_update_type_canonical (tree t) > > if (TYPE_QUALS (x) == TYPE_QUALS (t)) > > TYPE_CANONICAL (x) = TYPE_CANONICAL (t); > > else if (TYPE_CANONICAL (t) != t > > - || check_qualified_type (x, t, TYPE_QUALS (x))) > > + || TYPE_QUALS (x) != TYPE_QUALS (TYPE_CANONICAL (t))) > > TYPE_CANONICAL (x) > > = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); > > else > > diff --git a/gcc/testsuite/gcc.dg/pr114930.c b/gcc/testsuite/gcc.dg/pr114930.c > > new file mode 100644 > > index 00000000000..5e982fb8929 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.dg/pr114930.c > > @@ -0,0 +1,9 @@ > > +/* { dg-do compile } > > + * { dg-options "-std=c23 -flto" } */ > > + > > +typedef struct WebPPicture WebPPicture; > > +typedef int (*WebPProgressHook)(const WebPPicture *); > > +WebPProgressHook progress_hook; > > +struct WebPPicture { > > +} WebPGetColorPalette(const struct WebPPicture *); > > + > > diff --git a/gcc/testsuite/gcc.dg/pr115502.c b/gcc/testsuite/gcc.dg/pr115502.c > > new file mode 100644 > > index 00000000000..02b52622c5a > > --- /dev/null > > +++ b/gcc/testsuite/gcc.dg/pr115502.c > > @@ -0,0 +1,9 @@ > > +/* { dg-do compile } > > + * { dg-options "-std=c23 -flto" } */ > > + > > +typedef struct _OSet OSet; > > +typedef OSet AvlTree; > > +void vgPlain_OSetGen_Lookup(const OSet *); > > +struct _OSet {}; > > +void vgPlain_OSetGen_Lookup(const AvlTree *); > > + > >
> Am 18.06.2024 um 20:18 schrieb Martin Uecker <uecker@tugraz.at>: > > Am Dienstag, dem 18.06.2024 um 17:27 +0200 schrieb Richard Biener: >> >>>> Am 18.06.2024 um 17:20 schrieb Martin Uecker <uecker@tugraz.at>: >>> >>> >>> As discussed this replaces the use of check_qualified_type with >>> a simple check for qualifiers as suggested by Jakub in >>> c_update_type_canonical. >> >> Note a canonical type should always be unqualified (for >> classical qualifiers, not address space or atomic qualification) > > The logic in build_qualified_type is the same as in this patch, > it constructs TYPE_CANONICAL with qualifiers. Or what am I > missing? Nothing if you chose to do TYPE_QUALS (canonical) by copy-pasting. Richard > Martin > >> >> Richard >> >>> Martin >>> >>> >>> Bootstrapped and regression tested on x86_64. >>> >>> >>> C23: Fix ICE related to incomplete structures [PR114930,PR115502]. >>> >>> The fix for PR114574 needs to be further revised because check_qualified_type >>> makes decision based on TYPE_NAME which can be incorrect for C when there >>> are TYPE_DECLS involved. >>> >>> gcc/c/: >>> * c-decl.c (c_update_type_canonical): Do not use check_qualified_type. >>> >>> gcc/testsuite/: >>> * gcc.dg/pr114930.c: New test. >>> * gcc.dg/pr115502.c: New test. >>> >>> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc >>> index 01326570e2b..610061a07f8 100644 >>> --- a/gcc/c/c-decl.cc >>> +++ b/gcc/c/c-decl.cc >>> @@ -9374,7 +9374,7 @@ c_update_type_canonical (tree t) >>> if (TYPE_QUALS (x) == TYPE_QUALS (t)) >>> TYPE_CANONICAL (x) = TYPE_CANONICAL (t); >>> else if (TYPE_CANONICAL (t) != t >>> - || check_qualified_type (x, t, TYPE_QUALS (x))) >>> + || TYPE_QUALS (x) != TYPE_QUALS (TYPE_CANONICAL (t))) >>> TYPE_CANONICAL (x) >>> = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); >>> else >>> diff --git a/gcc/testsuite/gcc.dg/pr114930.c b/gcc/testsuite/gcc.dg/pr114930.c >>> new file mode 100644 >>> index 00000000000..5e982fb8929 >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.dg/pr114930.c >>> @@ -0,0 +1,9 @@ >>> +/* { dg-do compile } >>> + * { dg-options "-std=c23 -flto" } */ >>> + >>> +typedef struct WebPPicture WebPPicture; >>> +typedef int (*WebPProgressHook)(const WebPPicture *); >>> +WebPProgressHook progress_hook; >>> +struct WebPPicture { >>> +} WebPGetColorPalette(const struct WebPPicture *); >>> + >>> diff --git a/gcc/testsuite/gcc.dg/pr115502.c b/gcc/testsuite/gcc.dg/pr115502.c >>> new file mode 100644 >>> index 00000000000..02b52622c5a >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.dg/pr115502.c >>> @@ -0,0 +1,9 @@ >>> +/* { dg-do compile } >>> + * { dg-options "-std=c23 -flto" } */ >>> + >>> +typedef struct _OSet OSet; >>> +typedef OSet AvlTree; >>> +void vgPlain_OSetGen_Lookup(const OSet *); >>> +struct _OSet {}; >>> +void vgPlain_OSetGen_Lookup(const AvlTree *); >>> + >>> >
On Wed, Jun 19, 2024 at 08:04:55AM +0200, Richard Biener wrote: > >> Note a canonical type should always be unqualified (for > >> classical qualifiers, not address space or atomic qualification) > > > > The logic in build_qualified_type is the same as in this patch, > > it constructs TYPE_CANONICAL with qualifiers. Or what am I > > missing? > > Nothing if you chose to do TYPE_QUALS (canonical) by copy-pasting. As I mentioned earlier, I'd prefer the following version and I have already tested it on the testcases (note, some changes to the tests, e.g. using lto effective target), just haven't bootstrapped/regtested it fully yet. gcc/c/ * c-decl.cc (c_update_type_canonical): Assert t is main variant with 0 TYPE_QUALS. Simplify and don't use check_qualified_type. gcc/testsuite/ * gcc.dg/pr114930.c: New test. * gcc.dg/pr115502.c: New test. --- gcc/c/c-decl.cc.jj 2024-06-07 12:17:09.582969919 +0200 +++ gcc/c/c-decl.cc 2024-06-18 21:30:22.881904947 +0200 @@ -9367,18 +9367,16 @@ is_flexible_array_member_p (bool is_last static void c_update_type_canonical (tree t) { - for (tree x = TYPE_MAIN_VARIANT (t); x; x = TYPE_NEXT_VARIANT (x)) + gcc_checking_assert (TYPE_MAIN_VARIANT (t) == t && !TYPE_QUALS (t)); + for (tree x = t; x; x = TYPE_NEXT_VARIANT (x)) { if (x != t && TYPE_STRUCTURAL_EQUALITY_P (x)) { - if (TYPE_QUALS (x) == TYPE_QUALS (t)) + if (!TYPE_QUALS (x)) TYPE_CANONICAL (x) = TYPE_CANONICAL (t); - else if (TYPE_CANONICAL (t) != t - || check_qualified_type (x, t, TYPE_QUALS (x))) + else TYPE_CANONICAL (x) = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); - else - TYPE_CANONICAL (x) = x; } else if (x != t) continue; --- gcc/testsuite/gcc.dg/pr114930.c.jj 2024-06-18 21:27:53.782729543 +0200 +++ gcc/testsuite/gcc.dg/pr114930.c 2024-06-18 21:27:53.782729543 +0200 @@ -0,0 +1,9 @@ +/* PR c/114930 */ +/* { dg-do compile { target lto } } */ +/* { dg-options "-std=c23 -flto" } */ + +typedef struct WebPPicture WebPPicture; +typedef int (*WebPProgressHook)(const WebPPicture *); +WebPProgressHook progress_hook; +struct WebPPicture { +} WebPGetColorPalette(const struct WebPPicture *); --- gcc/testsuite/gcc.dg/pr115502.c.jj 2024-06-18 21:27:53.793729408 +0200 +++ gcc/testsuite/gcc.dg/pr115502.c 2024-06-18 21:27:53.793729408 +0200 @@ -0,0 +1,9 @@ +/* PR c/115502 */ +/* { dg-do compile { target lto } } */ +/* { dg-options "-std=c23 -flto" } */ + +typedef struct _OSet OSet; +typedef OSet AvlTree; +void vgPlain_OSetGen_Lookup(const OSet *); +struct _OSet {}; +void vgPlain_OSetGen_Lookup(const AvlTree *); Jakub
Am Mittwoch, dem 19.06.2024 um 08:04 +0200 schrieb Richard Biener: > > > Am 18.06.2024 um 20:18 schrieb Martin Uecker <uecker@tugraz.at>: > > > > Am Dienstag, dem 18.06.2024 um 17:27 +0200 schrieb Richard Biener: > > > > > > > > Am 18.06.2024 um 17:20 schrieb Martin Uecker <uecker@tugraz.at>: > > > > > > > > > > > > As discussed this replaces the use of check_qualified_type with > > > > a simple check for qualifiers as suggested by Jakub in > > > > c_update_type_canonical. > > > > > > Note a canonical type should always be unqualified (for > > > classical qualifiers, not address space or atomic qualification) > > > > The logic in build_qualified_type is the same as in this patch, > > it constructs TYPE_CANONICAL with qualifiers. Or what am I > > missing? > > Nothing if you chose to do TYPE_QUALS (canonical) by copy-pasting. I would rather like to undertand how this work. Is the following correct? For a type T with TYPE_CANONICAL (T) == S, if we construct a qualified type "const" T it would get a const qualified version Q of S as canonical type. S has TYPE_CANONICAL (S) == S and Q has TYPE_CANONICAL (Q) == Q. But the middle end would then ignore this for regular qualifiers and use the TYPE_CANONICAL of the main variant for computing the aliasing set? Setting it differently for qualified types would still be important so that derived types get their own different TYPE_CANONICAL during construction. The other thing I would like to confirm? The alias set is initialized to -1 so set later in the middle end based on TYPE_CANONICAL (if not set to zero before). This is never done before the FE is finished or do we need to worry that this may become inconsistent? Martin > Richard > > > Martin > > > > > > > > Richard > > > > > > > Martin > > > > > > > > > > > > Bootstrapped and regression tested on x86_64. > > > > > > > > > > > > C23: Fix ICE related to incomplete structures [PR114930,PR115502]. > > > > > > > > The fix for PR114574 needs to be further revised because check_qualified_type > > > > makes decision based on TYPE_NAME which can be incorrect for C when there > > > > are TYPE_DECLS involved. > > > > > > > > gcc/c/: > > > > * c-decl.c (c_update_type_canonical): Do not use check_qualified_type. > > > > > > > > gcc/testsuite/: > > > > * gcc.dg/pr114930.c: New test. > > > > * gcc.dg/pr115502.c: New test. > > > > > > > > diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc > > > > index 01326570e2b..610061a07f8 100644 > > > > --- a/gcc/c/c-decl.cc > > > > +++ b/gcc/c/c-decl.cc > > > > @@ -9374,7 +9374,7 @@ c_update_type_canonical (tree t) > > > > if (TYPE_QUALS (x) == TYPE_QUALS (t)) > > > > TYPE_CANONICAL (x) = TYPE_CANONICAL (t); > > > > else if (TYPE_CANONICAL (t) != t > > > > - || check_qualified_type (x, t, TYPE_QUALS (x))) > > > > + || TYPE_QUALS (x) != TYPE_QUALS (TYPE_CANONICAL (t))) > > > > TYPE_CANONICAL (x) > > > > = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); > > > > else > > > > diff --git a/gcc/testsuite/gcc.dg/pr114930.c b/gcc/testsuite/gcc.dg/pr114930.c > > > > new file mode 100644 > > > > index 00000000000..5e982fb8929 > > > > --- /dev/null > > > > +++ b/gcc/testsuite/gcc.dg/pr114930.c > > > > @@ -0,0 +1,9 @@ > > > > +/* { dg-do compile } > > > > + * { dg-options "-std=c23 -flto" } */ > > > > + > > > > +typedef struct WebPPicture WebPPicture; > > > > +typedef int (*WebPProgressHook)(const WebPPicture *); > > > > +WebPProgressHook progress_hook; > > > > +struct WebPPicture { > > > > +} WebPGetColorPalette(const struct WebPPicture *); > > > > + > > > > diff --git a/gcc/testsuite/gcc.dg/pr115502.c b/gcc/testsuite/gcc.dg/pr115502.c > > > > new file mode 100644 > > > > index 00000000000..02b52622c5a > > > > --- /dev/null > > > > +++ b/gcc/testsuite/gcc.dg/pr115502.c > > > > @@ -0,0 +1,9 @@ > > > > +/* { dg-do compile } > > > > + * { dg-options "-std=c23 -flto" } */ > > > > + > > > > +typedef struct _OSet OSet; > > > > +typedef OSet AvlTree; > > > > +void vgPlain_OSetGen_Lookup(const OSet *); > > > > +struct _OSet {}; > > > > +void vgPlain_OSetGen_Lookup(const AvlTree *); > > > > + > > > > > >
Am Mittwoch, dem 19.06.2024 um 08:29 +0200 schrieb Jakub Jelinek: > On Wed, Jun 19, 2024 at 08:04:55AM +0200, Richard Biener wrote: > > > > Note a canonical type should always be unqualified (for > > > > classical qualifiers, not address space or atomic qualification) > > > > > > The logic in build_qualified_type is the same as in this patch, > > > it constructs TYPE_CANONICAL with qualifiers. Or what am I > > > missing? > > > > Nothing if you chose to do TYPE_QUALS (canonical) by copy-pasting. > > As I mentioned earlier, I'd prefer the following version and I have > already tested it on the testcases (note, some changes to the tests, > e.g. using lto effective target), just haven't bootstrapped/regtested it > fully yet. Ok, thanks, I wasn't sure about the conclusion of the discussion. Martin > > gcc/c/ > * c-decl.cc (c_update_type_canonical): Assert t is main variant > with 0 TYPE_QUALS. Simplify and don't use check_qualified_type. > gcc/testsuite/ > * gcc.dg/pr114930.c: New test. > * gcc.dg/pr115502.c: New test. > > --- gcc/c/c-decl.cc.jj 2024-06-07 12:17:09.582969919 +0200 > +++ gcc/c/c-decl.cc 2024-06-18 21:30:22.881904947 +0200 > @@ -9367,18 +9367,16 @@ is_flexible_array_member_p (bool is_last > static void > c_update_type_canonical (tree t) > { > - for (tree x = TYPE_MAIN_VARIANT (t); x; x = TYPE_NEXT_VARIANT (x)) > + gcc_checking_assert (TYPE_MAIN_VARIANT (t) == t && !TYPE_QUALS (t)); > + for (tree x = t; x; x = TYPE_NEXT_VARIANT (x)) > { > if (x != t && TYPE_STRUCTURAL_EQUALITY_P (x)) > { > - if (TYPE_QUALS (x) == TYPE_QUALS (t)) > + if (!TYPE_QUALS (x)) > TYPE_CANONICAL (x) = TYPE_CANONICAL (t); > - else if (TYPE_CANONICAL (t) != t > - || check_qualified_type (x, t, TYPE_QUALS (x))) > + else > TYPE_CANONICAL (x) > = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); > - else > - TYPE_CANONICAL (x) = x; > } > else if (x != t) > continue; > --- gcc/testsuite/gcc.dg/pr114930.c.jj 2024-06-18 21:27:53.782729543 +0200 > +++ gcc/testsuite/gcc.dg/pr114930.c 2024-06-18 21:27:53.782729543 +0200 > @@ -0,0 +1,9 @@ > +/* PR c/114930 */ > +/* { dg-do compile { target lto } } */ > +/* { dg-options "-std=c23 -flto" } */ > + > +typedef struct WebPPicture WebPPicture; > +typedef int (*WebPProgressHook)(const WebPPicture *); > +WebPProgressHook progress_hook; > +struct WebPPicture { > +} WebPGetColorPalette(const struct WebPPicture *); > --- gcc/testsuite/gcc.dg/pr115502.c.jj 2024-06-18 21:27:53.793729408 +0200 > +++ gcc/testsuite/gcc.dg/pr115502.c 2024-06-18 21:27:53.793729408 +0200 > @@ -0,0 +1,9 @@ > +/* PR c/115502 */ > +/* { dg-do compile { target lto } } */ > +/* { dg-options "-std=c23 -flto" } */ > + > +typedef struct _OSet OSet; > +typedef OSet AvlTree; > +void vgPlain_OSetGen_Lookup(const OSet *); > +struct _OSet {}; > +void vgPlain_OSetGen_Lookup(const AvlTree *); > > Jakub >
On Wed, 19 Jun 2024, Martin Uecker wrote: > Am Mittwoch, dem 19.06.2024 um 08:04 +0200 schrieb Richard Biener: > > > > > Am 18.06.2024 um 20:18 schrieb Martin Uecker <uecker@tugraz.at>: > > > > > > Am Dienstag, dem 18.06.2024 um 17:27 +0200 schrieb Richard Biener: > > > > > > > > > > Am 18.06.2024 um 17:20 schrieb Martin Uecker <uecker@tugraz.at>: > > > > > > > > > > > > > > > As discussed this replaces the use of check_qualified_type with > > > > > a simple check for qualifiers as suggested by Jakub in > > > > > c_update_type_canonical. > > > > > > > > Note a canonical type should always be unqualified (for > > > > classical qualifiers, not address space or atomic qualification) > > > > > > The logic in build_qualified_type is the same as in this patch, > > > it constructs TYPE_CANONICAL with qualifiers. Or what am I > > > missing? > > > > Nothing if you chose to do TYPE_QUALS (canonical) by copy-pasting. > > I would rather like to undertand how this work. Is the following > correct? > > For a type T with TYPE_CANONICAL (T) == S, if we construct a qualified > type "const" T it would get a const qualified version Q of S as > canonical type. S has TYPE_CANONICAL (S) == S and Q has > TYPE_CANONICAL (Q) == Q. I think this is a misunderstanding - TYPE_CANONICAL of both T and Q should be the same (S). > But the middle end would then ignore this for regular qualifiers > and use the TYPE_CANONICAL of the main variant for computing > the aliasing set? The middle end uses the canonical type for alias set purposes. The indirection via TYPE_MAIN_VARIANT shouldn't be necessary. > Setting it differently for qualified types would still be important > so that derived types get their own different TYPE_CANONICAL > during construction. > > > The other thing I would like to confirm? The alias set is > initialized to -1 so set later in the middle end based on > TYPE_CANONICAL (if not set to zero before). This is never > done before the FE is finished or do we need to worry that > this may become inconsistent? It used to be that -Wstrict-aliasing calls get_alias_set so TYPE_ALIAS_SET can become set, I don't remember this being avoided so yes, you'd have to worry about this. Note TYPE_ALIAS_SET is only ever considered on the canonical type (but IIRC we have checking code looking at other type alias set). Richard. > Martin > > > Richard > > > > > Martin > > > > > > > > > > > Richard > > > > > > > > > Martin > > > > > > > > > > > > > > > Bootstrapped and regression tested on x86_64. > > > > > > > > > > > > > > > C23: Fix ICE related to incomplete structures [PR114930,PR115502]. > > > > > > > > > > The fix for PR114574 needs to be further revised because check_qualified_type > > > > > makes decision based on TYPE_NAME which can be incorrect for C when there > > > > > are TYPE_DECLS involved. > > > > > > > > > > gcc/c/: > > > > > * c-decl.c (c_update_type_canonical): Do not use check_qualified_type. > > > > > > > > > > gcc/testsuite/: > > > > > * gcc.dg/pr114930.c: New test. > > > > > * gcc.dg/pr115502.c: New test. > > > > > > > > > > diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc > > > > > index 01326570e2b..610061a07f8 100644 > > > > > --- a/gcc/c/c-decl.cc > > > > > +++ b/gcc/c/c-decl.cc > > > > > @@ -9374,7 +9374,7 @@ c_update_type_canonical (tree t) > > > > > if (TYPE_QUALS (x) == TYPE_QUALS (t)) > > > > > TYPE_CANONICAL (x) = TYPE_CANONICAL (t); > > > > > else if (TYPE_CANONICAL (t) != t > > > > > - || check_qualified_type (x, t, TYPE_QUALS (x))) > > > > > + || TYPE_QUALS (x) != TYPE_QUALS (TYPE_CANONICAL (t))) > > > > > TYPE_CANONICAL (x) > > > > > = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); > > > > > else > > > > > diff --git a/gcc/testsuite/gcc.dg/pr114930.c b/gcc/testsuite/gcc.dg/pr114930.c > > > > > new file mode 100644 > > > > > index 00000000000..5e982fb8929 > > > > > --- /dev/null > > > > > +++ b/gcc/testsuite/gcc.dg/pr114930.c > > > > > @@ -0,0 +1,9 @@ > > > > > +/* { dg-do compile } > > > > > + * { dg-options "-std=c23 -flto" } */ > > > > > + > > > > > +typedef struct WebPPicture WebPPicture; > > > > > +typedef int (*WebPProgressHook)(const WebPPicture *); > > > > > +WebPProgressHook progress_hook; > > > > > +struct WebPPicture { > > > > > +} WebPGetColorPalette(const struct WebPPicture *); > > > > > + > > > > > diff --git a/gcc/testsuite/gcc.dg/pr115502.c b/gcc/testsuite/gcc.dg/pr115502.c > > > > > new file mode 100644 > > > > > index 00000000000..02b52622c5a > > > > > --- /dev/null > > > > > +++ b/gcc/testsuite/gcc.dg/pr115502.c > > > > > @@ -0,0 +1,9 @@ > > > > > +/* { dg-do compile } > > > > > + * { dg-options "-std=c23 -flto" } */ > > > > > + > > > > > +typedef struct _OSet OSet; > > > > > +typedef OSet AvlTree; > > > > > +void vgPlain_OSetGen_Lookup(const OSet *); > > > > > +struct _OSet {}; > > > > > +void vgPlain_OSetGen_Lookup(const AvlTree *); > > > > > + > > > > > > > > > >
On Wed, 19 Jun 2024, Jakub Jelinek wrote: > On Wed, Jun 19, 2024 at 08:04:55AM +0200, Richard Biener wrote: > > >> Note a canonical type should always be unqualified (for > > >> classical qualifiers, not address space or atomic qualification) > > > > > > The logic in build_qualified_type is the same as in this patch, > > > it constructs TYPE_CANONICAL with qualifiers. Or what am I > > > missing? > > > > Nothing if you chose to do TYPE_QUALS (canonical) by copy-pasting. > > As I mentioned earlier, I'd prefer the following version and I have > already tested it on the testcases (note, some changes to the tests, > e.g. using lto effective target), just haven't bootstrapped/regtested it > fully yet. LGTM, but I'd defer to a FE maintainer for approval. Richard. > gcc/c/ > * c-decl.cc (c_update_type_canonical): Assert t is main variant > with 0 TYPE_QUALS. Simplify and don't use check_qualified_type. > gcc/testsuite/ > * gcc.dg/pr114930.c: New test. > * gcc.dg/pr115502.c: New test. > > --- gcc/c/c-decl.cc.jj 2024-06-07 12:17:09.582969919 +0200 > +++ gcc/c/c-decl.cc 2024-06-18 21:30:22.881904947 +0200 > @@ -9367,18 +9367,16 @@ is_flexible_array_member_p (bool is_last > static void > c_update_type_canonical (tree t) > { > - for (tree x = TYPE_MAIN_VARIANT (t); x; x = TYPE_NEXT_VARIANT (x)) > + gcc_checking_assert (TYPE_MAIN_VARIANT (t) == t && !TYPE_QUALS (t)); > + for (tree x = t; x; x = TYPE_NEXT_VARIANT (x)) > { > if (x != t && TYPE_STRUCTURAL_EQUALITY_P (x)) > { > - if (TYPE_QUALS (x) == TYPE_QUALS (t)) > + if (!TYPE_QUALS (x)) > TYPE_CANONICAL (x) = TYPE_CANONICAL (t); > - else if (TYPE_CANONICAL (t) != t > - || check_qualified_type (x, t, TYPE_QUALS (x))) > + else > TYPE_CANONICAL (x) > = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); > - else > - TYPE_CANONICAL (x) = x; > } > else if (x != t) > continue; > --- gcc/testsuite/gcc.dg/pr114930.c.jj 2024-06-18 21:27:53.782729543 +0200 > +++ gcc/testsuite/gcc.dg/pr114930.c 2024-06-18 21:27:53.782729543 +0200 > @@ -0,0 +1,9 @@ > +/* PR c/114930 */ > +/* { dg-do compile { target lto } } */ > +/* { dg-options "-std=c23 -flto" } */ > + > +typedef struct WebPPicture WebPPicture; > +typedef int (*WebPProgressHook)(const WebPPicture *); > +WebPProgressHook progress_hook; > +struct WebPPicture { > +} WebPGetColorPalette(const struct WebPPicture *); > --- gcc/testsuite/gcc.dg/pr115502.c.jj 2024-06-18 21:27:53.793729408 +0200 > +++ gcc/testsuite/gcc.dg/pr115502.c 2024-06-18 21:27:53.793729408 +0200 > @@ -0,0 +1,9 @@ > +/* PR c/115502 */ > +/* { dg-do compile { target lto } } */ > +/* { dg-options "-std=c23 -flto" } */ > + > +typedef struct _OSet OSet; > +typedef OSet AvlTree; > +void vgPlain_OSetGen_Lookup(const OSet *); > +struct _OSet {}; > +void vgPlain_OSetGen_Lookup(const AvlTree *); > > Jakub > >
Am Mittwoch, dem 19.06.2024 um 08:57 +0200 schrieb Richard Biener: > On Wed, 19 Jun 2024, Martin Uecker wrote: > > > Am Mittwoch, dem 19.06.2024 um 08:04 +0200 schrieb Richard Biener: > > > > > > > Am 18.06.2024 um 20:18 schrieb Martin Uecker <uecker@tugraz.at>: > > > > > > > > Am Dienstag, dem 18.06.2024 um 17:27 +0200 schrieb Richard Biener: > > > > > > > > > > > > Am 18.06.2024 um 17:20 schrieb Martin Uecker <uecker@tugraz.at>: > > > > > > > > > > > > > > > > > > As discussed this replaces the use of check_qualified_type with > > > > > > a simple check for qualifiers as suggested by Jakub in > > > > > > c_update_type_canonical. > > > > > > > > > > Note a canonical type should always be unqualified (for > > > > > classical qualifiers, not address space or atomic qualification) > > > > > > > > The logic in build_qualified_type is the same as in this patch, > > > > it constructs TYPE_CANONICAL with qualifiers. Or what am I > > > > missing? > > > > > > Nothing if you chose to do TYPE_QUALS (canonical) by copy-pasting. > > > > I would rather like to undertand how this work. Is the following > > correct? > > > > For a type T with TYPE_CANONICAL (T) == S, if we construct a qualified > > type "const" T it would get a const qualified version Q of S as > > canonical type. S has TYPE_CANONICAL (S) == S and Q has > > TYPE_CANONICAL (Q) == Q. > > I think this is a misunderstanding - TYPE_CANONICAL of both T and Q > should be the same (S). Ok. Then should it, instead of TYPE_CANONICAL (x) = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); be tree c = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); TYPE_CANONICAL (x) = TREE_CANONICAL (c); in the patch below? Martin > > > But the middle end would then ignore this for regular qualifiers > > and use the TYPE_CANONICAL of the main variant for computing > > the aliasing set? > > The middle end uses the canonical type for alias set purposes. > The indirection via TYPE_MAIN_VARIANT shouldn't be necessary. > > > Setting it differently for qualified types would still be important > > so that derived types get their own different TYPE_CANONICAL > > during construction. > > > > > > The other thing I would like to confirm? The alias set is > > initialized to -1 so set later in the middle end based on > > TYPE_CANONICAL (if not set to zero before). This is never > > done before the FE is finished or do we need to worry that > > this may become inconsistent? > > It used to be that -Wstrict-aliasing calls get_alias_set so > TYPE_ALIAS_SET can become set, I don't remember this being > avoided so yes, you'd have to worry about this. Note > TYPE_ALIAS_SET is only ever considered on the canonical type > (but IIRC we have checking code looking at other type alias set). > > Richard. > > > Martin > > > > > Richard > > > > > > > Martin > > > > > > > > > > > > > > Richard > > > > > > > > > > > Martin > > > > > > > > > > > > > > > > > > Bootstrapped and regression tested on x86_64. > > > > > > > > > > > > > > > > > > C23: Fix ICE related to incomplete structures [PR114930,PR115502]. > > > > > > > > > > > > The fix for PR114574 needs to be further revised because check_qualified_type > > > > > > makes decision based on TYPE_NAME which can be incorrect for C when there > > > > > > are TYPE_DECLS involved. > > > > > > > > > > > > gcc/c/: > > > > > > * c-decl.c (c_update_type_canonical): Do not use check_qualified_type. > > > > > > > > > > > > gcc/testsuite/: > > > > > > * gcc.dg/pr114930.c: New test. > > > > > > * gcc.dg/pr115502.c: New test. > > > > > > > > > > > > diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc > > > > > > index 01326570e2b..610061a07f8 100644 > > > > > > --- a/gcc/c/c-decl.cc > > > > > > +++ b/gcc/c/c-decl.cc > > > > > > @@ -9374,7 +9374,7 @@ c_update_type_canonical (tree t) > > > > > > if (TYPE_QUALS (x) == TYPE_QUALS (t)) > > > > > > TYPE_CANONICAL (x) = TYPE_CANONICAL (t); > > > > > > else if (TYPE_CANONICAL (t) != t > > > > > > - || check_qualified_type (x, t, TYPE_QUALS (x))) > > > > > > + || TYPE_QUALS (x) != TYPE_QUALS (TYPE_CANONICAL (t))) > > > > > > TYPE_CANONICAL (x) > > > > > > = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); > > > > > > else > > > > > > diff --git a/gcc/testsuite/gcc.dg/pr114930.c b/gcc/testsuite/gcc.dg/pr114930.c > > > > > > new file mode 100644 > > > > > > index 00000000000..5e982fb8929 > > > > > > --- /dev/null > > > > > > +++ b/gcc/testsuite/gcc.dg/pr114930.c > > > > > > @@ -0,0 +1,9 @@ > > > > > > +/* { dg-do compile } > > > > > > + * { dg-options "-std=c23 -flto" } */ > > > > > > + > > > > > > +typedef struct WebPPicture WebPPicture; > > > > > > +typedef int (*WebPProgressHook)(const WebPPicture *); > > > > > > +WebPProgressHook progress_hook; > > > > > > +struct WebPPicture { > > > > > > +} WebPGetColorPalette(const struct WebPPicture *); > > > > > > + > > > > > > diff --git a/gcc/testsuite/gcc.dg/pr115502.c b/gcc/testsuite/gcc.dg/pr115502.c > > > > > > new file mode 100644 > > > > > > index 00000000000..02b52622c5a > > > > > > --- /dev/null > > > > > > +++ b/gcc/testsuite/gcc.dg/pr115502.c > > > > > > @@ -0,0 +1,9 @@ > > > > > > +/* { dg-do compile } > > > > > > + * { dg-options "-std=c23 -flto" } */ > > > > > > + > > > > > > +typedef struct _OSet OSet; > > > > > > +typedef OSet AvlTree; > > > > > > +void vgPlain_OSetGen_Lookup(const OSet *); > > > > > > +struct _OSet {}; > > > > > > +void vgPlain_OSetGen_Lookup(const AvlTree *); > > > > > > + > > > > > > > > > > > > > > >
On Wed, 19 Jun 2024, Martin Uecker wrote: > Am Mittwoch, dem 19.06.2024 um 08:57 +0200 schrieb Richard Biener: > > On Wed, 19 Jun 2024, Martin Uecker wrote: > > > > > Am Mittwoch, dem 19.06.2024 um 08:04 +0200 schrieb Richard Biener: > > > > > > > > > Am 18.06.2024 um 20:18 schrieb Martin Uecker <uecker@tugraz.at>: > > > > > > > > > > Am Dienstag, dem 18.06.2024 um 17:27 +0200 schrieb Richard Biener: > > > > > > > > > > > > > > Am 18.06.2024 um 17:20 schrieb Martin Uecker <uecker@tugraz.at>: > > > > > > > > > > > > > > > > > > > > > As discussed this replaces the use of check_qualified_type with > > > > > > > a simple check for qualifiers as suggested by Jakub in > > > > > > > c_update_type_canonical. > > > > > > > > > > > > Note a canonical type should always be unqualified (for > > > > > > classical qualifiers, not address space or atomic qualification) > > > > > > > > > > The logic in build_qualified_type is the same as in this patch, > > > > > it constructs TYPE_CANONICAL with qualifiers. Or what am I > > > > > missing? > > > > > > > > Nothing if you chose to do TYPE_QUALS (canonical) by copy-pasting. > > > > > > I would rather like to undertand how this work. Is the following > > > correct? > > > > > > For a type T with TYPE_CANONICAL (T) == S, if we construct a qualified > > > type "const" T it would get a const qualified version Q of S as > > > canonical type. S has TYPE_CANONICAL (S) == S and Q has > > > TYPE_CANONICAL (Q) == Q. > > > > I think this is a misunderstanding - TYPE_CANONICAL of both T and Q > > should be the same (S). > > Ok. Then should it, instead of > > TYPE_CANONICAL (x) > = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); That looks indeed weird. What are the constraints on 't' for c_update_type_canonical? If it is TYPE_STRUCTURAL_EQUALITY_P the above will segfault. The docs of the function says /* Recompute TYPE_CANONICAL for variants of the type including qualified versions of the type and related pointer types after an aggregate type has been finalized. but it seems to also possibly update the main variants canonical type. But the main variants canonical type should be the canonical type of all other variants, so I would expect the function to determine the main variant, possibly update (why?) it's canonical type and then assign that to all variant types. Richard. > be > > tree c = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); > TYPE_CANONICAL (x) = TREE_CANONICAL (c); > > in the patch below? > > Martin > > > > > > But the middle end would then ignore this for regular qualifiers > > > and use the TYPE_CANONICAL of the main variant for computing > > > the aliasing set? > > > > The middle end uses the canonical type for alias set purposes. > > The indirection via TYPE_MAIN_VARIANT shouldn't be necessary. > > > > > Setting it differently for qualified types would still be important > > > so that derived types get their own different TYPE_CANONICAL > > > during construction. > > > > > > > > > The other thing I would like to confirm? The alias set is > > > initialized to -1 so set later in the middle end based on > > > TYPE_CANONICAL (if not set to zero before). This is never > > > done before the FE is finished or do we need to worry that > > > this may become inconsistent? > > > > It used to be that -Wstrict-aliasing calls get_alias_set so > > TYPE_ALIAS_SET can become set, I don't remember this being > > avoided so yes, you'd have to worry about this. Note > > TYPE_ALIAS_SET is only ever considered on the canonical type > > (but IIRC we have checking code looking at other type alias set). > > > > Richard. > > > > > Martin > > > > > > > Richard > > > > > > > > > Martin > > > > > > > > > > > > > > > > > Richard > > > > > > > > > > > > > Martin > > > > > > > > > > > > > > > > > > > > > Bootstrapped and regression tested on x86_64. > > > > > > > > > > > > > > > > > > > > > C23: Fix ICE related to incomplete structures [PR114930,PR115502]. > > > > > > > > > > > > > > The fix for PR114574 needs to be further revised because check_qualified_type > > > > > > > makes decision based on TYPE_NAME which can be incorrect for C when there > > > > > > > are TYPE_DECLS involved. > > > > > > > > > > > > > > gcc/c/: > > > > > > > * c-decl.c (c_update_type_canonical): Do not use check_qualified_type. > > > > > > > > > > > > > > gcc/testsuite/: > > > > > > > * gcc.dg/pr114930.c: New test. > > > > > > > * gcc.dg/pr115502.c: New test. > > > > > > > > > > > > > > diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc > > > > > > > index 01326570e2b..610061a07f8 100644 > > > > > > > --- a/gcc/c/c-decl.cc > > > > > > > +++ b/gcc/c/c-decl.cc > > > > > > > @@ -9374,7 +9374,7 @@ c_update_type_canonical (tree t) > > > > > > > if (TYPE_QUALS (x) == TYPE_QUALS (t)) > > > > > > > TYPE_CANONICAL (x) = TYPE_CANONICAL (t); > > > > > > > else if (TYPE_CANONICAL (t) != t > > > > > > > - || check_qualified_type (x, t, TYPE_QUALS (x))) > > > > > > > + || TYPE_QUALS (x) != TYPE_QUALS (TYPE_CANONICAL (t))) > > > > > > > TYPE_CANONICAL (x) > > > > > > > = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); > > > > > > > else > > > > > > > diff --git a/gcc/testsuite/gcc.dg/pr114930.c b/gcc/testsuite/gcc.dg/pr114930.c > > > > > > > new file mode 100644 > > > > > > > index 00000000000..5e982fb8929 > > > > > > > --- /dev/null > > > > > > > +++ b/gcc/testsuite/gcc.dg/pr114930.c > > > > > > > @@ -0,0 +1,9 @@ > > > > > > > +/* { dg-do compile } > > > > > > > + * { dg-options "-std=c23 -flto" } */ > > > > > > > + > > > > > > > +typedef struct WebPPicture WebPPicture; > > > > > > > +typedef int (*WebPProgressHook)(const WebPPicture *); > > > > > > > +WebPProgressHook progress_hook; > > > > > > > +struct WebPPicture { > > > > > > > +} WebPGetColorPalette(const struct WebPPicture *); > > > > > > > + > > > > > > > diff --git a/gcc/testsuite/gcc.dg/pr115502.c b/gcc/testsuite/gcc.dg/pr115502.c > > > > > > > new file mode 100644 > > > > > > > index 00000000000..02b52622c5a > > > > > > > --- /dev/null > > > > > > > +++ b/gcc/testsuite/gcc.dg/pr115502.c > > > > > > > @@ -0,0 +1,9 @@ > > > > > > > +/* { dg-do compile } > > > > > > > + * { dg-options "-std=c23 -flto" } */ > > > > > > > + > > > > > > > +typedef struct _OSet OSet; > > > > > > > +typedef OSet AvlTree; > > > > > > > +void vgPlain_OSetGen_Lookup(const OSet *); > > > > > > > +struct _OSet {}; > > > > > > > +void vgPlain_OSetGen_Lookup(const AvlTree *); > > > > > > > + > > > > > > > > > > > > > > > > > > > > > >
On Wed, Jun 19, 2024 at 09:26:00AM +0200, Martin Uecker wrote: > Ok. Then should it, instead of > > TYPE_CANONICAL (x) > = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); > > be > > tree c = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); > TYPE_CANONICAL (x) = TREE_CANONICAL (c); > > in the patch below? I think you are right, that matches what build_qualified_type does: else if (TYPE_CANONICAL (type) != type) /* Build the underlying canonical type, since it is different from TYPE. */ { tree c = build_qualified_type (TYPE_CANONICAL (type), type_quals); TYPE_CANONICAL (t) = TYPE_CANONICAL (c); } else /* T is its own canonical type. */ TYPE_CANONICAL (t) = t; Jakub
On Wed, Jun 19, 2024 at 09:36:40AM +0200, Richard Biener wrote: > > TYPE_CANONICAL (x) > > = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); > > That looks indeed weird. What are the constraints on 't' for > c_update_type_canonical? If it is TYPE_STRUCTURAL_EQUALITY_P > the above will segfault. > > The docs of the function says > > /* Recompute TYPE_CANONICAL for variants of the type including qualified > versions of the type and related pointer types after an aggregate type > has been finalized. > > but it seems to also possibly update the main variants canonical type. > But the main variants canonical type should be the canonical type of > all other variants, so I would expect the function to determine > the main variant, possibly update (why?) it's canonical type and > then assign that to all variant types. The function is supposed to be called on previously TYPE_STRUCTURAL_EQUALITY_P main variant on which the FE set TYPE_CANONICAL to non-NULL: gcc_checking_assert (TYPE_STRUCTURAL_EQUALITY_P (t)); tree *e = c_struct_htab->find_slot_with_hash (t, hash, INSERT); if (*e) TYPE_CANONICAL (t) = *e; else { TYPE_CANONICAL (t) = t; *e = t; } c_update_type_canonical (t); and the function should propagate that canonical type or canonical types derived from that also to its variants (qualified or unqualified) and pointers to those as long as the variants/pointers are still TYPE_STRUCTURAL_EQUALITY_P before the change. Jakub
On Wed, Jun 19, 2024 at 09:26:00AM +0200, Martin Uecker wrote: > Ok. Then should it, instead of > > TYPE_CANONICAL (x) > = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); > > be > > tree c = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); > TYPE_CANONICAL (x) = TREE_CANONICAL (c); > > in the patch below? Ok, I've tried that, but that doesn't work, it ICEs on the pr114574-2.c testcase. What happens is that TYPE_CANONICAL (t) = t; is set by the caller and the first loop first sees x == t, so only handles pointers, and then moves to const struct S. As TYPE_CANONICAL (t) is t, build_qualified_type looks for an existing qualified type (and, not just that, it also moves the found qualified type to TYPE_NEXT_VARIANT (t) to speed it up next time!!), in this case returns x, so it then effectively does nothing, TYPE_CANONICAL (x) = TYPE_CANONICAL (x); and leaves the type TYPE_STRUCTURAL_EQUALITY_P, which is not what we want. Dunno if it in theory could also find some type later in the TYPE_NEXT_VARIANT chain and move it earlier. So, if the new patch is to be used, we need to add some extra handling for these problematic cases. One is if c == x (which can happen solely if TYPE_CANONICAL (t) == t), that is easy to handle, in that case we should make it the canonical type of itself, so TYPE_CANONICAL (x) = x; rather than TYPE_CANONICAL (x) = TYPE_CANONICAL (c). And then there is the theoretical case that c is some type from the TYPE_MAIN_VARIANT chain which we haven't processed yet. And that build_qualified_type moved it to the second position in the chain even when we haven't processed that yet. For that, I think we need to first process that c and only then restart handling of x. So, either we could: gcc_checking_assert (TYPE_MAIN_VARIANT (t) == t && !TYPE_QUALS (t)); for (tree x = t; x; x = TYPE_NEXT_VARIANT (x)) { if (x != t && TYPE_STRUCTURAL_EQUALITY_P (x)) { if (!TYPE_QUALS (x)) TYPE_CANONICAL (x) = TYPE_CANONICAL (t); else { tree c = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); if (TYPE_STRUCTURAL_EQUALITY_P (c)) { gcc_checking_assert (TYPE_CANONICAL (t) == t); if (c == x) TYPE_CANONICAL (x) = x; else { /* build_qualified_type unhelpfully moved c from some later spot in TYPE_MAIN_VARIANT (t) chain to right after t. Restart processing the whole chain. */ gcc_checking_assert (TYPE_MAIN_VARIANT (t) == c); x = t; continue; } } else TYPE_CANONICAL (x) = TYPE_CANONICAL (c); } } ... but that could walk perhaps very long chain over and over (sure, for the already handled cases it would see TYPE_STRUCTUAL_EQUALITY_P (x) is no longer the case, but still, I'm afraid it increases compile time complexity for pathological cases too much. Or perhaps undo what get_qualified_type's /* Put the found variant at the head of the variant list so frequently searched variants get found faster. The C++ FE benefits greatly from this. */ tree t = *tp; *tp = TYPE_NEXT_VARIANT (t); TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (mv); TYPE_NEXT_VARIANT (mv) = t; return t; optimization or build_variant_type_copy clearly as well (it chains new types to TYPE_NEXT_VARIANT (mv) as well). So perhaps instead we need to undo the move. Here is what I've bootstrapped/regtested and what broke pr114574-2.c: gcc/c/ * c-decl.cc (c_update_type_canonical): Assert t is main variant with 0 TYPE_QUALS. Simplify and don't use check_qualified_type. gcc/testsuite/ * gcc.dg/pr114930.c: New test. * gcc.dg/pr115502.c: New test. --- gcc/c/c-decl.cc.jj 2024-06-07 12:17:09.582969919 +0200 +++ gcc/c/c-decl.cc 2024-06-19 13:35:46.648956792 +0200 @@ -9367,18 +9367,19 @@ is_flexible_array_member_p (bool is_last static void c_update_type_canonical (tree t) { - for (tree x = TYPE_MAIN_VARIANT (t); x; x = TYPE_NEXT_VARIANT (x)) + gcc_checking_assert (TYPE_MAIN_VARIANT (t) == t && !TYPE_QUALS (t)); + for (tree x = t; x; x = TYPE_NEXT_VARIANT (x)) { if (x != t && TYPE_STRUCTURAL_EQUALITY_P (x)) { - if (TYPE_QUALS (x) == TYPE_QUALS (t)) + if (!TYPE_QUALS (x)) TYPE_CANONICAL (x) = TYPE_CANONICAL (t); - else if (TYPE_CANONICAL (t) != t - || check_qualified_type (x, t, TYPE_QUALS (x))) - TYPE_CANONICAL (x) - = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); else - TYPE_CANONICAL (x) = x; + { + tree + c = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); + TYPE_CANONICAL (x) = TYPE_CANONICAL (c); + } } else if (x != t) continue; --- gcc/testsuite/gcc.dg/pr114930.c.jj 2024-06-18 21:27:53.782729543 +0200 +++ gcc/testsuite/gcc.dg/pr114930.c 2024-06-18 21:27:53.782729543 +0200 @@ -0,0 +1,9 @@ +/* PR c/114930 */ +/* { dg-do compile { target lto } } */ +/* { dg-options "-std=c23 -flto" } */ + +typedef struct WebPPicture WebPPicture; +typedef int (*WebPProgressHook)(const WebPPicture *); +WebPProgressHook progress_hook; +struct WebPPicture { +} WebPGetColorPalette(const struct WebPPicture *); --- gcc/testsuite/gcc.dg/pr115502.c.jj 2024-06-18 21:27:53.793729408 +0200 +++ gcc/testsuite/gcc.dg/pr115502.c 2024-06-18 21:27:53.793729408 +0200 @@ -0,0 +1,9 @@ +/* PR c/115502 */ +/* { dg-do compile { target lto } } */ +/* { dg-options "-std=c23 -flto" } */ + +typedef struct _OSet OSet; +typedef OSet AvlTree; +void vgPlain_OSetGen_Lookup(const OSet *); +struct _OSet {}; +void vgPlain_OSetGen_Lookup(const AvlTree *); Jakub
On Wed, Jun 19, 2024 at 07:32:28PM +0200, Jakub Jelinek wrote: > Ok, I've tried that, but that doesn't work, it ICEs on the > pr114574-2.c testcase. The following works on quick testing of dg.exp=pr11[45]*.c but haven't bootstrapped/regtested it yet. 2024-06-19 Jakub Jelinek <jakub@redhat.com> Martin Uecker <uecker@tugraz.at> PR c/114930 PR c/115502 gcc/c/ * c-decl.cc (c_update_type_canonical): Assert t is main variant with 0 TYPE_QUALS. Simplify and don't use check_qualified_type. Deal with the case where build_qualified_type returns TYPE_STRUCTURAL_EQUALITY_P type. gcc/testsuite/ * gcc.dg/pr114574-1.c: Require lto effective target. * gcc.dg/pr114574-2.c: Likewise. * gcc.dg/pr114930.c: New test. * gcc.dg/pr115502.c: New test. --- gcc/c/c-decl.cc.jj 2024-06-07 12:17:09.582969919 +0200 +++ gcc/c/c-decl.cc 2024-06-19 19:59:24.955836263 +0200 @@ -9367,18 +9367,44 @@ is_flexible_array_member_p (bool is_last static void c_update_type_canonical (tree t) { - for (tree x = TYPE_MAIN_VARIANT (t); x; x = TYPE_NEXT_VARIANT (x)) + gcc_checking_assert (TYPE_MAIN_VARIANT (t) == t && !TYPE_QUALS (t)); + for (tree x = t, l = NULL_TREE; x; l = x, x = TYPE_NEXT_VARIANT (x)) { if (x != t && TYPE_STRUCTURAL_EQUALITY_P (x)) { - if (TYPE_QUALS (x) == TYPE_QUALS (t)) + if (!TYPE_QUALS (x)) TYPE_CANONICAL (x) = TYPE_CANONICAL (t); - else if (TYPE_CANONICAL (t) != t - || check_qualified_type (x, t, TYPE_QUALS (x))) - TYPE_CANONICAL (x) - = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); else - TYPE_CANONICAL (x) = x; + { + tree + c = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); + if (TYPE_STRUCTURAL_EQUALITY_P (c)) + { + gcc_checking_assert (TYPE_CANONICAL (t) == t); + if (c == x) + TYPE_CANONICAL (x) = x; + else + { + /* build_qualified_type for this function unhelpfully + moved c from some later spot in TYPE_MAIN_VARIANT (t) + chain to right after t (or created it there). Move + it right before x and process c and then x. */ + gcc_checking_assert (TYPE_NEXT_VARIANT (t) == c); + if (l == t) + x = t; + else + { + TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (c); + TYPE_NEXT_VARIANT (l) = c; + TYPE_NEXT_VARIANT (c) = x; + x = l; + } + continue; + } + } + else + TYPE_CANONICAL (x) = TYPE_CANONICAL (c); + } } else if (x != t) continue; --- gcc/testsuite/gcc.dg/pr114574-1.c.jj 2024-04-20 00:05:07.273690453 +0200 +++ gcc/testsuite/gcc.dg/pr114574-1.c 2024-06-19 20:00:33.015984692 +0200 @@ -1,6 +1,6 @@ -/* PR lto/114574 - * { dg-do compile } - * { dg-options "-flto" } */ +/* PR lto/114574 */ +/* { dg-do compile { target lto } } */ +/* { dg-options "-flto" } */ const struct S * x; struct S {}; --- gcc/testsuite/gcc.dg/pr114574-2.c.jj 2024-04-20 00:05:07.274690440 +0200 +++ gcc/testsuite/gcc.dg/pr114574-2.c 2024-06-19 20:00:55.084704278 +0200 @@ -1,6 +1,6 @@ -/* PR lto/114574 - * { dg-do compile } - * { dg-options "-flto -std=c23" } */ +/* PR lto/114574 */ +/* { dg-do compile { target lto } } */ +/* { dg-options "-flto -std=c23" } */ const struct S * x; struct S {}; --- gcc/testsuite/gcc.dg/pr114930.c.jj 2024-06-18 21:27:53.782729543 +0200 +++ gcc/testsuite/gcc.dg/pr114930.c 2024-06-18 21:27:53.782729543 +0200 @@ -0,0 +1,9 @@ +/* PR c/114930 */ +/* { dg-do compile { target lto } } */ +/* { dg-options "-std=c23 -flto" } */ + +typedef struct WebPPicture WebPPicture; +typedef int (*WebPProgressHook)(const WebPPicture *); +WebPProgressHook progress_hook; +struct WebPPicture { +} WebPGetColorPalette(const struct WebPPicture *); --- gcc/testsuite/gcc.dg/pr115502.c.jj 2024-06-18 21:27:53.793729408 +0200 +++ gcc/testsuite/gcc.dg/pr115502.c 2024-06-18 21:27:53.793729408 +0200 @@ -0,0 +1,9 @@ +/* PR c/115502 */ +/* { dg-do compile { target lto } } */ +/* { dg-options "-std=c23 -flto" } */ + +typedef struct _OSet OSet; +typedef OSet AvlTree; +void vgPlain_OSetGen_Lookup(const OSet *); +struct _OSet {}; +void vgPlain_OSetGen_Lookup(const AvlTree *); Jakub
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc index 01326570e2b..610061a07f8 100644 --- a/gcc/c/c-decl.cc +++ b/gcc/c/c-decl.cc @@ -9374,7 +9374,7 @@ c_update_type_canonical (tree t) if (TYPE_QUALS (x) == TYPE_QUALS (t)) TYPE_CANONICAL (x) = TYPE_CANONICAL (t); else if (TYPE_CANONICAL (t) != t - || check_qualified_type (x, t, TYPE_QUALS (x))) + || TYPE_QUALS (x) != TYPE_QUALS (TYPE_CANONICAL (t))) TYPE_CANONICAL (x) = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x)); else diff --git a/gcc/testsuite/gcc.dg/pr114930.c b/gcc/testsuite/gcc.dg/pr114930.c new file mode 100644 index 00000000000..5e982fb8929 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr114930.c @@ -0,0 +1,9 @@ +/* { dg-do compile } + * { dg-options "-std=c23 -flto" } */ + +typedef struct WebPPicture WebPPicture; +typedef int (*WebPProgressHook)(const WebPPicture *); +WebPProgressHook progress_hook; +struct WebPPicture { +} WebPGetColorPalette(const struct WebPPicture *); + diff --git a/gcc/testsuite/gcc.dg/pr115502.c b/gcc/testsuite/gcc.dg/pr115502.c new file mode 100644 index 00000000000..02b52622c5a --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr115502.c @@ -0,0 +1,9 @@ +/* { dg-do compile } + * { dg-options "-std=c23 -flto" } */ + +typedef struct _OSet OSet; +typedef OSet AvlTree; +void vgPlain_OSetGen_Lookup(const OSet *); +struct _OSet {}; +void vgPlain_OSetGen_Lookup(const AvlTree *); +
As discussed this replaces the use of check_qualified_type with a simple check for qualifiers as suggested by Jakub in c_update_type_canonical. Martin Bootstrapped and regression tested on x86_64. C23: Fix ICE related to incomplete structures [PR114930,PR115502]. The fix for PR114574 needs to be further revised because check_qualified_type makes decision based on TYPE_NAME which can be incorrect for C when there are TYPE_DECLS involved. gcc/c/: * c-decl.c (c_update_type_canonical): Do not use check_qualified_type. gcc/testsuite/: * gcc.dg/pr114930.c: New test. * gcc.dg/pr115502.c: New test.