Fwd: Problem report: Struct aliasing problem causes Thread_Ready_Chain corruption in 4.6.99.3

[Joel Sherrill]

Till Straumann wrote:
> **BEWARE**
>
> Thomas is right. And after reading the aforementioned article
> I tried their example (simplified):
>
> inline void
> writethree(int *p)
> {
> short *palias = (short*)p;
>    /* compiler assumes 'palias' and 'p' *dont* address the same memory 
> region */
>    *palias = 3;
> }
>
> int
> testalias()
> {
> int value = 0xdeadbeef;
>     writethree(&value);
>     return value;
> }
>
> When I compile (gcc-4.1.2, gcc-4.1.1) this with
>
> -fstrict-aliasing -O -Wstrict-aliasing=2
>
> then the call to 'writethree' gets optimized away
> **w/o warning** (since 'palias' is not a valid alias of  'p').
>
>   
I just compiled one of the sparc BSPs with all tests enabled and got

243 warnings with -Wstrict-alias=2
63 warnings without -Wstrict-alias=2

That's just one BSP and one arbitrary set of libcpu and libbsp code.  There
are about 90 BSPs when you count variations so there is a lot of uncompiled
code let to give more warnings.

I am beginning to be very scared of making a release with this 
strict-alias enabled.
On one hand, there has been a LOT of successful testing and on the 
other, there is
Peer's edge condition where something broke in a painful manner.  And we 
know
testing does not prove the absence of errors only the ability of our tests.

Ralf.. to be conservative, let's turn off strict-alias'ing for 4.7 and 4.8.
In addition, turn on -Wstrict-alias=2 for 4.8.  Hopefully we can resolve the
warnings and work from there.

Peer.. I would appreciate it if you could use my last patch and see if 
it generates
correct code in your situation.  If it does, I will see if I can get a 
ruling from
some gcc person on how dangerous the code is for the future.

> I must admit that I didn't fully understand the
> strict-alias rule until today and I'm a bit shocked.
> I'm sure it breaks code I've written.
>
>   

It seems like it could break a lot of device drivers and communications
protocol code.  The above type of code isn't that hard to find in device
drivers.
> Until every cast has been hand-checked carefully
> I see no way around -fno-strict-aliasing
>
> (And, yes, the 'intermediate variable fix' seems to
> not fix anything except silencing the warning).
>
> If gcc sees enough of the source code (e.g. when inlining)
> to know exactly what the 'original' pointers were
> (prior to any casting and using intermediate variables)
> then it 'ruthlessly' enforces aliasing rules no
> matter what casting you do:
>
> Consider this:
>
> typedef union {
>     int   i;
>     short s;
> } u2;
>
> int rtn3()
> {
> int   rval = 3;
> u2    *pu  = &rval;
> short *ps = (short*)pu;
>     *ps = 0xdead;
>     return rval;
> }
>
> Even though casting a int* into u2* is legal
> (u2 contains a int member) and u2* to short*
> is, too (u2 also contains a short member)
> gcc still optimizes *ps=0xdead away because
> it says: at the end of the day, ps must not be
> an alias of  &rval and it knows that both
> point to the same address.
> OTOH, setting
>
> pu->s = 0xdead;
>
> would not go away.
>
> If code was not written from the ground up with the
> strict-aliasing rule in mind then it seems hard to
> ensure the rule is not violated.
>
> -- Till
>
> Thomas Doerfler wrote:
>   
>> Ralf,
>>
>> Ralf Corsepius schrieb:
>>   
>>     
>>> On Tue, 2006-11-21 at 16:38 +0100, Thomas Doerfler wrote:
>>>
>>>     
>>>       
>>>> Therefore your patch in malloc.c is IMHO identical to the previous
>>>> explicit cast.
>>>>       
>>>>         
>>> Please prove Joel's patch to be broken.
>>>     
>>>       
>> maybe it is my limited horizon, but I don't see the difference applied
>> by the following patch:
>>
>>    if (_System_state_Is_up(_System_state_Get())) {
>>      if ((_Thread_Dispatch_disable_level > 0) || (_ISR_Nest_level > 0)) {
>> -      Chain_Append(&RTEMS_Malloc_GC_list, (Chain_Node *)ptr);
>> +      Chain_Node *node = (Chain_Node *)ptr;
>> +      Chain_Append(&RTEMS_Malloc_GC_list, node);
>>        return;
>>
>> The intermediate local variable "node" can easily be optimized by the
>> compiler. For the compiler, it should be obvious, that "node" is a
>> direct alias for "(Chain_Node *)ptr". So I don't see ANY reason, what
>> this additional variable would change in the compiler's behavior.
>>
>> It MAY BE, that this change produces different code, but I don't see,
>> why the code should be more correct.
>>
>> wkr,
>> Thomas.
>>
>>   
>>     
>>> Ralf
>>>
>>>
>>>     
>>>       
>>   
>>     
>
> _______________________________________________
> rtems-users mailing list
> rtems-users at rtems.com
> http://rtems.rtems.org/mailman/listinfo/rtems-users
>