[Scummvm-devel] Broken Sword 2 problem

[Norbert Lange]

Am 01.09.2010, 13:49 Uhr, schrieb yotam barnoy <yotambarnoy at gmail.com>:

> Haven't tried your test, but I think that even if it seems to solve
> the problem, it may not be a real solution but an accidental one.
> There's still aliasing going on ie. the struct is in the same memory
> space as the uint32 or whatever we're reading/writing, and apparently
> that's a bad thing for an optimizing compiler.
>
> I'm currently experimenting with using the __may_alias__ gcc attribute
> which one of the gcc guys suggested to me. In other words, using
> __attribute__ ((__packed__, __may_alias__)).
>
> I tried another suggestion of his which caused crashing (not sure why):
>
> typedef uint32 __attribute__((aligned(1), __may_alias__))  
> unaligned_uint32;
> return *((unaligned_uint32 *)ptr);

I tried similar things when I wrote the improvements for unaligned access  
in endian.h, "aligned" is only used for specifying a minimum and thus  
making alignment bigger, it will still be greater (a multiple) of the  
type-size.
Also size alone doesnt matter to alignment of the variable since it only  
matters on which boundary the variable is stored, it will affect the  
variable stored after it though. So you need to have "packed" structs or  
arrays where the lack of necessary padding could result in unaligned  
variables, "packed" structs/arrays force the compiler to expect unaligned  
access (by gcc`s definition).

I dont know of another way to get gcc to emit unaligned access  
instructions and I spent quite some time fiddling with it, ironically MS`  
compilers have a beautiful "unaligned" declaration.. which doesnt mean  
much since the only architecture that could benefit from it is the  
deceased Itanium.

I`d wonder though if the case for architectures that dont need aligning  
(line 169-185) have the very same aliasing issues thanks to casting the  
pointer, only the fallback-case is safe by using char-type accesses.

Im not sure if I understood the C99 specs right tho - as far as I  
understand, only the effective memory-accesses should count, uint32 should  
be the same whether accessed through a struct or directly (which makes  
sense). On one hand, as long you keep using the same amount of bits for  
reading/writing all accesses should use the same uint16/uint32 type and  
the compiler should be aware of aliasing issues.... on the other hand  
apparently this dint safe the psp-toolchain from producing "wrong" code.  
Im still not sure if this is a compiler bug?!

PS. If you are using a recent psp-toolchain then __builtin_bswap32 is  
natively supported and preferable to the now used inline-assembly, my  
patch for the toolchain got included sometime last year. taking out  
"defined(__psp__)" at line 73 should be enough

>
> Hopefully __may_alias__ will do the trick.

yeah, should do.

Norbert

> Yotam
>
>
> On Wed, Sep 1, 2010 at 2:37 PM, Willem Jan Palenstijn <wjp at usecode.org>  
> wrote:
>> On Tue, Aug 31, 2010 at 06:26:41PM +0300, yotam barnoy wrote:
>>> For the (hopefully) last episode in this bug saga, see commit 52473.
>>
>> Wow, very nice catch! Well done!
>>
>> If I understand your commit message correctly, the issue is that some  
>> of the
>> READ_*_UINT* functions break strict aliasing rules? It would probably  
>> be good
>> to try to fix that instead of using -fno-strict-aliasing, if possible.
>>
>>
>> The subtleties of these strict aliasing rules still elude me, but it  
>> might be
>> the case that the issue is that the two Unaligned32 structs are local  
>> to their
>> functions, and might be considered different.
>>
>> I'm trying with this code:
>>
>> inline __attribute__((__always_inline__)) int read_value(const void*  
>> ptr)
>> {
>>        struct Unaligned32 { int val; } __attribute__ ((__packed__));
>>        return ((const Unaligned32 *)ptr)->val;
>> }
>>
>> inline __attribute__((__always_inline__)) void write_value(void* ptr,  
>> int value)
>> {
>>        struct Unaligned32 { int val; } __attribute__ ((__packed__));
>>        ((Unaligned32 *)ptr)->val = value;
>> }
>>
>> int test(char* x) {
>>        int y = read_value(x);
>>        write_value(x, y+1);
>>        return read_value(x);
>> }
>>
>> When compiled with g++ 4.5.1 on x86_64 with -O3 and strict aliasing, it  
>> returns
>> y, and not y+1.
>>
>> When I move the definition of Unaligned32 out of the functions, it  
>> returns y+1:
>>
>> struct Unaligned32 { int val; } __attribute__ ((__packed__));
>> inline __attribute__((__always_inline__)) int read_value(const void*  
>> ptr)
>> {
>>        return ((const Unaligned32 *)ptr)->val;
>> }
>> inline __attribute__((__always_inline__)) void write_value(void* ptr,  
>> int value)
>> {
>>        ((Unaligned32 *)ptr)->val = value;
>> }
>>
>> int test(char* x) {
>>        int y = read_value(x);
>>        write_value(x, y+1);
>>        return read_value(x);
>> }
>>
>>
>> Does it behave the same for you on your platform?
>>
>> -Willem Jan
>>
>
> ------------------------------------------------------------------------------
> This SF.net Dev2Dev email is sponsored by:
>
> Show off your parallel programming skills.
> Enter the Intel(R) Threading Challenge 2010.
> http://p.sf.net/sfu/intel-thread-sfd
> _______________________________________________
> Scummvm-devel mailing list
> Scummvm-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/scummvm-devel