On Wed, Feb 26, 2020 at 06:01:30PM -0700, Martin Sebor wrote:
> On 2/26/20 1:44 PM, Marek Polacek wrote:
> > r7-2111 introduced maybe_constant_value in cp_fully_fold.
> > maybe_constant_value uses cxx_eval_outermost_constant_expr, which
> > can clear TREE_CONSTANT:
> > 6510 else if (non_constant_p && TREE_CONSTANT (r))
> > [...]
> > 6529 TREE_CONSTANT (r) = false;
> >
> > In this test the array size is '(long int) "h"'. This used to be
> > TREE_CONSTANT but given the change above, the flag will be cleared
> > when we cp_fully_fold the array size in compute_array_index_type_loc.
> > That means we don't emit an error in the
> > 10391 else if (TREE_CONSTANT (size)
> > block in the same function, and we go on. Then we compute ITYPE
> > using cp_build_binary_op and use maybe_constant_value on it and
> > suddenly we have something that's TREE_CONSTANT again. And then we
> > crash in reshape_init_array_1 in tree_to_uhwi, because what we have
> > doesn't fit in an unsigned HWI.
> >
> > icc accepts this code, but since we used to reject it, I see no desire
> > to make this work, so I've added a check that triggers when we end up
> > with a constant that is not an INTEGER_CST.
>
> G++ accepts this equivalent:
>
> void f () {
> long n = ( long ) "h";
> const int tbl [n] = { 12 };
> ...
> }
>
> and it doesn't look like the patch changes that.
It does not, but g++ will reject the same code if you make 'n' const:
a) if 'n' is not const, we won't be able to fold 'n' and it's not
TREE_CONSTANT -> we treat it as a VLA
b) if 'n' is const, we still can't fold it to an INTEGER_CST but it is
TREE_CONSTANT -> we give an error
c) if 'n' is constexpr, it's ill-formed since we're converting a pointer
to integral type (more on this in my next mail)
I would reject this even in a), because we're likely allocating something
huge that's likely to overflow the stack, making the code very questionable.
> I think all these cases of initialized VLAs ought to be handled
> the same way: either they should all be rejected or they should
> all be accepted. The rather dated pr58646, also a regression,
> is another example of a few ICEs in this area. See also this
> recently submitted patch:
> https://gcc.gnu.org/ml/gcc-patches/2020-02/msg01148.html
Yeah, it's a gift that keeps on giving :/.
> FWIW, at one point I got VLA initialization to work again (r234966),
> after it had been removed due to the last minute removal of VLAs
> from the standard, but it caused trouble for Java and I was forced
> to revert the changes. I've been hoping to revisit it but other
> things have been getting in the way.
I think clang doesn't allow VLA initialization at all, so there's that.
I haven't reviewed our old correspondence on this topic but I think I
was, and still am, for restricting what we allow users to do with VLAs.
--
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
