[llvm-commits] [LLVM, PR11652 PATCH]: Fixed Bug 11652 - assertion failures when Type.cpp is compiled with -Os
Stepan Dyatkovskiy
STPWORLD at narod.ru
Tue Jan 3 12:08:35 PST 2012
OK. Commited as r147470.
-Stepan.
03.01.2012, 23:44, "Duncan Sands" <baldrick at free.fr>:
> Hi Stepan,
>
>> Hi, Duncan. Please find the second patch attached.
>
> looks good to me. Please put the include for DataTypes after the
> include for Casting to keep them in alphabetical order and then apply.
>
> Ciao, Duncan.
>
>> -Stepan.
>>
>> 03.01.2012, 18:21, "Duncan Sands"<baldrick at free.fr>:
>>> 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