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

[Chandler Carruth]

OK!

With all that discussion, here is an updated patch. I've included most of
the tricky test cases described here (where they seemed valuable) along
with lots of comments to help document some of this discussion and the
conclusions.

Any test cases missing? Anything else I need to do to get this ready for
commit?


On Wed, Sep 12, 2012 at 3:46 AM, John McCall <rjmccall at apple.com> wrote:

> On Sep 12, 2012, at 1:03 AM, Richard Smith wrote:
>
> On Tue, Sep 11, 2012 at 10:52 PM, John McCall <rjmccall at apple.com> wrote:
>
>>
>> On Sep 11, 2012, at 8:19 PM, Richard Smith wrote:
>>
>> On Tue, Sep 11, 2012 at 6:47 PM, John McCall <rjmccall at apple.com> wrote:
>>
>>> 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;
>>>   };
>>>
>>
>> [The common initial sequence exemption only applies to structs as union
>> members, so that case is fine. Assuming these are each wrapped in a
>> struct...]
>>
>>
>> I think you misread what I wrote or assumed it away.  I wrote, and meant,
>> a struct;  there's no union here.
>>
>
> Yes, you're absolutely right, I misread the 'struct' as 'union'. My
> apologies.
>
>
>> I am saying that I believe it is legal to widen a load so that it spans
>> other data that happens to include a volatile object.  That is, it is
>> perfectly legal to touch a volatile object when not requested to, as long
>> as you (1) implement the actual requested accesses exactly as the abstract
>> machine specifies, (2) don't introduce data races as covered by
>> [intro.memory], and (3) don't violate your implementation-defined semantics
>> for volatile accesses.
>>
>
> I see. C++ does not include C's statement that "what constitutes an access
> to an object that has volatile-qualified type is implementation-defined",
> but I suppose we can assume it's intended, based on the non-normative text
> in [dcl.type.cv]p7 which says that the intent is to match C.
>
>
> Right, that's what I'm arguing.  We have the flexibility to restrict
> volatile semantics so that we don't have to obsessively worry about
> volatile chars in every nook and shadow.
>
> That said, I agree that we should also pursue the defect that Chandler
> described.
>
> John.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120913/8c1ee288/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR13691.patch
Type: application/octet-stream
Size: 67150 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120913/8c1ee288/attachment.obj>