[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 11:44:48 PST 2012
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