We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 5309 - SDL_sinf throwing exception when DLL compiled with /O2
Summary: SDL_sinf throwing exception when DLL compiled with /O2
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: *don't know* (show other bugs)
Version: HG 2.0
Hardware: x86 Windows 10
: P2 normal
Assignee: Ryan C. Gordon
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-10-07 21:42 UTC by Anthony @ POW Games
Modified: 2020-10-09 01:39 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Anthony @ POW Games 2020-10-07 21:42:55 UTC
If I compile SDL2.dll with VS optimised for speed (the default set by SDL's VS project) and then call:

   SDL_sinf(-1656174.88f);

__kernel_rem_pio2 crashes when returning (return n&7;): Access violation writing location 0x00000000.

This is an age-old bug, but I've only just pinned it down. I can't pretend to understand how the optimisation is breaking this function, but optimisation shouldn't break it. It's a complex function with inline assember? Maybe it can be excluded from optimising?
Comment 1 Ozkan Sezer 2020-10-08 02:00:05 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...)
Comment 2 Ozkan Sezer 2020-10-08 08:52:38 UTC
Should be fixed by https://hg.libsdl.org/SDL/rev/9a555589af66
If not, please drop a note here with details.
Comment 3 Ozkan Sezer 2020-10-08 11:51:56 UTC
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.
Comment 4 Anthony @ POW Games 2020-10-08 14:37:47 UTC
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?
Comment 5 Ozkan Sezer 2020-10-08 15:32:14 UTC
(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.
Comment 6 Anthony @ POW Games 2020-10-08 15:54:41 UTC
> 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.
Comment 7 Ozkan Sezer 2020-10-08 16:21:15 UTC
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) {
Comment 8 Sam Lantinga 2020-10-08 21:42:02 UTC
Ryan, don't we have an ifdef for static analysis?
Comment 9 Ozkan Sezer 2020-10-09 01:39:10 UTC
(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?