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

Duncan Sands baldrick at free.fr
Tue Jan 3 06:21:18 PST 2012


Hi Stepan,

> 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");
> }

yes, something like that.  Better to use

   assert(getTypeID() == ID && "Type data too large for field!");

though IMO.

Ciao, Duncan.
>
> -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




More information about the llvm-commits mailing list