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

[Stepan Dyatkovskiy]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 11652-2.0-getTypeID.patch
Type: application/octet-stream
Size: 8639 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120102/94ad7d5c/attachment.obj>