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
