| Summary: | _allmul and _allshr in SDL_stdlib.c is not working properly | ||
|---|---|---|---|
| Product: | SDL | Reporter: | eastcowboy2002 |
| Component: | *don't know* | Assignee: | Ryan C. Gordon <icculus> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | critical | ||
| Priority: | P1 | CC: | MarkPizzolato-libSDLbugs, salos |
| Version: | 2.0.4 | ||
| Hardware: | x86 | ||
| OS: | Windows (All) | ||
This bug exists Fixed, thanks! https://hg.libsdl.org/SDL/rev/45f11ff360a8 Related to the _allmul problem is an equivalent problem in the _allshr routine.
long long X = 0xAAAAAAAA55555555ll;
long long Y;
Y = X >> 63;
The current code produces a result of 0x00000000FFFFFFFF
The correct result is 0xFFFFFFFFFFFFFFFF
The following implementation of _allshr in SDL_stdlib.c works correctly:
void
__declspec(naked)
_allshr()
{
/* *INDENT-OFF* */
__asm {
cmp cl,3Fh
jae RETSIGN
cmp cl,20h
jae MORE32
shrd eax,edx,cl
sar edx,cl
ret
MORE32:
mov eax,edx
and cl,1Fh
sar eax,cl
ret
RETSIGN:
sar edx,1Fh
mov eax,edx
ret
}
/* *INDENT-ON* */
}
The fact that we've got 2 errors like this in this module doesn't inspire confidence. :-( I looked closely at the other shift routines and they seem to be good. I didn't dig through the other assembler code though....
ARGHH!! I missed one case above: (right shift > 32) needs to propagate the sign in the high longword.
void
__declspec(naked)
_allshr()
{
/* *INDENT-OFF* */
__asm {
cmp cl,3Fh
jae RETSIGN
cmp cl,20h
jae MORE32
shrd eax,edx,cl
sar edx,cl
ret
MORE32:
mov eax,edx
sar edx,1Fh
and cl,1Fh
sar eax,cl
ret
RETSIGN:
sar edx,1Fh
mov eax,edx
ret
}
/* *INDENT-ON* */
}
Part of this problem persists now in the released version of libSDL 2.0.5. The _allmul() issue has indeed been fixed (and was in the source code available back in May when I reported the related problem with _allshr()). The _allshr() problem is still there. This code is only referenced when statically building a SDL application, so the DLL code is unaffected. Should I create a new bug report for this, or will the existing information provided be sufficient? |
Look at this simple case: Sint64 a = 0xFFFFFFFF; Sint64 b = 0xFFFFFFFF; a *= b; assert(a == 0xFFFFFFFE00000001); // assert failed. // value of a is: 0xFFFFFFFF00000001 Test Environment: Windows 7 Visual C++ 2013 simply add SDL 2.0.3 source files to my project for this test In SDL_lib.c there is a function "_allmul" that replace the default _allmul function provided by Visual C++ 2013. The two versions look quite different. Here is the Visual C++ 2013's implemention of _allmul void __declspec(naked) _allmul_VS2013() { /* *INDENT-OFF* */ __asm { mov eax,dword ptr [esp+8] mov ecx,dword ptr [esp+10h] or ecx,eax mov ecx,dword ptr [esp+0Ch] jne hard mov eax,dword ptr [esp+4] mul ecx ret 10h hard: push ebx mul ecx mov ebx,eax mov eax,dword ptr [esp+8] mul dword ptr [esp+14h] add ebx,eax mov eax,dword ptr [esp+8] mul ecx add edx,ebx pop ebx ret 10h } /* *INDENT-ON* */ } I am not good at assemble language but I think it should be fixed by some way. I'm using SDL and SDL_image, but when I loading some webp pictures, I see that most of pixels are green. I spent more than a hole day to find out this (first check libwebp, then SDL_image, and finally SDL itself. hope other people will have better luck than me). Just simply remove this _allmul function, or replace its content by the code shown above, both methods will be OK. P.S. directly use SDL2.lib will be OK, too. I looked the assemble and find that (my project) is not using SDL's _allmul function.