[llvm-commits] [LLVM, PR11652 PATCH]: Fixed Bug 11652 - assertion failures when Type.cpp is compiled with -Os

[Stepan Dyatkovskiy]

Oh.. of course. Commited as r147446. For next patch, did you mean that I should check that TypeID was properly stored in its 8 bits? Something like this:

void setTypeID(TypeID ID) {
   IDAndSubclassData = (ID & 0xFF) | (IDAndSubclassData & 0xFFFFFF00);
   assert(IDAndSubclassData & 0xFF == ID && "Type data too large for field");
}

-Stepan

03.01.2012, 12:51, "Duncan Sands" <baldrick at free.fr>:
> Hi Stepan,
>
>>  Hi Duncan, can I use DataTypes.h instead?
>
> this patch doesn't require either.  It is the next patch that needs it, right?
>
> Ciao, Duncan.
>
>>  -Stepan.
>>
>>  03.01.2012, 12:32, "Duncan Sands"<baldrick at free.fr>:
>>>  Hi Stepan, this looks fine except for a pointless include of stdint.h.  Please
>>>  apply, except for the include.
>>>
>>>  Thanks for doing this,
>>>
>>>  Duncan.
>>>>    Hi, Duncan. Please find the first patch in attachment. Replacement: ID with getTypeID().
>>>>    - Stepan
>>>>
>>>>    02.01.2012, 19:25, "Duncan Sands"<baldrick at free.fr>:
>>>>>    Hi Stepan,
>>>>>>      OK. Please look at patch in attachment.
>>>>>>      I'm not sure that it is better than previous patch. Probably the first one looks like a workaround, but it changes setSubclassData only. New patch changes set/getSubclassData set/getTypeID, and all methods that uses ID.
>>>>>    thanks for doing this.  I think it is a better abstraction to have getters
>>>>>    and setters for ID, like the ones that already exist for SubclassData.  Can
>>>>>    you therefore split the patch in two: one patch that adds getters and setters,
>>>>>    and then a second one that drops the bitfield in favour of explicit bit
>>>>>    fiddling.
>>>>>
>>>>>    Additional comments:
>>>>>        - you made some lines too long (>    80 columns).
>>>>>        - this is not your fault, but I think there should be a check that ID values
>>>>>          fit in the allocated space, for example by checking somehow that there is
>>>>>          enough room for every value of the TypeID type.  Alternatively, in setTypeID
>>>>>          check that the value you read back out matches the value put in.  The
>>>>>          constructor can also set the ID.  It should probably initialize
>>>>>          IDAndSubclassData to zero, and then call setTypeID in the body of the
>>>>>          constructor to set the value.
>>>>>
>>>>>    Ciao, Duncan.
>>>>>>      -Stepan.
>>>>>>
>>>>>>      02.01.2012, 15:04, "Duncan Sands"<baldrick at free.fr>:
>>>>>>>      Hi Stepan,
>>>>>>>>        ID is used very extensively in Type.h. We need to fix a lots, so we need to fix all methods like:
>>>>>>>>        bool isIntegerTy() const { return ID == IntegerTyID; }
>>>>>>>      you could turn ID into a private method that extracts the id part of the field.
>>>>>>>      Then you just need to turn ID into ID() in places such as isIntegerTy.  Likewise
>>>>>>>      for SubclassData.
>>>>>>>>        But in the same time we can apply some working decision until gcc bug will fixed.
>>>>>>>>        May be add some dummy field?
>>>>>>>>           TypeID   ID : 8;
>>>>>>>>           unsigned SubclassData : 24;
>>>>>>>>           unsigned KungFuPanda;        // Will protect NumContainedTys from overwriting.
>>>>>>>>           unsigned NumContainedTys; // Will OK.
>>>>>>>      Even if the gcc bug is fixed, people will be using  older compilers with the bug
>>>>>>>      for years to come.  So this field would be around essentially forever.  Given
>>>>>>>      that, I don't think this is a good solution.  If you are prepared to make the
>>>>>>>      class bigger, you might as well not have the fields be bitfields at all (and
>>>>>>>      change the order so that things are well packed).
>>>>>>>
>>>>>>>      Ciao, Duncan.
>>>>>>>>        -Stepan.
>>>>>>>>
>>>>>>>>        02.01.2012, 14:38, "Duncan Sands"<baldrick at free.fr>:
>>>>>>>>>        Hi Stepan,
>>>>>>>>>>          I tried it doesn't helps. Now it seems that ID is overwritten. 4807 unexpected failures.
>>>>>>>>>        OK, thanks for the info.  How about doing the bit fiddling yourself instead?
>>>>>>>>>        I.e. rather than trying to fool the optimizers, don't use bitfields: declare
>>>>>>>>>        an unsigned field IDAndSubclassData and store and load values from it using
>>>>>>>>>        explicit shifts etc.  This would then completely avoid all problems coming
>>>>>>>>>        from misoptimization of bitfields (which has happened a lot historically),
>>>>>>>>>        and would be less fragile than trying to fool the optimizers via some magic
>>>>>>>>>        incantation.
>>>>>>>>>
>>>>>>>>>        Ciao, Duncan.
>>>>>>>>>>          -Stepan.
>>>>>>>>>>
>>>>>>>>>>          02.01.2012, 14:02, "Duncan Sands"<baldrick at free.fr>:
>>>>>>>>>>>          Hi Stepan,
>>>>>>>>>>>>            The problem is in Type.h. The fields in Type class are declared in next order:
>>>>>>>>>>>>            TypeID ID : 8;
>>>>>>>>>>>>            unsigned SubclassData : 24;
>>>>>>>>>>>>            unsigned NumContainedTys;
>>>>>>>>>>>          does the problem still occur if you flip the order of ID and SubclassData?
>>>>>>>>>>>          I.e.
>>>>>>>>>>>              unsigned SubclassData : 24;
>>>>>>>>>>>              TypeID ID : 8;
>>>>>>>>>>>              unsigned NumContainedTys;
>>>>>>>>>>>          ?
>>>>>>>>>>>          Ciao, Duncan.
>>>>>>>>>>>>            Attempt to set new SubclassData value rewrites lowest byte in NumContainedTys
>>>>>>>>>>>>            when -Os is set. GCC bug? Anyway setting SubclassData with two workaround
>>>>>>>>>>>>            strings fixes the problem:
>>>>>>>>>>>>
>>>>>>>>>>>>            void setSubclassData(unsigned val) {
>>>>>>>>>>>>            unsigned tmp = NumContainedTys; // Workaround for GCC -Os
>>>>>>>>>>>>            SubclassData = val;
>>>>>>>>>>>>            NumContainedTys = tmp; // Workaround for GCC -Os
>>>>>>>>>>>>            // Ensure we don't have any accidental truncation.
>>>>>>>>>>>>            assert(SubclassData == val&&          "Subclass data too large for field");
>>>>>>>>>>>>            }
>>>>>>>>>>>>
>>>>>>>>>>>>            Probably there is another ways to protect NumContainedTys from overwritting?
>>>>>>>>>>>>
>>>>>>>>>>>>            Please find the patch in attachment for review.
>>>>>>>>>>>>
>>>>>>>>>>>>            -Stepan.
>>>>>>>>>>>>
>>>>>>>>>>>>            _______________________________________________
>>>>>>>>>>>>            llvm-commits mailing list
>>>>>>>>>>>>            llvm-commits at cs.uiuc.edu
>>>>>>>>>>>>            http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>>>>>>>          _______________________________________________
>>>>>>>>>>>          llvm-commits mailing list
>>>>>>>>>>>          llvm-commits at cs.uiuc.edu
>>>>>>>>>>>          http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits