Amd64 [un]fixes in SDL_endian.h

when we added libsdl-1.2.8 to Gentoo, amd64 users hit a rash of bad builds
with gcc-3.4 … simply trying to do something like build sdl_mixer would
reproduce the problem …

summary report (there were a lot of duplicates :D)
http://bugs.gentoo.org/show_bug.cgi?id=77300

the netbsd fix imported here:
http://www.libsdl.org/cgi/cvsweb.cgi/SDL12/include/SDL_endian.h.diff?r1=1.11&r2=1.12
made our problems go away nicely (i was able to reproduce original build
failures and confirm the fix on an amd64 dev box)

however, it has since been reverted:
http://www.libsdl.org/cgi/cvsweb.cgi/SDL12/include/SDL_endian.h.diff?r1=1.12&r2=1.13
due to this e-mail:
http://twomix.devolution.com/pipermail/sdl/2005-January/066828.html

that error is the exact error we were seeing in Gentoo with SDL_endian.h
rev1.11 and which rev1.12 fixed … so perhaps the report was based upon
older versions of SDL_endian.h and not the newer fixed one ?
-mike

Mike Frysinger wrote:

when we added libsdl-1.2.8 to Gentoo, amd64 users hit a rash of bad builds
with gcc-3.4 … simply trying to do something like build sdl_mixer would
reproduce the problem …

summary report (there were a lot of duplicates :D)
http://bugs.gentoo.org/show_bug.cgi?id=77300

the netbsd fix imported here:
http://www.libsdl.org/cgi/cvsweb.cgi/SDL12/include/SDL_endian.h.diff?r1=1.11&r2=1.12
made our problems go away nicely (i was able to reproduce original build
failures and confirm the fix on an amd64 dev box)

however, it has since been reverted:
http://www.libsdl.org/cgi/cvsweb.cgi/SDL12/include/SDL_endian.h.diff?r1=1.12&r2=1.13
due to this e-mail:
http://twomix.devolution.com/pipermail/sdl/2005-January/066828.html

that error is the exact error we were seeing in Gentoo with SDL_endian.h
rev1.11 and which rev1.12 fixed … so perhaps the report was based upon
older versions of SDL_endian.h and not the newer fixed one ?

No.

I don’t have an amd64, so I can only do a shot in the dark.
Does this SDL_endian.h work for both of you ?

Stephane

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed…
Name: SDL_endian.h
URL: http://lists.libsdl.org/pipermail/sdl-libsdl.org/attachments/20050122/fd43b0ac/attachment.txt

Kirill Ponomarew wrote:>On Sat, Jan 22, 2005 at 05:06:12PM +0100, Stephane Marchesin wrote:

however, it has since been reverted:
http://www.libsdl.org/cgi/cvsweb.cgi/SDL12/include/SDL_endian.h.diff?r1=1.12&r2=1.13
due to this e-mail:
http://twomix.devolution.com/pipermail/sdl/2005-January/066828.html

that error is the exact error we were seeing in Gentoo with SDL_endian.h
rev1.11 and which rev1.12 fixed … so perhaps the report was based upon
older versions of SDL_endian.h and not the newer fixed one ?

No.

I don’t have an amd64, so I can only do a shot in the dark.
Does this SDL_endian.h work for both of you ?

This patch works just fine on FreeBSD-5.x/6.x amd64, and was
committed some time ago.

So you didn’t actually reverse the CVS patch but applied it !

Ok, so the patch should go back to cvs :slight_smile:

Stephane

i assume we’re still just talking about SDL_Swap16() since i didnt download
the header and run a diff, so i ran a bunch of quick tests and found:
"=q" (x) fails
"=r" (x) fails
"=Q" (x) works
-mikeOn Saturday 22 January 2005 11:06 am, Stephane Marchesin wrote:

I don’t have an amd64, so I can only do a shot in the dark.
Does this SDL_endian.h work for both of you ?

the netbsd fix imported here:
http://www.libsdl.org/cgi/cvsweb.cgi/SDL12/include/SDL_endian.h.diff?r1=1.11&r2=1.12
made our problems go away nicely (i was able to reproduce original build
failures and confirm the fix on an amd64 dev box)

Doesn’t a recent gcc know to use that instruction anyway?
I think you can just get rid of the assembly code. The code
might even run faster, since gcc would have more information
about the code.

Since the problem only hits gcc-3.4, and older compilers
might benefit from the assembly, try this:

#if GNUC < 3 || ( GNUC == 3 && GNUC_MINOR < 4 )
// assembly code goes here
#else
// plain C code
#endifOn Sat, 2005-01-22 at 00:40 -0500, Mike Frysinger wrote:

Albert Cahalan wrote:>On Sat, 2005-01-22 at 00:40 -0500, Mike Frysinger wrote:

the netbsd fix imported here:
http://www.libsdl.org/cgi/cvsweb.cgi/SDL12/include/SDL_endian.h.diff?r1=1.11&r2=1.12
made our problems go away nicely (i was able to reproduce original build
failures and confirm the fix on an amd64 dev box)

Doesn’t a recent gcc know to use that instruction anyway?
I think you can just get rid of the assembly code. The code
might even run faster, since gcc would have more information
about the code.

It might be true that removing it might be faster, but the reason is not
that gcc has more or less “information about the code”.

The reason is that xchg is an implicitly locking instruction when
working with memory operands, which could be costly on smp.
That’s why I suggested a fix putting “r” instead of “q”, btw : to force
the use of a register at all times and thus avoid the memory operand.

Stephane

Albert Cahalan wrote:

the netbsd fix imported here:
http://www.libsdl.org/cgi/cvsweb.cgi/SDL12/include/SDL_endian.h.diff?r1=1.11&r2=1.12
made our problems go away nicely (i was able to reproduce original build
failures and confirm the fix on an amd64 dev box)

Doesn’t a recent gcc know to use that instruction anyway?
I think you can just get rid of the assembly code. The code
might even run faster, since gcc would have more information
about the code.

It might be true that removing it might be faster, but the reason is not
that gcc has more or less “information about the code”.

Perhaps the code merely tests a bit after swapping.
With the assembly, you’ll have that xchg instruction.
With plain C code, gcc can just test a different bit
instead of swapping the bytes.

Perhaps the code will save the least-significant byte
of a short into a char. With plain C code, gcc can
simply shift as needed and store the result.

Here’s what Linux does for i386:

swab16 is plain C (with note that gcc handles it)
swab32 is bswap, or for obsolete junk: xchgb,rorl,xchgb
swab64 is bswapl, or for obsolete junk: swab32(),swab32(),xchgl

Here’s what Linux does for x86-64:

swab16 is plain C (with note that gcc handles it)
swab32 is bswapl
swab64 is bswapq

I expect that the above will change as older compilers
become less commonly used.

PowerPC would benefit from byteswap operations that take
a pointer. The CPU has load/store instructions that swap
the bytes, but no register-to-register ones that do.
For serious byte swapping, AltiVec would be good to use.On Tue, 2005-01-25 at 17:35 +0100, Stephane Marchesin wrote:

On Sat, 2005-01-22 at 00:40 -0500, Mike Frysinger wrote:

that error is the exact error we were seeing in Gentoo with SDL_endian.h
rev1.11 and which rev1.12 fixed … so perhaps the report was based upon
older versions of SDL_endian.h and not the newer fixed one ?

That sounds like what happened. Okay, ‘q’ is being changed back to ‘Q’

Thanks!
-Sam Lantinga, Software Engineer, Blizzard Entertainment