Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On 10/6/20 11:58 AM, Andrew MacLeod wrote:
On 10/6/20 1:48 PM, Jakub Jelinek wrote:
On Tue, Oct 06, 2020 at 01:41:54PM -0400, Andrew MacLeod wrote:
On 10/6/20 1:32 PM, Jakub Jelinek via Gcc-patches wrote:
On Tue, Oct 06, 2020 at 10:42:12AM -0600, Martin Sebor wrote:
The manual documents the [0] extension and mentions but discourages
using [1]. Nothing is said about other sizes and the warnings such
as -Warray-bounds have been increasingly complaining about accesses
past the declared constant bound (it won't complain about past-
the-end accesses to a mem[1], but will about those to mem[2]).
It would be nice if existing GCC code could eventually be converted
to avoid relying on the [1] hack. I would hope we would avoid making
use of it in new code (and certainly avoid extending its uses to other
sizes).
I don't see how we could, because [0] is an extension and GCC needs to
support host compilers that don't support it, and similarly [] is an
extension in C++ and can't be relied on.
Changing say rtl or gimple structs from the way we define them
currently
to say templates dependent on the size of the embedded arrays is I'm
afraid not really possible.
Jakub
Aldy is currently testing this.. I presume this is what you had in mind?
Yes.
I'm not actually sure if it is best to use struct irange (and why struct
irange rather than just irange?) for the r variable, whether it shouldn't
be unsigned char * or whatever is best suitable type for placement new.
bah, the struct is a carry over from when it was struct new_ir. doh.
as to the type for placement new, I dunno.
Placement new takes void* as an argument. Since that's also what
obstack_alloc() returns that should obviate the need for a cast.
Martin
diff --git a/gcc/value-range.h b/gcc/value-range.h
index 94b48e55e77..c86301fe885 100644
--- a/gcc/value-range.h
+++ b/gcc/value-range.h
@@ -668,13 +668,13 @@ irange_allocator::allocate (unsigned num_pairs)
if (num_pairs < 2)
num_pairs = 2;
- struct newir {
- irange range;
- tree mem[1];
- };
- size_t nbytes = (sizeof (newir) + sizeof (tree) * 2 * (num_pairs -
1));
- struct newir *r = (newir *) obstack_alloc (_obstack, nbytes);
- return new (r) irange (r->mem, num_pairs);
+ // Allocate the irange and required memory for the vector.
+ struct irange *r = (irange *) obstack_alloc (_obstack, sizeof
(irange));
+
+ size_t nbytes = sizeof (tree) * 2 * num_pairs;
+ tree *mem = (tree *) obstack_alloc (_obstack, nbytes);
+
+ return new (r) irange (mem, num_pairs);
}
inline irange *
Jakub
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On 10/6/20 2:41 PM, Andreas Schwab wrote:
On Okt 06 2020, Andrew MacLeod via Gcc-patches wrote:
diff --git a/gcc/value-range.h b/gcc/value-range.h
index 7031a823138..02a952bf81f 100644
--- a/gcc/value-range.h
+++ b/gcc/value-range.h
@@ -668,13 +668,12 @@ irange_allocator::allocate (unsigned num_pairs)
if (num_pairs < 2)
num_pairs = 2;
- struct newir {
-irange range;
-tree mem[2];
- };
- size_t nbytes = (sizeof (newir) + sizeof (tree) * 2 * (num_pairs - 1));
- struct newir *r = (newir *) obstack_alloc (_obstack, nbytes);
- return new (r) irange (r->mem, num_pairs);
+ size_t nbytes = sizeof (tree) * 2 * num_pairs;
+
+ // Allocate the irnge and required memory for the vector
Typo: irange
Andreas.
Ha. Its all good now. THIS is actually the final final FINAL patch
which is going thru testing.
diff --git a/gcc/value-range.h b/gcc/value-range.h
index 7031a823138..63c96204cda 100644
--- a/gcc/value-range.h
+++ b/gcc/value-range.h
@@ -668,13 +668,12 @@ irange_allocator::allocate (unsigned num_pairs)
if (num_pairs < 2)
num_pairs = 2;
- struct newir {
-irange range;
-tree mem[2];
- };
- size_t nbytes = (sizeof (newir) + sizeof (tree) * 2 * (num_pairs - 1));
- struct newir *r = (newir *) obstack_alloc (_obstack, nbytes);
- return new (r) irange (r->mem, num_pairs);
+ size_t nbytes = sizeof (tree) * 2 * num_pairs;
+
+ // Allocate the irange and required memory for the vector.
+ void *r = obstack_alloc (_obstack, sizeof (irange));
+ tree *mem = (tree *) obstack_alloc (_obstack, nbytes);
+ return new (r) irange (mem, num_pairs);
}
inline irange *
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On Okt 06 2020, Andrew MacLeod via Gcc-patches wrote:
> diff --git a/gcc/value-range.h b/gcc/value-range.h
> index 7031a823138..02a952bf81f 100644
> --- a/gcc/value-range.h
> +++ b/gcc/value-range.h
> @@ -668,13 +668,12 @@ irange_allocator::allocate (unsigned num_pairs)
>if (num_pairs < 2)
> num_pairs = 2;
>
> - struct newir {
> -irange range;
> -tree mem[2];
> - };
> - size_t nbytes = (sizeof (newir) + sizeof (tree) * 2 * (num_pairs - 1));
> - struct newir *r = (newir *) obstack_alloc (_obstack, nbytes);
> - return new (r) irange (r->mem, num_pairs);
> + size_t nbytes = sizeof (tree) * 2 * num_pairs;
> +
> + // Allocate the irnge and required memory for the vector
Typo: irange
Andreas.
--
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On 10/6/20 2:18 PM, Jakub Jelinek wrote: On Tue, Oct 06, 2020 at 02:09:42PM -0400, Andrew MacLeod wrote: + size_t nbytes = sizeof (tree) * 2 * num_pairs; + + // Allocate the irnge and required memory for the vector + void *r = (irange *) obstack_alloc (_obstack, sizeof (irange)); Then either void *r = (void *) obstack_alloc (_obstack, sizeof (irange)); or even better void *r = obstack_alloc (_obstack, sizeof (irange)); Id already noticed that and went with the latter
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
The following cast looks odd: On 10/6/20 8:09 PM, Andrew MacLeod via Gcc-patches wrote: + // Allocate the irnge and required memory for the vector + void *r = (irange *) obstack_alloc (_obstack, sizeof (irange)); Tobias - Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On Tue, Oct 06, 2020 at 02:09:42PM -0400, Andrew MacLeod wrote: > + size_t nbytes = sizeof (tree) * 2 * num_pairs; > + > + // Allocate the irnge and required memory for the vector > + void *r = (irange *) obstack_alloc (_obstack, sizeof (irange)); Then either void *r = (void *) obstack_alloc (_obstack, sizeof (irange)); or even better void *r = obstack_alloc (_obstack, sizeof (irange)); > + tree *mem = (tree *) obstack_alloc (_obstack, nbytes); > + return new (r) irange (mem, num_pairs); > } > > inline irange * Jakub
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On 10/6/20 1:58 PM, Andrew MacLeod via Gcc-patches wrote:
On 10/6/20 1:48 PM, Jakub Jelinek wrote:
On Tue, Oct 06, 2020 at 01:41:54PM -0400, Andrew MacLeod wrote:
On 10/6/20 1:32 PM, Jakub Jelinek via Gcc-patches wrote:
On Tue, Oct 06, 2020 at 10:42:12AM -0600, Martin Sebor wrote:
The manual documents the [0] extension and mentions but discourages
using [1]. Nothing is said about other sizes and the warnings such
as -Warray-bounds have been increasingly complaining about accesses
past the declared constant bound (it won't complain about past-
the-end accesses to a mem[1], but will about those to mem[2]).
It would be nice if existing GCC code could eventually be converted
to avoid relying on the [1] hack. I would hope we would avoid making
use of it in new code (and certainly avoid extending its uses to
other
sizes).
I don't see how we could, because [0] is an extension and GCC needs to
support host compilers that don't support it, and similarly [] is an
extension in C++ and can't be relied on.
Changing say rtl or gimple structs from the way we define them
currently
to say templates dependent on the size of the embedded arrays is I'm
afraid not really possible.
Jakub
Aldy is currently testing this.. I presume this is what you had in
mind?
Yes.
I'm not actually sure if it is best to use struct irange (and why struct
irange rather than just irange?) for the r variable, whether it
shouldn't
be unsigned char * or whatever is best suitable type for placement new.
bah, the struct is a carry over from when it was struct new_ir. doh.
as to the type for placement new, I dunno.
The authorities inform me that void * is preferred.. so.. final spin, in
testing now:
diff --git a/gcc/value-range.h b/gcc/value-range.h
index 7031a823138..02a952bf81f 100644
--- a/gcc/value-range.h
+++ b/gcc/value-range.h
@@ -668,13 +668,12 @@ irange_allocator::allocate (unsigned num_pairs)
if (num_pairs < 2)
num_pairs = 2;
- struct newir {
-irange range;
-tree mem[2];
- };
- size_t nbytes = (sizeof (newir) + sizeof (tree) * 2 * (num_pairs - 1));
- struct newir *r = (newir *) obstack_alloc (_obstack, nbytes);
- return new (r) irange (r->mem, num_pairs);
+ size_t nbytes = sizeof (tree) * 2 * num_pairs;
+
+ // Allocate the irnge and required memory for the vector
+ void *r = (irange *) obstack_alloc (_obstack, sizeof (irange));
+ tree *mem = (tree *) obstack_alloc (_obstack, nbytes);
+ return new (r) irange (mem, num_pairs);
}
inline irange *
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On 10/6/20 1:48 PM, Jakub Jelinek wrote:
On Tue, Oct 06, 2020 at 01:41:54PM -0400, Andrew MacLeod wrote:
On 10/6/20 1:32 PM, Jakub Jelinek via Gcc-patches wrote:
On Tue, Oct 06, 2020 at 10:42:12AM -0600, Martin Sebor wrote:
The manual documents the [0] extension and mentions but discourages
using [1]. Nothing is said about other sizes and the warnings such
as -Warray-bounds have been increasingly complaining about accesses
past the declared constant bound (it won't complain about past-
the-end accesses to a mem[1], but will about those to mem[2]).
It would be nice if existing GCC code could eventually be converted
to avoid relying on the [1] hack. I would hope we would avoid making
use of it in new code (and certainly avoid extending its uses to other
sizes).
I don't see how we could, because [0] is an extension and GCC needs to
support host compilers that don't support it, and similarly [] is an
extension in C++ and can't be relied on.
Changing say rtl or gimple structs from the way we define them currently
to say templates dependent on the size of the embedded arrays is I'm
afraid not really possible.
Jakub
Aldy is currently testing this.. I presume this is what you had in mind?
Yes.
I'm not actually sure if it is best to use struct irange (and why struct
irange rather than just irange?) for the r variable, whether it shouldn't
be unsigned char * or whatever is best suitable type for placement new.
bah, the struct is a carry over from when it was struct new_ir. doh.
as to the type for placement new, I dunno.
diff --git a/gcc/value-range.h b/gcc/value-range.h
index 94b48e55e77..c86301fe885 100644
--- a/gcc/value-range.h
+++ b/gcc/value-range.h
@@ -668,13 +668,13 @@ irange_allocator::allocate (unsigned num_pairs)
if (num_pairs < 2)
num_pairs = 2;
- struct newir {
-irange range;
-tree mem[1];
- };
- size_t nbytes = (sizeof (newir) + sizeof (tree) * 2 * (num_pairs - 1));
- struct newir *r = (newir *) obstack_alloc (_obstack, nbytes);
- return new (r) irange (r->mem, num_pairs);
+ // Allocate the irange and required memory for the vector.
+ struct irange *r = (irange *) obstack_alloc (_obstack, sizeof (irange));
+
+ size_t nbytes = sizeof (tree) * 2 * num_pairs;
+ tree *mem = (tree *) obstack_alloc (_obstack, nbytes);
+
+ return new (r) irange (mem, num_pairs);
}
inline irange *
Jakub
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On Tue, Oct 06, 2020 at 01:41:54PM -0400, Andrew MacLeod wrote:
> On 10/6/20 1:32 PM, Jakub Jelinek via Gcc-patches wrote:
> > On Tue, Oct 06, 2020 at 10:42:12AM -0600, Martin Sebor wrote:
> > > The manual documents the [0] extension and mentions but discourages
> > > using [1]. Nothing is said about other sizes and the warnings such
> > > as -Warray-bounds have been increasingly complaining about accesses
> > > past the declared constant bound (it won't complain about past-
> > > the-end accesses to a mem[1], but will about those to mem[2]).
> > >
> > > It would be nice if existing GCC code could eventually be converted
> > > to avoid relying on the [1] hack. I would hope we would avoid making
> > > use of it in new code (and certainly avoid extending its uses to other
> > > sizes).
> > I don't see how we could, because [0] is an extension and GCC needs to
> > support host compilers that don't support it, and similarly [] is an
> > extension in C++ and can't be relied on.
> > Changing say rtl or gimple structs from the way we define them currently
> > to say templates dependent on the size of the embedded arrays is I'm
> > afraid not really possible.
> >
> > Jakub
> >
> Aldy is currently testing this.. I presume this is what you had in mind?
Yes.
I'm not actually sure if it is best to use struct irange (and why struct
irange rather than just irange?) for the r variable, whether it shouldn't
be unsigned char * or whatever is best suitable type for placement new.
> diff --git a/gcc/value-range.h b/gcc/value-range.h
> index 94b48e55e77..c86301fe885 100644
> --- a/gcc/value-range.h
> +++ b/gcc/value-range.h
> @@ -668,13 +668,13 @@ irange_allocator::allocate (unsigned num_pairs)
>if (num_pairs < 2)
> num_pairs = 2;
>
> - struct newir {
> -irange range;
> -tree mem[1];
> - };
> - size_t nbytes = (sizeof (newir) + sizeof (tree) * 2 * (num_pairs - 1));
> - struct newir *r = (newir *) obstack_alloc (_obstack, nbytes);
> - return new (r) irange (r->mem, num_pairs);
> + // Allocate the irange and required memory for the vector.
> + struct irange *r = (irange *) obstack_alloc (_obstack, sizeof (irange));
> +
> + size_t nbytes = sizeof (tree) * 2 * num_pairs;
> + tree *mem = (tree *) obstack_alloc (_obstack, nbytes);
> +
> + return new (r) irange (mem, num_pairs);
> }
>
> inline irange *
Jakub
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On 10/6/20 1:32 PM, Jakub Jelinek via Gcc-patches wrote:
On Tue, Oct 06, 2020 at 10:42:12AM -0600, Martin Sebor wrote:
The manual documents the [0] extension and mentions but discourages
using [1]. Nothing is said about other sizes and the warnings such
as -Warray-bounds have been increasingly complaining about accesses
past the declared constant bound (it won't complain about past-
the-end accesses to a mem[1], but will about those to mem[2]).
It would be nice if existing GCC code could eventually be converted
to avoid relying on the [1] hack. I would hope we would avoid making
use of it in new code (and certainly avoid extending its uses to other
sizes).
I don't see how we could, because [0] is an extension and GCC needs to
support host compilers that don't support it, and similarly [] is an
extension in C++ and can't be relied on.
Changing say rtl or gimple structs from the way we define them currently
to say templates dependent on the size of the embedded arrays is I'm
afraid not really possible.
Jakub
Aldy is currently testing this.. I presume this is what you had in mind?
Andrew
diff --git a/gcc/value-range.h b/gcc/value-range.h
index 94b48e55e77..c86301fe885 100644
--- a/gcc/value-range.h
+++ b/gcc/value-range.h
@@ -668,13 +668,13 @@ irange_allocator::allocate (unsigned num_pairs)
if (num_pairs < 2)
num_pairs = 2;
- struct newir {
-irange range;
-tree mem[1];
- };
- size_t nbytes = (sizeof (newir) + sizeof (tree) * 2 * (num_pairs - 1));
- struct newir *r = (newir *) obstack_alloc (_obstack, nbytes);
- return new (r) irange (r->mem, num_pairs);
+ // Allocate the irange and required memory for the vector.
+ struct irange *r = (irange *) obstack_alloc (_obstack, sizeof (irange));
+
+ size_t nbytes = sizeof (tree) * 2 * num_pairs;
+ tree *mem = (tree *) obstack_alloc (_obstack, nbytes);
+
+ return new (r) irange (mem, num_pairs);
}
inline irange *
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On Tue, Oct 06, 2020 at 10:42:12AM -0600, Martin Sebor wrote: > The manual documents the [0] extension and mentions but discourages > using [1]. Nothing is said about other sizes and the warnings such > as -Warray-bounds have been increasingly complaining about accesses > past the declared constant bound (it won't complain about past- > the-end accesses to a mem[1], but will about those to mem[2]). > > It would be nice if existing GCC code could eventually be converted > to avoid relying on the [1] hack. I would hope we would avoid making > use of it in new code (and certainly avoid extending its uses to other > sizes). I don't see how we could, because [0] is an extension and GCC needs to support host compilers that don't support it, and similarly [] is an extension in C++ and can't be relied on. Changing say rtl or gimple structs from the way we define them currently to say templates dependent on the size of the embedded arrays is I'm afraid not really possible. Jakub
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On 10/6/20 1:52 AM, Jakub Jelinek via Gcc-patches wrote:
On Tue, Oct 06, 2020 at 09:37:21AM +0200, Aldy Hernandez via Gcc-patches wrote:
Pushed as obvious.
gcc/ChangeLog:
* value-range.h (irange_allocator::allocate): Increase
newir storage by one.
---
gcc/value-range.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 (_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).
The manual documents the [0] extension and mentions but discourages
using [1]. Nothing is said about other sizes and the warnings such
as -Warray-bounds have been increasingly complaining about accesses
past the declared constant bound (it won't complain about past-
the-end accesses to a mem[1], but will about those to mem[2]).
It would be nice if existing GCC code could eventually be converted
to avoid relying on the [1] hack. I would hope we would avoid making
use of it in new code (and certainly avoid extending its uses to other
sizes).
If it's difficult to write efficient C++ code without relying on
these hacks we are in the perfect position to propose a solution
to C++. Otherwise, if a portable solution already exists, we
should be able to adopt it.
Martin
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;
irange *r = (irange *) obstack_alloc (_obstack, nbytes);
return new (r) irange ((tree *) (r + 1), num_pairs);
without any new type.
Jakub
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On 10/6/20 8:55 AM, Jakub Jelinek wrote: On Tue, Oct 06, 2020 at 08:47:53AM -0400, Andrew MacLeod wrote: I think the proper alignment will be guaranteed if irange and tree[] are obstack_alloc'd separately. They don't need to be adjacent, do they? They do not, it just seemed wasteful to do 2 allocs each time, and it'd be nice to have them co-located since accessing one inevitable leads to accessing the other. When using normal allocator like malloc or ggc allocation I'd totally agree here, but I actually think with obstack it is better to do two successive allocations. obstack_alloc is generally pretty cheap, in the common case there is room for the allocation and so it just bumps the next pointer in the structure and that is it, for the two allocation the same like with one. And, if there is room just for the irange and not for the subsequent allocation, with two allocations they wouldn't be collocated, but wouldn't waste the memory that would otherwise remain unused. Jakub okeydoke then. we can do 2 allocations.
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On Tue, Oct 06, 2020 at 08:47:53AM -0400, Andrew MacLeod wrote: > > I think the proper alignment will be guaranteed if irange and tree[] are > > obstack_alloc'd separately. They don't need to be adjacent, do they? > > > > > They do not, it just seemed wasteful to do 2 allocs each time, and it'd be > nice to have them co-located since accessing one inevitable leads to > accessing the other. When using normal allocator like malloc or ggc allocation I'd totally agree here, but I actually think with obstack it is better to do two successive allocations. obstack_alloc is generally pretty cheap, in the common case there is room for the allocation and so it just bumps the next pointer in the structure and that is it, for the two allocation the same like with one. And, if there is room just for the irange and not for the subsequent allocation, with two allocations they wouldn't be collocated, but wouldn't waste the memory that would otherwise remain unused. Jakub
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
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 (_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
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On 10/6/20 6:40 AM, Andreas Schwab wrote: On Okt 06 2020, Jakub Jelinek wrote: On Tue, Oct 06, 2020 at 10:47:34AM +0200, Andreas Schwab wrote: On Okt 06 2020, Jakub Jelinek via Gcc-patches wrote: I mean, we could just use: size_t nbytes = sizeof (irange) + sizeof (tree) * 2 * num_pairs; irange *r = (irange *) obstack_alloc (_obstack, nbytes); return new (r) irange ((tree *) (r + 1), num_pairs); without any new type. Modulo proper alignment. Sure, but irange's last element is tree * which is pointer to pointer, and we need here an array of tree, i.e. pointers. So, it would indeed break on a hypothetical host that has smaller struct X ** alignment than struct X * alignment. I'm not aware of any. One could add a static_assert to verify that (that alignof (irange) >= alignof (tree) and that sizeof (irange) % alignof (tree) == 0). I think the proper alignment will be guaranteed if irange and tree[] are obstack_alloc'd separately. They don't need to be adjacent, do they? They do not, it just seemed wasteful to do 2 allocs each time, and it'd be nice to have them co-located since accessing one inevitable leads to accessing the other.
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On Okt 06 2020, Jakub Jelinek wrote: > On Tue, Oct 06, 2020 at 10:47:34AM +0200, Andreas Schwab wrote: >> On Okt 06 2020, Jakub Jelinek via Gcc-patches wrote: >> >> > I mean, we could just use: >> > size_t nbytes = sizeof (irange) + sizeof (tree) * 2 * num_pairs; >> > irange *r = (irange *) obstack_alloc (_obstack, nbytes); >> > return new (r) irange ((tree *) (r + 1), num_pairs); >> > without any new type. >> >> Modulo proper alignment. > > Sure, but irange's last element is tree * which is pointer to pointer, > and we need here an array of tree, i.e. pointers. So, it would indeed break > on a hypothetical host that has smaller struct X ** alignment than struct X * > alignment. I'm not aware of any. > One could add a static_assert to verify that (that alignof (irange) >= > alignof (tree) > and that sizeof (irange) % alignof (tree) == 0). I think the proper alignment will be guaranteed if irange and tree[] are obstack_alloc'd separately. They don't need to be adjacent, do they? Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
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 (_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);
> > 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
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On 10/6/20 9:52 AM, Jakub Jelinek wrote:
On Tue, Oct 06, 2020 at 09:37:21AM +0200, Aldy Hernandez via Gcc-patches wrote:
Pushed as obvious.
gcc/ChangeLog:
* value-range.h (irange_allocator::allocate): Increase
newir storage by one.
---
gcc/value-range.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 (_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?
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.
Aldy
irange *r = (irange *) obstack_alloc (_obstack, nbytes);
return new (r) irange ((tree *) (r + 1), num_pairs);
without any new type.
Jakub
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On Tue, Oct 06, 2020 at 10:47:34AM +0200, Andreas Schwab wrote: > On Okt 06 2020, Jakub Jelinek via Gcc-patches wrote: > > > I mean, we could just use: > > size_t nbytes = sizeof (irange) + sizeof (tree) * 2 * num_pairs; > > irange *r = (irange *) obstack_alloc (_obstack, nbytes); > > return new (r) irange ((tree *) (r + 1), num_pairs); > > without any new type. > > Modulo proper alignment. Sure, but irange's last element is tree * which is pointer to pointer, and we need here an array of tree, i.e. pointers. So, it would indeed break on a hypothetical host that has smaller struct X ** alignment than struct X * alignment. I'm not aware of any. One could add a static_assert to verify that (that alignof (irange) >= alignof (tree) and that sizeof (irange) % alignof (tree) == 0). Jakub
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On Okt 06 2020, Jakub Jelinek via Gcc-patches wrote: > I mean, we could just use: > size_t nbytes = sizeof (irange) + sizeof (tree) * 2 * num_pairs; > irange *r = (irange *) obstack_alloc (_obstack, nbytes); > return new (r) irange ((tree *) (r + 1), num_pairs); > without any new type. Modulo proper alignment. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
Re: [PUSHED] Fix off-by-one storage problem in irange_allocator.
On Tue, Oct 06, 2020 at 09:37:21AM +0200, Aldy Hernandez via Gcc-patches wrote:
> Pushed as obvious.
>
> gcc/ChangeLog:
>
> * value-range.h (irange_allocator::allocate): Increase
> newir storage by one.
> ---
> gcc/value-range.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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 (_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).
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;
irange *r = (irange *) obstack_alloc (_obstack, nbytes);
return new (r) irange ((tree *) (r + 1), num_pairs);
without any new type.
Jakub
[PUSHED] Fix off-by-one storage problem in irange_allocator.
Pushed as obvious.
gcc/ChangeLog:
* value-range.h (irange_allocator::allocate): Increase
newir storage by one.
---
gcc/value-range.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 (_obstack, nbytes);
--
2.26.2
