Strict aliasing and chains revisited

[Till Straumann]

On 10/25/2010 12:38 AM, Sebastian Huber wrote:
> On 10/21/2010 05:42 PM, Till Straumann wrote:
>>   On 10/21/2010 12:51 AM, Sebastian Huber wrote:
>>> On 10/21/2010 01:49 AM, Till Straumann wrote:
>>>>    I didn't think about this carefully but I guess a clean solution
>>>> would be sacrificing the space for 1 pointer and make the
>>>> Chain_Control a proper struct comprised of 2 Chain_Nodes.
>>> The chain is a pivot data structure in RTEMS and some operations are
>>> not that
>>> efficient if you sacrifice the node overlap.  Also chain controls are
>>> used in
>>> great numbers (one ready chain per task priority).  We should keep
>>> that structure.
>>>
>>>> A hacked solution would be making Chain_Control a (hopefully properly
>>>> packed)
>>>>
>>>> union Chain_Control {
>>>>      struct {
>>>>         Chain_Node node;
>>>>         Chain_Node *fill;
>>>>      } head;
>>>>      struct {
>>>>         Chain_Node  *fill;
>>>>         Chain_Node  node;
>>>>      } tail;
>>>> } Chain_Control;
>>> Yes, this is an alternative to the casts.  I have no preference here.
>> I don't think the casts remove the alias problem. The fact
>> that it doesn't seem to happen doesn't guarantee that the
>> code is correct (gcc behavior changes).
>>
>> I would feel more comfortable with a proper declaration,
>> i.e., avoiding type punning.
> Ok, I changed the Chain_Control definition to:
>
> typedef union {
>      struct {
>        Chain_Node Node;
>        Chain_Node *fill;
>      } Head;
>
>      struct {
>        Chain_Node *fill;
>        Chain_Node Node;
>      } Tail;
> } __attribute__ ((packed)) Chain_Control;
>
> Test suites pass on ARM, MIPS and SPARC.  One problem with this change is that
> the field names change, and this may break applications which use this internal
> API.
>
This can also be seen as an advantage since it exposes
violations of the alias rule.

T.
>>> Till, what do you think about the change in watchdoginsert.c?
>> If there are compiler memory  barriers in place then
>> there should be no problem. You mentioned the barrier
>> in ISR_Flash() -- is there also a barrier before entry
>> to the routine? This might not be necessary - I'm travelling
>> and don't have time + access to the code for proper
>> analysis, sorry - but we should double-check.
> [...]
>
> Yes, before the _ISR_Flash() there is an _ISR_Disable() which is also a memory
> barrier.
>