Re: Memory corruption due to word sharing
On 02/07/2012 05:32 AM, Aldy Hernandez wrote:
struct A {
virtual void f();
int f1 : 1;--- bit 64
};
struct B : public A {
int f2 : 1; // { dg-warning ABI }--- bit 65
int : 0;
int f3 : 4;
int f4 : 3;
};
It is my understanding that f1 and f2 must be in distinct memory
regions. So writing to f1 cannot clobber f2.
That's my understanding as well.
Jason
Re: Memory corruption due to word sharing
Testcase is for example g++.dg/abi/bitfield5.C, bit layout annotated:
struct A {
virtual void f();
int f1 : 1;--- bit 64
};
struct B : public A {
int f2 : 1; // { dg-warning ABI }--- bit 65
int : 0;
int f3 : 4;
int f4 : 3;
};
maybe it was a bug (above happens with -fabi-version=1 only),
but certainly an ABI may specify that we should do that packing.
What does the C++ memory model say here? (incidentially that's
one case I was worried about when reviewing your patches,
just I didn't think of _bitfield_ tail-packing ... ;)).
I suppose I could just force the bitfield region to start
at a byte boundary.
I think we talked about this months ago when working on the memory model
stuff. Andrew Macleod brought it up, but I can't find the thread.
It is my understanding that f1 and f2 must be in distinct memory
regions. So writing to f1 cannot clobber f2.
I would like to get confirmation from Jason though.
Re: Memory corruption due to word sharing
(Somehow my reply was private to Aldy ... forwarding to gcc-patches
now, given that it contains a patch and we changed topics)
On Fri, 3 Feb 2012, Richard Guenther wrote:
On Thu, 2 Feb 2012, Aldy Hernandez wrote:
Linus Torvalds torva...@linux-foundation.org writes:
Seriously - is there any real argument *against* just using the base
type as a hint for access size?
If I'm on the hook for attempting to fix this again, I'd also like to
know if there are any arguments against using the base type.
Well, if you consider
struct {
int i : 1;
char c;
};
then you'll realize that 'i' has SImode (and int type) but the
underlying bitfield has only 1 byte size (thus, QImode) and
'c' starts at offset 1.
So no, you cannot use the base type either.
I've playing with the following patch yesterday, which computes
an underlying object for all bitfields and forcefully lowers
all bitfield component-refs to use that underlying object
(just to check correctness, it doesn't generate nice code as
BIT_FIELD_REF on memory is effectively resulting in the same
code as if using the bitfield FIELD_DECLs directly - we'd
need to explicitely split things into separate stmts with RMW
cycles).
You should be able to re-use the underlying object compute though
(and we can make it more intelligent even) during expansion
for the C++ memory model (and in fact underlying object compute
might just do sth different dependent on the memory model in
effect).
Disclaimer: untested.
The following works (roughly, still mostly untested). SRA needs
a fix (included) and the gimplify.c hunk really only shows what
we are supposed to be able to do (access the representative).
As-is SRA could now do a nice job on bitfields, but that needs
some changes - or we lower all bitfield ops in some extra pass
(if not then expand would need to be changed to look at the
representatives instead).
Still the idea is to compute all these things up-front during
type layout instead of re-discovering them at each bitfield
access we expand in get_bit_range. And we can use that information
consistently across passes.
We should of course try harder to avoid adding a new field to
struct tree_field_decl - DECL_INITIAL came to my mind, but
the C frontend happens to use that for bitfields ... while
it probably could as well use lang_type.enum_{min,max}?
Comments?
Thanks,
Richard.
2012-02-03 Richard Guenther rguent...@suse.de
* tree.h (DECL_BIT_FIELD_REPRESENTATIVE): New define.
(struct tree_field_decl): New field bit_field_representative.
* stor-layout.c (start_bitfield_representative): New function.
(finish_bitfield_representative): Likewise.
(finish_bitfield_layout): Likewise.
(finish_record_layout): Call finish_bitfield_layout.
* gimplify.c (gimplify_expr): Translate bitfield accesses
to BIT_FIELD_REFs of the representative.
* tree-sra.c (create_access_replacement): Only rename the
replacement if we can rewrite it into SSA form. Properly
mark register typed replacements that we cannot rewrite
with TREE_ADDRESSABLE.
Index: gcc/stor-layout.c
===
*** gcc/stor-layout.c.orig 2012-02-03 10:42:49.0 +0100
--- gcc/stor-layout.c 2012-02-03 12:37:38.0 +0100
*** finalize_type_size (tree type)
*** 1722,1727
--- 1722,1889
}
}
+ /* Return a new underlying object for a bitfield started with FIELD. */
+
+ static tree
+ start_bitfield_representative (tree field)
+ {
+ tree repr = make_node (FIELD_DECL);
+ DECL_FIELD_OFFSET (repr) = DECL_FIELD_OFFSET (field);
+ DECL_FIELD_BIT_OFFSET (repr) = DECL_FIELD_BIT_OFFSET (field);
+ SET_DECL_OFFSET_ALIGN (repr, DECL_OFFSET_ALIGN (field));
+ DECL_SIZE (repr) = DECL_SIZE (field);
+ DECL_PACKED (repr) = DECL_PACKED (field);
+ DECL_CONTEXT (repr) = DECL_CONTEXT (field);
+ return repr;
+ }
+
+ /* Finish up a bitfield group that was started by creating the underlying
+object REPR with the last fied in the bitfield group FIELD. */
+
+ static void
+ finish_bitfield_representative (tree repr, tree field)
+ {
+ unsigned HOST_WIDE_INT bitsize, maxbitsize, modesize;
+ enum machine_mode mode;
+ tree nextf, size;
+
+ size = size_diffop (DECL_FIELD_OFFSET (field),
+ DECL_FIELD_OFFSET (repr));
+ gcc_assert (host_integerp (size, 1));
+ bitsize = (tree_low_cst (size, 1) * BITS_PER_UNIT
++ tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1)
+- tree_low_cst (DECL_FIELD_BIT_OFFSET (repr), 1)
++ tree_low_cst (DECL_SIZE (field), 1));
+
+ /* Now nothing tells us how to pad out bitsize ... */
+ nextf = DECL_CHAIN (field);
+ while (nextf TREE_CODE (nextf) != FIELD_DECL)
+ nextf = DECL_CHAIN (nextf);
+ if (nextf)
+ {
+ tree maxsize = size_diffop (DECL_FIELD_OFFSET (nextf),
+
Re: Memory corruption due to word sharing
On Fri, 3 Feb 2012, Richard Guenther wrote:
On Fri, 3 Feb 2012, Richard Guenther wrote:
On Thu, 2 Feb 2012, Aldy Hernandez wrote:
Linus Torvalds torva...@linux-foundation.org writes:
Seriously - is there any real argument *against* just using the base
type as a hint for access size?
If I'm on the hook for attempting to fix this again, I'd also like to
know if there are any arguments against using the base type.
Well, if you consider
struct {
int i : 1;
char c;
};
then you'll realize that 'i' has SImode (and int type) but the
underlying bitfield has only 1 byte size (thus, QImode) and
'c' starts at offset 1.
So no, you cannot use the base type either.
I've playing with the following patch yesterday, which computes
an underlying object for all bitfields and forcefully lowers
all bitfield component-refs to use that underlying object
(just to check correctness, it doesn't generate nice code as
BIT_FIELD_REF on memory is effectively resulting in the same
code as if using the bitfield FIELD_DECLs directly - we'd
need to explicitely split things into separate stmts with RMW
cycles).
You should be able to re-use the underlying object compute though
(and we can make it more intelligent even) during expansion
for the C++ memory model (and in fact underlying object compute
might just do sth different dependent on the memory model in
effect).
Disclaimer: untested.
The following works (roughly, still mostly untested). SRA needs
a fix (included) and the gimplify.c hunk really only shows what
we are supposed to be able to do (access the representative).
As-is SRA could now do a nice job on bitfields, but that needs
some changes - or we lower all bitfield ops in some extra pass
(if not then expand would need to be changed to look at the
representatives instead).
Still the idea is to compute all these things up-front during
type layout instead of re-discovering them at each bitfield
access we expand in get_bit_range. And we can use that information
consistently across passes.
We should of course try harder to avoid adding a new field to
struct tree_field_decl - DECL_INITIAL came to my mind, but
the C frontend happens to use that for bitfields ... while
it probably could as well use lang_type.enum_{min,max}?
Comments?
Funnily C++ uses tail-padding of base types to pack bitfields
and thus I run into
gcc_assert (maxbitsize % BITS_PER_UNIT == 0);
Testcase is for example g++.dg/abi/bitfield5.C, bit layout annotated:
struct A {
virtual void f();
int f1 : 1; --- bit 64
};
struct B : public A {
int f2 : 1; // { dg-warning ABI }--- bit 65
int : 0;
int f3 : 4;
int f4 : 3;
};
maybe it was a bug (above happens with -fabi-version=1 only),
but certainly an ABI may specify that we should do that packing.
What does the C++ memory model say here? (incidentially that's
one case I was worried about when reviewing your patches,
just I didn't think of _bitfield_ tail-packing ... ;)).
I suppose I could just force the bitfield region to start
at a byte boundary.
Richard.
