Re: [PATCH] c, v3: Fix ICE with -g and -std=c23 related to incomplete types [PR114361]
On Mon, 15 Apr 2024, Jakub Jelinek wrote: > 2024-04-15 Martin Uecker > Jakub Jelinek > > PR lto/114574 > PR c/114361 > 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. OK. -- Joseph S. Myers josmy...@redhat.com
Re: [PATCH] c, v3: Fix ICE with -g and -std=c23 related to incomplete types [PR114361]
On Mon, Apr 15, 2024 at 01:33:18PM +0200, Richard Biener wrote:
> > But looking at all the build_variant_type_copy callers and the call itself,
> > the call itself sets TYPE_CANONICAL to the TYPE_CANONICAL of the type on
> > which it is called and the only caller I can find that changes
> > TYPE_CANONICAL sometimes is build_qualified_type.
> > So, I'd hope that normally all variant types of an aggregate type (or
> > pointer type) have the same TYPE_CANONICAL if they have the same TYPE_QUALS
> > and if they have it different, they have TYPE_CANONICAL of
> > build_qualified_type of the base TYPE_CANONICAL.
>
> The middle-end assumes that TYPE_CANONICAL of all variant types are
> the same, for TBAA purposes it immediately "puns" to
> TYPE_CANONICAL (TYPE_MAIN_VARIANT (..)). It also assumes that
> the canonical type is not a variant type. Note we never "honor"
> TYPE_STRUCTURAL_EQUALITY_P on a variant type (because we don't look
> at it, we only look at whether the main variant has
> TYPE_STRUCTURAL_EQUALITY_P).
>
> Thus, TYPE_CANONICAL of variant types in principle doesn't need to be
> set (but not all places might go the extra step looking at the main
> variant before accessing TYPE_CANONICAL).
The patch as posted passed bootstrap/regtest.
Even if the middle-end doesn't look at TYPE_CANONICAL on anything but
TYPE_MAIN_VARIANT, for the case of say POINTER_TYPE to TYPE_READONLY
qualified struct (like struct S const *) the pointer still has
TYPE_CANONICAL based on the qualified type rather than main variant,
so not recomputing TYPE_CANONICAL for the variant would have consequences
on TYPE_CANONICAL for types derived from those variant types.
So, if I have
struct S;
const struct S *p;
struct S { int s; };
const struct S q[10];
then if I don't recompute TYPE_CANONICAL for const struct S, both the
const struct S * TYPE_CANONICAL would remain TYPE_STRUCTURAL_EQUALITY_P,
but also the ARRAY_TYPE would have it as well, even when it has been
created only after finalizing the type.
Jakub
Re: [PATCH] c, v3: 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 10:05:58AM +0200, Jakub Jelinek wrote:
> > 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.
>
> Actually no, I've just screwed up something in my testing.
> One can reproduce it easily with -O2 -flto 20021205-1.c -std=c23
> if the ipa-free-lang-data.cc hunk is removed.
> This happens when fld_incomplete_type_of is called on a POINTER_TYPE
> to RECORD_TYPE x, where the RECORD_TYPE x is not the TYPE_MAIN_VARIANT,
> but another variant created by set_underlying_type. The
> c_update_type_canonical didn't touch TYPE_CANONICAL in those, I was too
> afraid I don't know what TYPE_CANONICAL should be for all variant types,
> so that TREE_TYPE (t) had TYPE_CANONICAL NULL. But when we call
> fld_incomplete_type_of on that TREE_TYPE (t), it sees it isn't
> TYPE_MAIN_VARIANT, so calls
> return (fld_type_variant
> (fld_incomplete_type_of (TYPE_MAIN_VARIANT (t), fld), t, fld));
> but TYPE_MAIN_VARIANT (t) has already TYPE_CANONICAL (TYPE_MAIN_VARIANT (t))
> == TYPE_MAIN_VARIANT (t), that one has been completed on finish_struct.
> And so we trigger the assertion, because
> TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))
> is no longer true, the former is non-NULL, the latter is NULL.
>
> But looking at all the build_variant_type_copy callers and the call itself,
> the call itself sets TYPE_CANONICAL to the TYPE_CANONICAL of the type on
> which it is called and the only caller I can find that changes
> TYPE_CANONICAL sometimes is build_qualified_type.
> So, I'd hope that normally all variant types of an aggregate type (or
> pointer type) have the same TYPE_CANONICAL if they have the same TYPE_QUALS
> and if they have it different, they have TYPE_CANONICAL of
> build_qualified_type of the base TYPE_CANONICAL.
The middle-end assumes that TYPE_CANONICAL of all variant types are
the same, for TBAA purposes it immediately "puns" to
TYPE_CANONICAL (TYPE_MAIN_VARIANT (..)). It also assumes that
the canonical type is not a variant type. Note we never "honor"
TYPE_STRUCTURAL_EQUALITY_P on a variant type (because we don't look
at it, we only look at whether the main variant has
TYPE_STRUCTURAL_EQUALITY_P).
Thus, TYPE_CANONICAL of variant types in principle doesn't need to be
set (but not all places might go the extra step looking at the main
variant before accessing TYPE_CANONICAL).
Richard.
> With the following updated patch (ipa-free-lang-data.cc hunk removed,
> c_update_type_canonical function updated, plus removed trailing whitespace
> from tests),
> make check-gcc RUNTESTFLAGS="--target_board=unix/-std=gnu23
> compile.exp='20021205-1.c 20040214-2.c 20060109-1.c pr113623.c pr46866.c
> pta-1.c' execute.exp='pr33870-1.c pr33870.c'"
> no longer ICEs (have just expected FAILs on 20040214-2.c which isn't
> compatible with C23) and make check-gcc -j32 doesn't regress compared
> to the unpatched one.
>
> Is this ok for trunk if it passes full bootstrap/regtest?
>
> 2024-04-15 Martin Uecker
> Jakub Jelinek
>
> PR lto/114574
> PR c/114361
> 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.jj2024-04-09 09:29:04.824520299 +0200
> +++ gcc/c/c-decl.cc 2024-04-15 12:26:43.000790475 +0200
> @@ -5051,6 +5051,8 @@ shadow_tag_warned (const struct c_declsp
> 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 tr
> the
[PATCH] c, v3: Fix ICE with -g and -std=c23 related to incomplete types [PR114361]
On Mon, Apr 15, 2024 at 10:05:58AM +0200, Jakub Jelinek wrote:
> 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.
Actually no, I've just screwed up something in my testing.
One can reproduce it easily with -O2 -flto 20021205-1.c -std=c23
if the ipa-free-lang-data.cc hunk is removed.
This happens when fld_incomplete_type_of is called on a POINTER_TYPE
to RECORD_TYPE x, where the RECORD_TYPE x is not the TYPE_MAIN_VARIANT,
but another variant created by set_underlying_type. The
c_update_type_canonical didn't touch TYPE_CANONICAL in those, I was too
afraid I don't know what TYPE_CANONICAL should be for all variant types,
so that TREE_TYPE (t) had TYPE_CANONICAL NULL. But when we call
fld_incomplete_type_of on that TREE_TYPE (t), it sees it isn't
TYPE_MAIN_VARIANT, so calls
return (fld_type_variant
(fld_incomplete_type_of (TYPE_MAIN_VARIANT (t), fld), t, fld));
but TYPE_MAIN_VARIANT (t) has already TYPE_CANONICAL (TYPE_MAIN_VARIANT (t))
== TYPE_MAIN_VARIANT (t), that one has been completed on finish_struct.
And so we trigger the assertion, because
TYPE_CANONICAL (t2) == TYPE_CANONICAL (TREE_TYPE (t))
is no longer true, the former is non-NULL, the latter is NULL.
But looking at all the build_variant_type_copy callers and the call itself,
the call itself sets TYPE_CANONICAL to the TYPE_CANONICAL of the type on
which it is called and the only caller I can find that changes
TYPE_CANONICAL sometimes is build_qualified_type.
So, I'd hope that normally all variant types of an aggregate type (or
pointer type) have the same TYPE_CANONICAL if they have the same TYPE_QUALS
and if they have it different, they have TYPE_CANONICAL of
build_qualified_type of the base TYPE_CANONICAL.
With the following updated patch (ipa-free-lang-data.cc hunk removed,
c_update_type_canonical function updated, plus removed trailing whitespace
from tests),
make check-gcc RUNTESTFLAGS="--target_board=unix/-std=gnu23
compile.exp='20021205-1.c 20040214-2.c 20060109-1.c pr113623.c pr46866.c
pta-1.c' execute.exp='pr33870-1.c pr33870.c'"
no longer ICEs (have just expected FAILs on 20040214-2.c which isn't
compatible with C23) and make check-gcc -j32 doesn't regress compared
to the unpatched one.
Is this ok for trunk if it passes full bootstrap/regtest?
2024-04-15 Martin Uecker
Jakub Jelinek
PR lto/114574
PR c/114361
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.jj 2024-04-09 09:29:04.824520299 +0200
+++ gcc/c/c-decl.cc 2024-04-15 12:26:43.000790475 +0200
@@ -5051,6 +5051,8 @@ shadow_tag_warned (const struct c_declsp
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 tr
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_
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,45 @@ is_flexible_array_member_p (bool is_last
return false;
}
+/* Recompute TYPE_CANONICAL for variants of the type including qualified
+ versions of the type and related pointer types after an aggregate type
+ has
