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 <uec...@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 <uec...@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 *);
> > > > > > > +
> > > > > > >
> > > > >
> > >
> > >
> >
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
