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

[Duncan Sands]

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