Re: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

Re: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

[Uecker, Martin]

> On Sun, Aug 1, 2021 at 7:37 PM Uecker, Martin
> <Martin.Uecker@med.uni-goettingen.de> wrote:
> >
> >
> >
> > Here is an attempt to fix some old and annoying bugs related
> > to VLAs and statement expressions. In particulary, this seems
> > to fix the issues with variably-modified types which are
> > returned from statement expressions (which works on clang),
> > but there are still bugs remaining related to structs
> > with VLA members (which seems to be a FE bug).
> >
> > Of course, I might be doing something stupid...
> 
> How's evaluation order of (f())[g()] defined (with f returning a
> pointer)?
> Isn't that just f() + g()*sizeof(int) and thus undefined?

Yes, in C it is

f() + g()

and it is unsequenced. But the order of 'f' and 'g'
is not relevant here and also the patch does not change 
it (the base expression is gimplified before the index).

Essentially, we have

({ ... }) + g() * sizeof(X) 

where X refers to a declaration in the statement expression.
Without the patch the size expressions are gimplified before
the base expression and also before the index expression. 
With the patch the ({ ... }) is gimplified also before the
size expression.

> If it's undefined then I think the incoming GENERIC is ill-defined.

I think it is OK because the arguments are evaluated 
before the operation.  Without the patch, parts of the 
operation (the size expressions) are gimplified before
the arguments and this seems wrong to me.


Martin

Re: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

[Uecker, Martin]

Am Montag, den 02.08.2021, 16:05 +0200 schrieb Martin Uecker:
> > On Sun, Aug 1, 2021 at 7:37 PM Uecker, Martin
> > <Martin.Uecker@med.uni-goettingen.de> wrote:
> > > 
> > > 
> > > Here is an attempt to fix some old and annoying bugs related
> > > to VLAs and statement expressions. In particulary, this seems
> > > to fix the issues with variably-modified types which are
> > > returned from statement expressions (which works on clang),
> > > but there are still bugs remaining related to structs
> > > with VLA members (which seems to be a FE bug).
> > > 
> > > Of course, I might be doing something stupid...
> > 
> > How's evaluation order of (f())[g()] defined (with f returning a
> > pointer)?
> > Isn't that just f() + g()*sizeof(int) and thus undefined?
> 
> Yes, in C it is
> 
> f() + g()
> 
> and it is unsequenced. But the order of 'f' and 'g'
> is not relevant here and also the patch does not change 
> it (the base expression is gimplified before the index).
> 
> Essentially, we have
> 
> ({ ... }) + g() * sizeof(X) 
> 
> where X refers to a declaration in the statement expression.
> Without the patch the size expressions are gimplified before
> the base expression and also before the index expression. 
> With the patch the ({ ... }) is gimplified also before the
> size expression.
> 
> > If it's undefined then I think the incoming GENERIC is ill-defined.
> 
> I think it is OK because the arguments are evaluated 
> before the operation.  Without the patch, parts of the 
> operation (the size expressions) are gimplified before
> the arguments and this seems wrong to me.

If I rewrite the ARRAY_REFs into *(f + g) in the test
cases, they also works with unpatched GCC. So it is really
the incorrect ordering in the gimplification of the
ARRAY_REF which is the problem.


Martin

Re [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

[Uecker, Martin]

Am Montag, den 02.08.2021, 16:19 +0200 schrieb Martin Uecker:
> 
> Am Montag, den 02.08.2021, 16:05 +0200 schrieb Martin Uecker:
> > > On Sun, Aug 1, 2021 at 7:37 PM Uecker, Martin
> > > <Martin.Uecker@med.uni-goettingen.de> wrote:
> > > > 
> > > > Here is an attempt to fix some old and annoying bugs related
> > > > to VLAs and statement expressions. In particulary, this seems
> > > > to fix the issues with variably-modified types which are
> > > > returned from statement expressions (which works on clang),
> > > > but there are still bugs remaining related to structs
> > > > with VLA members (which seems to be a FE bug).
> > > > 
> > > > Of course, I might be doing something stupid...
> > > 
> > > How's evaluation order of (f())[g()] defined (with f returning a
> > > pointer)?
> > > Isn't that just f() + g()*sizeof(int) and thus undefined?
> > 
> > Yes, in C it is
> > 
> > f() + g()
> > 
> > and it is unsequenced. But the order of 'f' and 'g'
> > is not relevant here and also the patch does not change 
> > it (the base expression is gimplified before the index).
> > 
> > Essentially, we have
> > 
> > ({ ... }) + g() * sizeof(X) 
> > 
> > where X refers to a declaration in the statement expression.
> > Without the patch the size expressions are gimplified before
> > the base expression and also before the index expression. 
> > With the patch the ({ ... }) is gimplified also before the
> > size expression.
> > 
> > > If it's undefined then I think the incoming GENERIC is ill-defined.
> > 
> > I think it is OK because the arguments are evaluated 
> > before the operation.  Without the patch, parts of the 
> > operation (the size expressions) are gimplified before
> > the arguments and this seems wrong to me.
> 
> If I rewrite the ARRAY_REFs into *(f + g) in the test
> cases, they also works with unpatched GCC. So it is really
> the incorrect ordering in the gimplification of the
> ARRAY_REF which is the problem.

But there seem quite a few other bugs:

For

int foo2b(void) 
{
  return sizeof *({ int n = 20; struct { int x[n]; } x; x.x[12] = 1; &x; });
}

I get:


;; Function foo2b (null)
;; enabled by -tree-original


{
  return (int) ((unsigned int) (sizetype) SAVE_EXPR <n> * 4);
}


Martin

Re: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

[Martin Uecker]

(resending from a different account, as emails seem to do not
go out from my other account at this time)

Am Montag, den 02.08.2021, 16:05 +0200 schrieb Martin Uecker:
> > On Sun, Aug 1, 2021 at 7:37 PM Uecker, Martin
> > <Martin.Uecker@med.uni-goettingen.de> wrote:
> > > 
> > > 
> > > Here is an attempt to fix some old and annoying bugs related
> > > to VLAs and statement expressions. In particulary, this seems
> > > to fix the issues with variably-modified types which are
> > > returned from statement expressions (which works on clang),
> > > but there are still bugs remaining related to structs
> > > with VLA members (which seems to be a FE bug).
> > > 
> > > Of course, I might be doing something stupid...
> > 
> > How's evaluation order of (f())[g()] defined (with f returning a
> > pointer)?
> > Isn't that just f() + g()*sizeof(int) and thus undefined?
> 
> Yes, in C it is
> 
> f() + g()
> 
> and it is unsequenced. But the order of 'f' and 'g'
> is not relevant here and also the patch does not change 
> it (the base expression is gimplified before the index).
> 
> Essentially, we have
> 
> ({ ... }) + g() * sizeof(X) 
> 
> where X refers to a declaration in the statement expression.
> Without the patch the size expressions are gimplified before
> the base expression and also before the index expression. 
> With the patch the ({ ... }) is gimplified also before the
> size expression.
> 
> > If it's undefined then I think the incoming GENERIC is ill-defined.
> 
> I think it is OK because the arguments are evaluated 
> before the operation.  Without the patch, parts of the 
> operation (the size expressions) are gimplified before
> the arguments and this seems wrong to me.
> 
> 
> Martin
> 
> 
> 
> 

Re: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

[Richard Biener]

On Tue, Aug 3, 2021 at 7:32 AM Martin Uecker <muecker@gwdg.de> wrote:
>
>
>
> (resending from a different account, as emails seem to do not
> go out from my other account at this time)
>
> Am Montag, den 02.08.2021, 16:05 +0200 schrieb Martin Uecker:
> > > On Sun, Aug 1, 2021 at 7:37 PM Uecker, Martin
> > > <Martin.Uecker@med.uni-goettingen.de> wrote:
> > > >
> > > >
> > > > Here is an attempt to fix some old and annoying bugs related
> > > > to VLAs and statement expressions. In particulary, this seems
> > > > to fix the issues with variably-modified types which are
> > > > returned from statement expressions (which works on clang),
> > > > but there are still bugs remaining related to structs
> > > > with VLA members (which seems to be a FE bug).
> > > >
> > > > Of course, I might be doing something stupid...
> > >
> > > How's evaluation order of (f())[g()] defined (with f returning a
> > > pointer)?
> > > Isn't that just f() + g()*sizeof(int) and thus undefined?
> >
> > Yes, in C it is
> >
> > f() + g()
> >
> > and it is unsequenced. But the order of 'f' and 'g'
> > is not relevant here and also the patch does not change
> > it (the base expression is gimplified before the index).
> >
> > Essentially, we have
> >
> > ({ ... }) + g() * sizeof(X)
> >
> > where X refers to a declaration in the statement expression.
> > Without the patch the size expressions are gimplified before
> > the base expression and also before the index expression.
> > With the patch the ({ ... }) is gimplified also before the
> > size expression.
> >
> > > If it's undefined then I think the incoming GENERIC is ill-defined.
> >
> > I think it is OK because the arguments are evaluated
> > before the operation.  Without the patch, parts of the
> > operation (the size expressions) are gimplified before
> > the arguments and this seems wrong to me.

But you said the evaluation order is undefined.  So IMHO
the GENERIC is undefined in evaluating the size of sth
that's not live?  That said, given the statement expression
result undergoes array to pointer decay doesn't this pointer
refer to an object that ended its lifetime?

"In a statement expression, any temporaries created within a statement
are destroyed at that statement's end."

That is, don't the testcases all invoke undefined behavior at runtime?

Richard.

> >
> > Martin
> >
> >
> >
> >
>

Re: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

[Martin Uecker]

Hi 
Am Dienstag, den 03.08.2021, 10:10 +0200 schrieb Richard Biener:
> On Tue, Aug 3, 2021 at 7:32 AM Martin Uecker <muecker@gwdg.de> wrote:
> > 
> > 
> > (resending from a different account, as emails seem to do not
> > go out from my other account at this time)
> > 
> > Am Montag, den 02.08.2021, 16:05 +0200 schrieb Martin Uecker:
> > > > On Sun, Aug 1, 2021 at 7:37 PM Uecker, Martin
> > > > <Martin.Uecker@med.uni-goettingen.de> wrote:
> > > > > 
> > > > > Here is an attempt to fix some old and annoying bugs related
> > > > > to VLAs and statement expressions. In particulary, this seems
> > > > > to fix the issues with variably-modified types which are
> > > > > returned from statement expressions (which works on clang),
> > > > > but there are still bugs remaining related to structs
> > > > > with VLA members (which seems to be a FE bug).
> > > > > 
> > > > > Of course, I might be doing something stupid...
> > > > 
> > > > How's evaluation order of (f())[g()] defined (with f returning
> > > > a
> > > > pointer)?
> > > > Isn't that just f() + g()*sizeof(int) and thus undefined?
> > > 
> > > Yes, in C it is
> > > 
> > > f() + g()
> > > 
> > > and it is unsequenced. But the order of 'f' and 'g'
> > > is not relevant here and also the patch does not change
> > > it (the base expression is gimplified before the index).
> > > 
> > > Essentially, we have
> > > 
> > > ({ ... }) + g() * sizeof(X)
> > > 
> > > where X refers to a declaration in the statement expression.
> > > Without the patch the size expressions are gimplified before
> > > the base expression and also before the index expression.
> > > With the patch the ({ ... }) is gimplified also before the
> > > size expression.
> > > 
> > > > If it's undefined then I think the incoming GENERIC is ill-
> > > > defined.
> > > 
> > > I think it is OK because the arguments are evaluated
> > > before the operation.  Without the patch, parts of the
> > > operation (the size expressions) are gimplified before
> > > the arguments and this seems wrong to me.
> 
> But you said the evaluation order is undefined.

The evaluation order of the two arguments (base
and index) is undefined.  But the operation itself has
to happen after the arguments are evaluated like
the call to a is sequenced before f and g:

a(f(), g())


Computing the correct step size in the pointer
arithmetic is part of the operation itself and not
part of the evaluation of the arguments.

The problem here is that this part of the operation
is done before the arguments are evaluated, which
is a compiler bug.

> So IMHO the GENERIC is undefined in evaluating the size of sth
> that's not live? 
>
>  That said, given the statement expression
> result undergoes array to pointer decay doesn't this pointer
> refer to an object that ended its lifetime?

> "In a statement expression, any temporaries created within a
> statement are destroyed at that statement's end."

> That is, don't the testcases all invoke undefined behavior at
> runtime?

This is true for one of the test cases (where not having
an ICE is then
a QoI issue), but not for the others
where the object is allocated by
malloc and a pointer
to the object is returned from the statement
expression.
This is supposed to work.


Martin

Re: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

[Richard Biener]

On Tue, Aug 3, 2021 at 10:28 AM Martin Uecker <muecker@gwdg.de> wrote:
>
>
> Hi
> Am Dienstag, den 03.08.2021, 10:10 +0200 schrieb Richard Biener:
> > On Tue, Aug 3, 2021 at 7:32 AM Martin Uecker <muecker@gwdg.de> wrote:
> > >
> > >
> > > (resending from a different account, as emails seem to do not
> > > go out from my other account at this time)
> > >
> > > Am Montag, den 02.08.2021, 16:05 +0200 schrieb Martin Uecker:
> > > > > On Sun, Aug 1, 2021 at 7:37 PM Uecker, Martin
> > > > > <Martin.Uecker@med.uni-goettingen.de> wrote:
> > > > > >
> > > > > > Here is an attempt to fix some old and annoying bugs related
> > > > > > to VLAs and statement expressions. In particulary, this seems
> > > > > > to fix the issues with variably-modified types which are
> > > > > > returned from statement expressions (which works on clang),
> > > > > > but there are still bugs remaining related to structs
> > > > > > with VLA members (which seems to be a FE bug).
> > > > > >
> > > > > > Of course, I might be doing something stupid...
> > > > >
> > > > > How's evaluation order of (f())[g()] defined (with f returning
> > > > > a
> > > > > pointer)?
> > > > > Isn't that just f() + g()*sizeof(int) and thus undefined?
> > > >
> > > > Yes, in C it is
> > > >
> > > > f() + g()
> > > >
> > > > and it is unsequenced. But the order of 'f' and 'g'
> > > > is not relevant here and also the patch does not change
> > > > it (the base expression is gimplified before the index).
> > > >
> > > > Essentially, we have
> > > >
> > > > ({ ... }) + g() * sizeof(X)
> > > >
> > > > where X refers to a declaration in the statement expression.
> > > > Without the patch the size expressions are gimplified before
> > > > the base expression and also before the index expression.
> > > > With the patch the ({ ... }) is gimplified also before the
> > > > size expression.
> > > >
> > > > > If it's undefined then I think the incoming GENERIC is ill-
> > > > > defined.
> > > >
> > > > I think it is OK because the arguments are evaluated
> > > > before the operation.  Without the patch, parts of the
> > > > operation (the size expressions) are gimplified before
> > > > the arguments and this seems wrong to me.
> >
> > But you said the evaluation order is undefined.
>
> The evaluation order of the two arguments (base
> and index) is undefined.  But the operation itself has
> to happen after the arguments are evaluated like
> the call to a is sequenced before f and g:
>
> a(f(), g())
>
>
> Computing the correct step size in the pointer
> arithmetic is part of the operation itself and not
> part of the evaluation of the arguments.
>
> The problem here is that this part of the operation
> is done before the arguments are evaluated, which
> is a compiler bug.

Yes, but the bug is IMHO in the C frontend which inserts the DECL_EXPR
at a wrong spot, not making sure it is evaluated before it is used.  Working
around this deficiency in the gimplifier sounds incorrect.

Does the same issue arise with writing the testcases as

 ({ ... }) + i;

?  How can we fix it then if you also need to support

 i + ({ ...});

?

> > So IMHO the GENERIC is undefined in evaluating the size of sth
> > that's not live?
> >
> >  That said, given the statement expression
> > result undergoes array to pointer decay doesn't this pointer
> > refer to an object that ended its lifetime?
>
> > "In a statement expression, any temporaries created within a
> > statement are destroyed at that statement's end."
>
> > That is, don't the testcases all invoke undefined behavior at
> > runtime?
>
> This is true for one of the test cases (where not having
> an ICE is then
> a QoI issue), but not for the others
> where the object is allocated by
> malloc and a pointer
> to the object is returned from the statement
> expression.
> This is supposed to work.
>
>
> Martin
>
>
>

Re: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

[Martin Uecker]

Am Dienstag, den 03.08.2021, 11:26 +0200 schrieb Richard Biener:
> On Tue, Aug 3, 2021 at 10:28 AM Martin Uecker <muecker@gwdg.de>
> wrote:
> > 
> > Hi
> > Am Dienstag, den 03.08.2021, 10:10 +0200 schrieb Richard Biener:
> > > On Tue, Aug 3, 2021 at 7:32 AM Martin Uecker <muecker@gwdg.de>
> > > wrote:
> > > > 
> > > > (resending from a different account, as emails seem to do not
> > > > go out from my other account at this time)
> > > > 
> > > > Am Montag, den 02.08.2021, 16:05 +0200 schrieb Martin Uecker:
> > > > > > On Sun, Aug 1, 2021 at 7:37 PM Uecker, Martin
> > > > > > <Martin.Uecker@med.uni-goettingen.de> wrote:
> > > > > > > Here is an attempt to fix some old and annoying bugs
> > > > > > > related
> > > > > > > to VLAs and statement expressions. In particulary, this
> > > > > > > seems
> > > > > > > to fix the issues with variably-modified types which are
> > > > > > > returned from statement expressions (which works on
> > > > > > > clang),
> > > > > > > but there are still bugs remaining related to structs
> > > > > > > with VLA members (which seems to be a FE bug).
> > > > > > > 
> > > > > > > Of course, I might be doing something stupid...
> > > > > > 
> > > > > > How's evaluation order of (f())[g()] defined (with f
> > > > > > returning
> > > > > > a
> > > > > > pointer)?
> > > > > > Isn't that just f() + g()*sizeof(int) and thus undefined?
> > > > > 
> > > > > Yes, in C it is
> > > > > 
> > > > > f() + g()
> > > > > 
> > > > > and it is unsequenced. But the order of 'f' and 'g'
> > > > > is not relevant here and also the patch does not change
> > > > > it (the base expression is gimplified before the index).
> > > > > 
> > > > > Essentially, we have
> > > > > 
> > > > > ({ ... }) + g() * sizeof(X)
> > > > > 
> > > > > where X refers to a declaration in the statement expression.
> > > > > Without the patch the size expressions are gimplified before
> > > > > the base expression and also before the index expression.
> > > > > With the patch the ({ ... }) is gimplified also before the
> > > > > size expression.
> > > > > 
> > > > > > If it's undefined then I think the incoming GENERIC is ill-
> > > > > > defined.
> > > > > 
> > > > > I think it is OK because the arguments are evaluated
> > > > > before the operation.  Without the patch, parts of the
> > > > > operation (the size expressions) are gimplified before
> > > > > the arguments and this seems wrong to me.
> > > 
> > > But you said the evaluation order is undefined.
> > 
> > The evaluation order of the two arguments (base
> > and index) is undefined.  But the operation itself has
> > to happen after the arguments are evaluated like
> > the call to a is sequenced before f and g:
> > 
> > a(f(), g())
> > 
> > 
> > Computing the correct step size in the pointer
> > arithmetic is part of the operation itself and not
> > part of the evaluation of the arguments.
> > 
> > The problem here is that this part of the operation
> > is done before the arguments are evaluated, which
> > is a compiler bug.
> 
> Yes, but the bug is IMHO in the C frontend which inserts the
> DECL_EXPR at a wrong spot, not making sure it is evaluated before it
> is used.  Working around this deficiency in the gimplifier sounds
> incorrect.

The size if part of the type of the value which is 
returned from the compound  expression. 

So that there is a declaration involved was maybe
misleading in my example and explanation.  

So let me try again:

The size *expression* needs be be computed inside the 
statement expression (also because of side effects)
and then the result of this computation needs to 
returned together (as part of) the value returned 
by the statement expression. 

But the compilers tries to gimplify the size expression 
that  comes with the value already before the statement 
expression is evaluated. This can not work, no matter
what the front end does.

> 
> Does the same issue arise with writing the testcases as
> 
>  ({ ... }) + i;
> 
> ?  How can we fix it then if you also need to support
> 
>  i + ({ ...});
> 
> 
> ?

This already works correctly. I assume that here
the gimplifcation is  already  done in the right 
order.

But ARRAY_REF is handled as a separate case and
there it is wrong.

Martin

> 
> > > So IMHO the GENERIC is undefined in evaluating the size of sth
> > > that's not live?
> > > 
> > >  That said, given the statement expression
> > > result undergoes array to pointer decay doesn't this pointer
> > > refer to an object that ended its lifetime?
> > > "In a statement expression, any temporaries created within a
> > > statement are destroyed at that statement's end."
> > > That is, don't the testcases all invoke undefined behavior at
> > > runtime?
> > 
> > This is true for one of the test cases (where not having
> > an ICE is then
> > a QoI issue), but not for the others
> > where the object is allocated by
> > malloc and a pointer
> > to the object is returned from the statement
> > expression.
> > This is supposed to work.
> > 
> > 
> > Martin
> > 
> > 
> > 

Re: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

[Martin Uecker]

Am Dienstag, den 03.08.2021, 11:26 +0200 schrieb Richard Biener:
> On Tue, Aug 3, 2021 at 10:28 AM Martin Uecker <muecker@gwdg.de> wrote:


> 
> Does the same issue arise with writing the testcases as
> 
>  ({ ... }) + i;
> 
> ?  How can we fix it then if you also need to support
> 
>  i + ({ ...});
> 
> ?

Here, the FE always moves the pointer to the right and
then produces something like:

*((int *) TARGET_EXPR <D.1952, {
    int n = 10;
    ...
    D.1952 = x;
  }> + ((sizetype) SAVE_EXPR <n> + 1) * 20);


So these are the cases which already work without
the path, although maybe it is wrong to have
the n in the SAVE_EXPR?

It gets gimplified to something like this,
which works:

  int[0:D.1959][0:D.1955] * D.1952;
  int n.0;
  sizetype D.1955;
  sizetype D.1959;
 
  {
    int n;
    int[0:D.1959][0:D.1955] * x;

        n = 10;
        n.0 = n;
    
	...

        _32 = (sizetype) n.0;
        _33 = (sizetype) n.1;
        _34 = _32 * _33;
        _35 = _34 * 4;
        x = __builtin_malloc (_35);
        D.1952 = x;
  }
  _36 = (sizetype) n.0;
  _37 = _36 + 1;
  _38 = _37 * 20;
  _39 = D.1952 + _38;
 

For the array ref, the FE produces:


  (*TARGET_EXPR <D.1951, {
    int n = 10;
    ...
    D.1951 = x;
  }>)[5][5];


With the patch, we get something like
the following in GIMPLE, which seems correct:

  int[0:D.1958][0:D.1954] * D.1951;
  int n.0;
  sizetype D.1954;

  {
    int n;
    int[0:D.1958][0:D.1954] * x;

    n = 10;
    n.0 = n;
 
    _7 = (sizetype) n.0;
    _8 = _7 * 4;
    D.1956 = _8;

    n.1 = n
 
    _22 = (sizetype) n.0;
    _23 = (sizetype) n.1;
    _24 = _22 * _23;
    _25 = _24 * 4;
    x = __builtin_malloc (_25);
    D.1951 = x;
  }
  _26 = D.1956 /[ex] 4;
  c = (*D.1951)[5]{lb: 0 sz: _26 * 4}[5];
 

MArtin

Re: Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

[Richard Biener]

On Tue, Aug 3, 2021 at 9:31 PM Martin Uecker <muecker@gwdg.de> wrote:
>
> Am Dienstag, den 03.08.2021, 11:26 +0200 schrieb Richard Biener:
> > On Tue, Aug 3, 2021 at 10:28 AM Martin Uecker <muecker@gwdg.de> wrote:
>
>
> >
> > Does the same issue arise with writing the testcases as
> >
> >  ({ ... }) + i;
> >
> > ?  How can we fix it then if you also need to support
> >
> >  i + ({ ...});
> >
> > ?
>
> Here, the FE always moves the pointer to the right and
> then produces something like:
>
> *((int *) TARGET_EXPR <D.1952, {
>     int n = 10;
>     ...
>     D.1952 = x;
>   }> + ((sizetype) SAVE_EXPR <n> + 1) * 20);
>
>
> So these are the cases which already work without
> the path, although maybe it is wrong to have
> the n in the SAVE_EXPR?
>
> It gets gimplified to something like this,
> which works:
>
>   int[0:D.1959][0:D.1955] * D.1952;
>   int n.0;
>   sizetype D.1955;
>   sizetype D.1959;
>
>   {
>     int n;
>     int[0:D.1959][0:D.1955] * x;
>
>         n = 10;
>         n.0 = n;
>
>         ...
>
>         _32 = (sizetype) n.0;
>         _33 = (sizetype) n.1;
>         _34 = _32 * _33;
>         _35 = _34 * 4;
>         x = __builtin_malloc (_35);
>         D.1952 = x;
>   }
>   _36 = (sizetype) n.0;
>   _37 = _36 + 1;
>   _38 = _37 * 20;
>   _39 = D.1952 + _38;
>
>
> For the array ref, the FE produces:
>
>
>   (*TARGET_EXPR <D.1951, {
>     int n = 10;
>     ...
>     D.1951 = x;
>   }>)[5][5];
>
>
> With the patch, we get something like
> the following in GIMPLE, which seems correct:
>
>   int[0:D.1958][0:D.1954] * D.1951;
>   int n.0;
>   sizetype D.1954;
>
>   {
>     int n;
>     int[0:D.1958][0:D.1954] * x;
>
>     n = 10;
>     n.0 = n;
>
>     _7 = (sizetype) n.0;
>     _8 = _7 * 4;
>     D.1956 = _8;
>
>     n.1 = n
>
>     _22 = (sizetype) n.0;
>     _23 = (sizetype) n.1;
>     _24 = _22 * _23;
>     _25 = _24 * 4;
>     x = __builtin_malloc (_25);
>     D.1951 = x;
>   }
>   _26 = D.1956 /[ex] 4;
>   c = (*D.1951)[5]{lb: 0 sz: _26 * 4}[5];

OK, thanks for the try to explaining.

I think the patch makes sense but the comment says

     Java requires that we elaborated nodes in source order.  That
     means we must gimplify the inner expression followed by each of
     the indices, in order.  But we can't gimplify the inner
     expression until we deal with any variable bounds, sizes, or
     positions in order to deal with PLACEHOLDER_EXPRs.

and I don't really understand that (not to say Java is no longer supported
by GCC, but Ada does use PLACEHOLDER_EXPRs).  In fact the comment
suggests that we _should_ gimplify the inner expression first ... at least it
seems to after your explanations ;)

Eric - do you know anything about this?

Martin - I think the patch is OK but please make sure to bootstrap & regtest
with all languages enabled (--enable-languages=all) and make sure you
have an Ada host compiler installed so you get Ada included as well.

Thanks,
Richard.

>
> MArtin
>

Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

[Eric Botcazou]

> I think the patch makes sense but the comment says
> 
>      Java requires that we elaborated nodes in source order.  That
>      means we must gimplify the inner expression followed by each of
>      the indices, in order.  But we can't gimplify the inner
>      expression until we deal with any variable bounds, sizes, or
>      positions in order to deal with PLACEHOLDER_EXPRs.
> 
> and I don't really understand that (not to say Java is no longer supported
> by GCC, but Ada does use PLACEHOLDER_EXPRs).  In fact the comment
> suggests that we _should_ gimplify the inner expression first ... at least
> it seems to after your explanations ;)

We already gimplify the inner expression first, i.e. before indices and 
operands, but we gimplify the "annotations" (special operands) before.

> Eric - do you know anything about this?

If the comment is right, then Martin's patch should break Ada indeed.

-- 
Eric Botcazou

Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

[Martin Uecker]

Am Montag, den 09.08.2021, 15:52 +0200 schrieb Eric Botcazou:
> > I think the patch makes sense but the comment says
> > 
> >      Java requires that we elaborated nodes in source order.  That
> >      means we must gimplify the inner expression followed by each
> > of
> >      the indices, in order.  But we can't gimplify the inner
> >      expression until we deal with any variable bounds, sizes, or
> >      positions in order to deal with PLACEHOLDER_EXPRs.
> > 
> > and I don't really understand that (not to say Java is no longer
> > supported
> > by GCC, but Ada does use PLACEHOLDER_EXPRs).  In fact the comment
> > suggests that we _should_ gimplify the inner expression first ...
> > at least
> > it seems to after your explanations ;)
> 
> We already gimplify the inner expression first, i.e. before indices
> and  operands, but we gimplify the "annotations" (special operands)
> before.
> 
> > Eric - do you know anything about this?
> 
> If the comment is right, then Martin's patch should break Ada indeed.
> 

Yes, it breaks Ada. I already found this out 
in the meanwhile.

But I do not understand how this works with the
PLACEHOLDER_EXPR.

My understanding of this is that this is for referring 
to some field of an outer struct which is then used in the
size expression, e.g. something like this (using
C syntax):

struct foo {
  int len;
  float (*x)[3][len];
};

And then for

struct foo* p;

p->x[1][1];  

we use the field 'len' of the struct pointed to by 'p'
the size expression.

But then why would you gimplify the size expression before
the base expression?  I would assume that you also want
to process the base expression first, because it is the
source of the struct which we access in the size expression.

Best,
Martin

Re: [PATCH] Fix ICE when mixing VLAs and statement expressions [PR91038]

[Eric Botcazou]

> Yes, it breaks Ada. I already found this out in the meanwhile.

OK, thanks for checking.

> My understanding of this is that this is for referring
> to some field of an outer struct which is then used in the
> size expression, e.g. something like this (using
> C syntax):
> 
> struct foo {
>   int len;
>   float (*x)[3][len];
> };

Yes, just

struct foo {
  int len;
  float a[len];
}

but it's also used for unconstrained array types and this seems to be the 
problem here, e.g. for:

package Vect1 is

   type Varray is array (Integer range <>) of Long_Float;
   for Varray'Alignment use 16;

   procedure Add (X, Y : not null access Varray; R : not null access Varray);

end Vect1;

package body Vect1 is

   procedure Add (X, Y : not null access Varray; R : not null access Varray) 
is
   begin
      for I in X'Range loop
         R(I) := X(I) + Y(I);
      end loop;
   end;

end Vect1;

> But then why would you gimplify the size expression before
> the base expression?  I would assume that you also want
> to process the base expression first, because it is the
> source of the struct which we access in the size expression.

For the above testcase, we have a dangling PLACEHOLDER_EXPRs

+===========================GNAT BUG DETECTED==============================+
| 12.0.0 20210805 (experimental) [master revision e314cfc371d:
5eb84b79079:ead235f60139edc6eb408d8d083cbb15e417b447] (x86_64-suse-linux) GCC 
error:|
| in gimplify_expr, at gimplify.c:15019                                    |
| Error detected around vect1.adb:6:23

probably because array_ref_low_bound, where it is normally eliminated, cannot 
do its jub properly when it is called after the base expression is gimplified.

-- 
Eric Botcazou