| Summary: | SDL_sinf throwing exception when DLL compiled with /O2 | ||
|---|---|---|---|
| Product: | SDL | Reporter: | Anthony @ POW Games <ant> |
| Component: | *don't know* | Assignee: | Ryan C. Gordon <icculus> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | normal | ||
| Priority: | P2 | CC: | sezeroz |
| Version: | HG 2.0 | ||
| Hardware: | x86 | ||
| OS: | Windows 10 | ||
|
Description
Anthony @ POW Games
2020-10-07 21:42:55 UTC
Can be an optimizer bug. Or, by only brief eyeballing the code, can jz ever be negative at line 193: http://hg.libsdl.org/SDL/file/3093f8936e5b/src/libm/k_rem_pio2.c#l193 (I did not ran the thing under a debugger or something, just a thought...) Should be fixed by https://hg.libsdl.org/SDL/rev/9a555589af66 If not, please drop a note here with details. Also, addition of those SDL_memset()s only for the sake of silencing static analyzers seems to be very excessive. sin() / cos() are supposed to be fast. Ozkhan, well spotted, that fixed it, thank you. Optimisation must have aggravated this bug - it's been plaguing me for years. I use SDL_sin and SDL_cos a lot, so if they can be made faster it would be good. Can something be done about casting SDL_sinf / cosf to doubles? Since the new drawing functions can now take floats (to avoid casting from ints), does it make sense that the math functions natively work on floats? (In reply to Anthony @ POW Games from comment #4) > Can something be done about casting SDL_sinf / cosf to doubles? Since the > new drawing functions can now take floats (to avoid casting from ints), does > it make sense that the math functions natively work on floats? I should be doable, but would require some work + testing. Not sure really how much worth that is. > Also, addition of those SDL_memset()s only for the sake
> of silencing static analyzers seems to be very excessive.
> sin() / cos() are supposed to be fast.
Okay, agreed, casting to double is no big problem for math functions - not worth duplicating it all for a float. But looking at the excessive memsets would help. I won't pretend to understand how __kernel_rem_pio2 works though :-)
Thanks again Ozkhan.
Those memset()s came with https://hg.libsdl.org/SDL/rev/cb546477e34f and https://hg.libsdl.org/SDL/rev/eb2820ee52ee -- authored by Ryan. Ryan (and Sam): is the following OK? diff --git a/src/libm/k_rem_pio2.c b/src/libm/k_rem_pio2.c --- a/src/libm/k_rem_pio2.c +++ b/src/libm/k_rem_pio2.c @@ -172,9 +172,11 @@ int32_t attribute_hidden __kernel_rem_pi /* set up f[0] to f[jx+jk] where f[jx+jk] = ipio2[jv+jk] */ j = jv-jx; m = jx+jk; for(i=0;i<=m;i++,j++) f[i] = (j<0)? zero : (double) ipio2[j]; +#if 0 /* only to make static analyzers happy.. */ if ((m+1) < SDL_arraysize(f)) { SDL_memset(&f[m+1], 0, sizeof (f) - ((m+1) * sizeof (f[0]))); } +#endif /* compute q[0],q[1],...q[jk] */ for (i=0;i<=jk;i++) { @@ -190,9 +192,11 @@ recompute: iq[i] = (int32_t)(z-two24*fw); z = q[j-1]+fw; } +#if 0 /* only to make static analyzers happy.. */ if (jz < SDL_arraysize(iq)) { SDL_memset(&iq[jz], 0, sizeof (iq) - (jz * sizeof (iq[0]))); } +#endif /* compute n */ z = scalbn(z,q0); /* actual value of z */ @@ -275,9 +279,11 @@ recompute: for(fw=0.0,k=0;k<=jp&&k<=jz-i;k++) fw += PIo2[k]*q[i+k]; fq[jz-i] = fw; } +#if 0 /* only to make static analyzers happy.. */ if ((jz+1) < SDL_arraysize(f)) { SDL_memset(&fq[jz+1], 0, sizeof (fq) - ((jz+1) * sizeof (fq[0]))); } +#endif /* compress fq[] into y[] */ switch(prec) { Ryan, don't we have an ifdef for static analysis? (In reply to Sam Lantinga from comment #8) > Ryan, don't we have an ifdef for static analysis? There is __clang_analyzer__ for, well, clang. Don't know of any others, or any SDL-specific macro. Or, we can remove those memset()s altogether. Ryan? |