[cfe-commits] PATCH: Large re-working of bitfield IR-gen, and a fix for PR13691

John McCall rjmccall at apple.com
Tue Sep 11 18:47:50 PDT 2012


On Sep 11, 2012, at 6:29 PM, Chandler Carruth wrote:
> On Tue, Sep 11, 2012 at 6:09 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Tue, Sep 11, 2012 at 6:04 PM, John McCall <rjmccall at apple.com> wrote:
> On Sep 11, 2012, at 4:41 PM, Richard Smith wrote:
> > On Tue, Sep 11, 2012 at 11:33 AM, John McCall <rjmccall at apple.com> wrote:
> > On Sep 11, 2012, at 11:07 AM, Chandler Carruth wrote:
> > > On Fri, Aug 31, 2012 at 1:01 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> > >> Another nasty case I just thought of:
> > >>
> > >> struct x { int i : 24; };
> > >> struct y { int i : 24; char j; };
> > >> union z {
> > >>   struct x x;
> > >>   struct y y;
> > >> };
> > >> union z a;
> > >> void f(int i) {
> > >>   a.x.i = i;
> > >> }
> > >> void g(char j) {
> > >>   a.y.j = j
> > >> }
> > >>
> > >> The two writes are to separate memory locations. :)
> > >
> > > Wait, hold on... I'm deeply confused. Maybe because I don't know how C11 unions work?
> > >
> > > With C++11, only one side of the union is allowed to be active, and so I don't think they are separate memory locations?
> >
> > I agree that this isn't a problem, but the analysis is a bit more complicated;
> > it hinges on the fact that it's okay to *read* from an inactive union member
> > under the common-prefix exception, but it's not okay to *write* to it.  The
> > same analysis applies in both standards:
> >
> > Is this still OK if the extra union member is volatile? Chandler and I have discussed this, and it's far from clear that it would be. (In particular, we can conceive of a seemingly-reasonable case where the union sits on an MMIO port, and only the fourth byte has volatile semantics.)
> 
> I see no reason why making the member volatile changes anything.
> Other members in the union can be assumed not to exist, because the
> active union member *must* be the one we're assigning to — indeed,
> in C such an assignment is how you change the active union member.
> 
> I'm talking about the load-widening case, not the store-widening. Given:
> 
> union U {
>   struct X { int a : 24; volatile char b; } x;
>   struct Y { int c : 24; } y;
> };
> 
> ... if x is the active member, and we load y.c, can we really load from the storage location containing x.b?
> 
> After going back and forth a few times amongst the committee members here, we propose this:
> 
> This is a defect. We should disallow reading from the common prefix of a union using a non-active member if there are any volatile members following the common prefix. There doesn't appear to be any way to support these union types and common-prefix reads and the committee's express desire that load widening is a valid compiler optimization.
> 
> Reasons why this truly seems like a defect:
> 
> 1) We can't even load widen through the padding bytes:
> 
> union U {
>   struct X { int a : 24; volatile char b; } x;
>   struct Y { int a : 24; int c; } y;
> };
> 
> 2) We can't even load widen when the next field in all members after the common prefix is non-volatile:
> 
> union U {
>   struct X { int a : 33; char b; volatile char c; } x;
>   struct Y { int a : 33; } y;
> };
> 
> 
> Seem plausible?

I could accept this as a defect.  I don't think it's required.

For example, I claim that, if we need x and z, we are allowed to use a 32-bit
load here (although, granted, it might not be a good idea):
  struct [[align(4)]] A {
    char x;
    volatile char y;
    char z;
  };

We are allowed to do this by dint of declaring "our implementation supports
memory-mapped I/O ports but does not define semantics for aggregates
which partially overlap them".  Voilà, the widened load can be assumed to
have no side effects and is therefore permitted.  If some kernel hackers want
to tell me that's unreasonable, then okay, we can negotiate something that
allows them to get work done while still generating efficient code for the
approximately all of our users who are not kernel hackers.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120911/a8099b6c/attachment.html>


More information about the cfe-commits mailing list