Re: c: ignore initializers for elements of variable-size types [PR93577]

[Christophe Lyon <christophe.lyon@linaro.org>]

On Mon, 16 Mar 2020 at 19:59, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Christophe Lyon via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > On Wed, 11 Mar 2020 at 00:37, Joseph Myers <joseph@codesourcery.com> wrote:
> >>
> >> On Tue, 10 Mar 2020, Christophe Lyon wrote:
> >>
> >> > sizeless-1.c and sizeless-2.c have the same code, but the latter is
> >> > compiled with -msve-vector-bits=256 and expects different
> >> > warnings/errors.
> >> > For line 33:
> >> > svint8_t *invalid_sve_sc_ptr = &(svint8_t) { *global_sve_sc_ptr };
> >> > we now have:
> >> > sizeless-1.c:33:44: error: empty scalar initializer
> >> > sizeless-1.c:33:44: note: (near initialization for '(anonymous)')
> >> > and
> >> > sizeless-2.c:33:44: error: initializer element is not constant
> >> > sizeless-2.c:33:44: note: (near initialization for 'invalid_sve_sc_ptr')
> >> > sizeless-2.c:33:44: error: SVE type 'svint8_t' does not have a fixed size
> >> > so I think the error comes from the compound literal being treated
> >> > differently with -msve-vector-bits=256
> >>
> >> I think the sizeless-2.c diagnostics are correct while there's a problem
> >> in the sizeless-1.c case (the initializer is not empty, so it should not
> >> be diagnosed as such).
> >>
> >> Does the process_init_element code
> >>
> >>   /* Ignore elements of an initializer for a variable-size type.
> >>      Those are diagnosed in digest_init.  */
> >>   if (COMPLETE_TYPE_P (constructor_type)
> >>       && TREE_CODE (TYPE_SIZE (constructor_type)) != INTEGER_CST)
> >>     return;
> >>
> >> fire for the sizeless-1.c case?  If so, maybe it needs to be restricted in
> >> some way to apply only to variable size structs / unions / arrays rather
> >> than whatever kind of variable-size type the SVE types are.
> >
> > It does. Type_size has POLY_INT_CST type.
> >
> > The attached small patch fixes the problem (tested on arm and aarch64).
> > OK?
>
> Thanks for doing this.  I'd wondered whether it should be based on
> this or on VECTOR_TYPE_P, but FWIW, I agree basing it on this is best.
>
> > diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
> > index d8025de..1302486 100644
> > --- a/gcc/c/c-typeck.c
> > +++ b/gcc/c/c-typeck.c
> > @@ -9968,7 +9968,8 @@ process_init_element (location_t loc, struct
> > c_expr value, bool implicit,
> >    /* Ignore elements of an initializer for a variable-size type.
> >       Those are diagnosed in digest_init.  */
> >    if (COMPLETE_TYPE_P (constructor_type)
> > -      && TREE_CODE (TYPE_SIZE (constructor_type)) != INTEGER_CST)
> > +      && TREE_CODE (TYPE_SIZE (constructor_type)) != INTEGER_CST
> > +      && TREE_CODE (TYPE_SIZE (constructor_type)) != POLY_INT_CST)
>
> Not worth respinning over, but since it hasn't been applied yet:
> a shorter alternative is to replace the != INTEGER_CST test with
> !poly_int_tree_p.
>
> > diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-1.c
> > b/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-1.c
> > index ec892a3..e53b871 100644
> > --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-1.c
> > +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-1.c
> > @@ -83,7 +83,6 @@ statements (int n)
> >    svint8_t array[2]; /* { dg-error {array elements cannot have SVE
> > type 'svint8_t'} } */
> >    svint8_t zero_length_array[0]; /* { dg-error {array elements cannot
> > have SVE type 'svint8_t'} } */
> >    svint8_t empty_init_array[] = {}; /* { dg-error {array elements
> > cannot have SVE type 'svint8_t'} } */
> > -                                   /* { dg-error {empty scalar
> > initializer} "" { target *-*-* } .-1 } */
> >    typedef svint8_t vla_type[n]; /* { dg-error {array elements cannot
> > have SVE type 'svint8_t'} } */
> >
> >    /* Assignment.  */
> > diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-2.c
> > b/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-2.c
> > index 7174393..9986d27 100644
> > --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-2.c
> > +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-2.c
> > @@ -83,7 +83,6 @@ statements (int n)
> >    svint8_t array[2]; /* { dg-error {array elements cannot have SVE
> > type 'svint8_t'} } */
> >    svint8_t zero_length_array[0]; /* { dg-error {array elements cannot
> > have SVE type 'svint8_t'} } */
> >    svint8_t empty_init_array[] = {}; /* { dg-error {array elements
> > cannot have SVE type 'svint8_t'} } */
> > -                                   /* { dg-error {empty scalar
> > initializer} "" { target *-*-* } .-1 } */
> >    typedef svint8_t vla_type[n]; /* { dg-error {array elements cannot
> > have SVE type 'svint8_t'} } */
> >
> >    /* Assignment.  */
>
> Jeff pointed out off-list that using:
>
>    N:  ... /* { dg-error {...} } */
>  N+1:      /* { dg-error {...} "" { target *-*-* } .-1 } */
>
> led to two identical test names for line N.  This was causing the
> results to fluctuate when using contrib/compare_tests (which I admit
> I don't do, so hadn't noticed).  Your patch fixes the cases that
> mattered, but for future-proofing reasons, this patch adds proper
> test names for the remaining instances.
>

Thanks.

Just checked, there are many more testcases with duplicate "names"
(266 under gcc.target/aarch64 only) :-(

Sigh...

> Tested on aarch64-linux-gnu and committed as obvious.
>
> Richard
>
>
> 2020-03-16  Richard Sandiford  <richard.sandiford@arm.com>
>
> gcc/testsuite/
>         * gcc.target/aarch64/sve/acle/general-c/sizeless-1.c: Add a test
>         name to .-1 dg-error tests.
>         * gcc.target/aarch64/sve/acle/general-c/sizeless-2.c: Likewise.
> ---
>  .../gcc.target/aarch64/sve/acle/general-c/sizeless-1.c          | 2 +-
>  .../gcc.target/aarch64/sve/acle/general-c/sizeless-2.c          | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-1.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-1.c
> index ec892a3fc83..045963d5c76 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-1.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-1.c
> @@ -31,7 +31,7 @@ union union1 {
>
>  svint8_t *global_sve_sc_ptr;
>  svint8_t *invalid_sve_sc_ptr = &(svint8_t) { *global_sve_sc_ptr }; /* { dg-error {initializer element is not constant} } */
> -  /* { dg-error {SVE type 'svint8_t' does not have a fixed size} "" { target *-*-* } .-1 } */
> +  /* { dg-error {SVE type 'svint8_t' does not have a fixed size} "2nd line" { target *-*-* } .-1 } */
>
>  /* Sizeless arguments and return values.  */
>
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-2.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-2.c
> index 71743930098..c7282faba46 100644
> --- a/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-2.c
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general-c/sizeless-2.c
> @@ -31,7 +31,7 @@ union union1 {
>
>  svint8_t *global_sve_sc_ptr;
>  svint8_t *invalid_sve_sc_ptr = &(svint8_t) { *global_sve_sc_ptr }; /* { dg-error {initializer element is not constant} } */
> -  /* { dg-error {SVE type 'svint8_t' does not have a fixed size} "" { target *-*-* } .-1 } */
> +  /* { dg-error {SVE type 'svint8_t' does not have a fixed size} "2nd line" { target *-*-* } .-1 } */
>
>  /* Sizeless arguments and return values.  */
>