[PUSHED] Fix off-by-one storage problem in irange_allocator.

[Andrew MacLeod]

On 10/6/20 6:22 AM, Jakub Jelinek via Gcc-patches wrote:
> On Tue, Oct 06, 2020 at 11:20:52AM +0200, Aldy Hernandez wrote:
>>>> diff --git a/gcc/value-range.h b/gcc/value-range.h
>>>> index 94b48e55e77..7031a823138 100644
>>>> --- a/gcc/value-range.h
>>>> +++ b/gcc/value-range.h
>>>> @@ -670,7 +670,7 @@ irange_allocator::allocate (unsigned num_pairs)
>>>>
>>>>      struct newir {
>>>>        irange range;
>>>> -    tree mem[1];
>>>> +    tree mem[2];
>>>>      };
>>>>      size_t nbytes = (sizeof (newir) + sizeof (tree) * 2 * (num_pairs - 1));
>>>>      struct newir *r = (newir *) obstack_alloc (&m_obstack, nbytes);
>>> So, we essentially want a flexible array member, which C++ without extension
>>> doesn't have, and thus need to rely on the compiler handling the trailing
>>> array as a poor men's flexible array member (again, GCC does for any size,
>>> but not 100% sure about other compilers, if they e.g. don't handle that way
>>> just size of 1).
>> We know we need _at least_ two trees, so what's wrong with the above?
> See the discussions we even had in GCC.  Some of us are arguing that only
> flexible array member should be treated as such, others also add [0] to
> that, others [1] and others any arrays at the trailing positions.
> Because standard C++ lacks both [] and [0], at least [1] support is needed
> eventhough perhaps pedantically it is invalid.  GCC after all heavily relies
> on that elsewhere, e.g. in rtl or gimple structures.  But it is still all
> just [1], not [2] or [32].  And e.g. Coverity complains about that a lot.
> There is another way around it, using [MAXIMUM_POSSIBLE_COUNT] instead and
> then allocating only a subset of those using offsetof to count the size.
> But that is undefined in a different way, would probably make Coverity
> happy and e.g. for RTL is doable because we have maximum number of operands,
> and for many gimple stmts too, except that e.g. GIMPLE_CALLs don't really
> have a maximum (well, have it as UINT_MAX - 3 or so).
>
> GCC to my knowledge will treat all the trailing arrays that way, but it is
> unclear if other compilers do the same or not.
> You can use mem[1] and just use
>        size_t nbytes = sizeof (newir) + sizeof (tree) * (2 * num_pairs - 1);

sure that is fine too. I was not aware of any issue with changing [1] to 
[2], it just seemed like the obvious thing :-).

so everything is copacetic  if we go back to [1] and add a sizeof(tree) 
instead?

Andrew



>
>>> Is there any reason why the code is written that way?
>>> I mean, we could just use:
>>>     size_t nbytes = sizeof (irange) + sizeof (tree) * 2 * num_pairs;
>> We had that originally, but IIRC, the alignment didn't come out right.
> That surprises me, because I don't understand how it could (unless irange
> didn't have a pointer member at that point).
>
> 	Jakub
>