Problem report: Struct aliasing problem causes Thread_Ready_Chain corruption in 4.6.99.3

[Joel Sherrill]

Thomas Doerfler (nt) wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Pavel,
>
> you are right, on some targets, my structure definition might get other
> alignments than intended.
>   
My attached patch seems to work and  does run the sptests on the SPARC.

I am using this structure and managed to remove the casts:

typedef union {
  struct {
    Chain_Node  Node;
    void       *filler;
  } Permanent_Head;
  struct {
    void       *filler;
    Chain_Node  Node;
  } Permanent_Tail;
  struct {
    Chain_Node *first;
    Chain_Node *permanent_null;
    Chain_Node *last;
  };
} Chain_Control;

> In that case I would like to discuss change the size of the
> "Chain_Control" structure. AFAIK, it has previously been defined to hold
> three pointers to save some memory (the "permanent_null" pointer ist
> shared between the nodes), but maybe it is not SO important. In that
> case, Chain_Control might also be defined as follows:
>
> typedef union {
>   struct Chain_Node_struct head;
>   struct Chain_Node_struct tail;
> } Chain_Control;
>
> What would get broken if we change to a 16 byte Chain_Control?
>   
My first thought is that it would break every operation involving the
first or last element on the list.

What does the chain look like empty? with 1 node?  with two nodes?

Does the header get automatically corrected when you go from 2 -> 1
node, from 1 -> 0 nodes?



> Any comments to this?
>
> wkr,
> Thomas.
>
> Pavel Pisa schrieb:
>   
>> Hello All,
>>
>> On Tuesday 21 November 2006 13:38, Thomas Doerfler wrote:
>>
>>     
>>> As far as I understood the problem, the following two data structures
>>> are used together, but GCC does not understand that they are sometimes
>>> aliased:
>>>
>>> struct Chain_Node_struct {
>>>  Chain_Node *next;
>>>  Chain_Node *previous;
>>> };
>>>
>>> typedef struct {
>>>  Chain_Node *first;
>>>  Chain_Node *permanent_null;
>>>  Chain_Node *last;
>>> } Chain_Control;
>>>
>>> An example for aliased use is:
>>>       
>> ....
>>
>>     
>>> Maybe it would help to modify the definition of Chain_Ctronol to
>>> something like:
>>>
>>> typedef union {
>>>  struct {
>>>    struct Chain_Node_struct head;
>>>    void *fill_loc1;
>>>  } head_node;
>>>
>>>  struct {
>>>    void *fill_loc1;
>>>    struct Chain_Node_struct tail;
>>>  } tail_node;
>>> } Chain_Control;
>>>
>>> The names are NOT good, and surely all chain.c/chain.inl functions would
>>> have to be adapted but this might be a mechanical search/replace procedure.
>>>
>>> Any comments to a change like this?
>>>       
>> I have strong feeling that above solution is highly risky.
>> There is no guarantee, that fields order in tail_node would not
>> be reordered. In the fact, there is high probability, that on some
>> targets order would be (4B fill_loc1, 4B spare, 8B tail_node)
>> or tail_node would be reordered to same layout as head_node
>> to follow alignment rules. I have feeling that that new ARM EABI tries
>> to preserve alignment to 8B for example and would expose these problems.
>>
>> The next solutions should work with GCC even, that they are not strictly
>> correct according to C99 (same union should not be accessed trough
>> different pattern than one which is used for it initialization
>> - it no problem for GCC, which is prepared for this use)
>>
>> typedef union {
>>     struct {
>>       Chain_Node *first;
>>       Chain_Node *permanent_null;
>>       Chain_Node *last;
>>     } control;
>>     struct {
>>       struct Chain_Node_struct head;
>>     } for_alias_marking_only;
>> } Chain_Control;
>>
>> If GCC extension allowing unnamed structures and unions fields
>> is explored, then definition can be changed like next example
>> without any core change requirements
>>
>>     
This attached patch uses the unnamed structure and passes all the tests.
What do you think of it?

>> typedef union {
>>     struct {
>>       Chain_Node *first;
>>       Chain_Node *permanent_null;
>>       Chain_Node *last;
>>     };
>>     struct {
>>       struct Chain_Node_struct head;
>>     } for_alias_marking_only;
>> } Chain_Control;
>>
>> I think, that next solution through explicit pointers typecasting
>> is unsafe
>>
>>
>>     
>>> -      Chain_Append(&RTEMS_Malloc_GC_list, (Chain_Node *)ptr);
>>> +      Chain_Node *node = (Chain_Node *)ptr;
>>> +      Chain_Append(&RTEMS_Malloc_GC_list, node);
>>>       
>> It has no effect on the aliasing rules implications.
>> It can have effect only due some different compiler
>> resolution/encoding of differently described prescription.
>> But it should lead to exactly same code for optimally optimizing
>> compiler.
>>
>>     
I threw this one out.  The chain is the problem at hand.

>> Best wishes
>>
>>             Pavel Pisa
>> _______________________________________________
>> rtems-users mailing list
>> rtems-users at rtems.com
>> http://rtems.rtems.org/mailman/listinfo/rtems-users
>>     
>
>
> - --
> - --------------------------------------------
> IMD Ingenieurbuero fuer Microcomputertechnik
> Thomas Doerfler           Herbststrasse 8
> D-82178 Puchheim          Germany
> email:    Thomas.Doerfler at imd-systems.de
> PGP public key available at:
>      http://www.imd-systems.de/pgpkey_en.html
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.2.5 (MingW32)
> Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
>
> iD8DBQFFY1ScwHyg4bDtfjQRAtDcAJsG9spwK7tK0mGEW08boGyvr3jVFACcCM0a
> p2KCBSgOvVOzi/4UjPX1iw8=
> =6B/d
> -----END PGP SIGNATURE-----
> _______________________________________________
> rtems-users mailing list
> rtems-users at rtems.com
> http://rtems.rtems.org/mailman/listinfo/rtems-users
>   

-------------- next part --------------
A non-text attachment was scrubbed...
Name: chain_casts_2.diff
Type: text/x-patch
Size: 8148 bytes
Desc: not available
URL: <http://lists.rtems.org/pipermail/users/attachments/20061121/a1940b62/attachment-0001.bin>