This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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 <torvalds@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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]