[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