math: Sincos is very wrong when called from PortAudio callback. #8623
Labels
Milestone
Comments
What do you mean by "working code". Are you saying that that callback does NOT exhibit the problem? Can you print t, sin instead of just sin? I have no idea why being in the callback would matter. Maybe src/math/sincos_amd64.s uses uninitialized registers, but that seems unlikely. Or maybe PortAudio runs on a thread where an interrupt handler has been installed and an interrupt arrives and smashes the floating point registers. Status changed to Accepted. |
Sorry, "working code" was confusing. The callback DOES exhibit the problem. Printing t, sin(t*pi/2) within the callback gives (incorrectly): 0.000, 0.00000 0.125, 0.19509 0.250, 0.38268 0.375, 0.55557 0.500, 0.70711 0.625, 0.55557 0.750, 0.38268 0.875, 0.19509 whereas the normal (correct) output is: 0.000, 0.00000 0.125, 0.19509 0.250, 0.38268 0.375, 0.55557 0.500, 0.70711 0.625, 0.83147 0.750, 0.92388 0.875, 0.98079 As you can see, the errant output is not garbage. It has a regularity, and appears to be correct half of the time. FWIW, here's t, badsin, sin for 0 ≤ t < 4: 0.000, 0.00000, 0.00000 0.125, 0.19509, 0.19509 0.250, 0.38268, 0.38268 0.375, 0.55557, 0.55557 0.500, 0.70711, 0.70711 0.625, 0.55557, 0.83147 0.750, 0.38268, 0.92388 0.875, 0.19509, 0.98079 1.000, 0.00000, 1.00000 1.125, 0.19509, 0.98079 1.250, 0.38268, 0.92388 1.375, 0.55557, 0.83147 1.500, 0.70711, 0.70711 1.625, 0.55557, 0.55557 1.750, 0.38268, 0.38268 1.875, 0.19509, 0.19509 2.000, 0.00000, 0.00000 2.125, -0.19509, -0.19509 2.250, -0.38268, -0.38268 2.375, -0.55557, -0.55557 2.500, -0.70711, -0.70711 2.625, -0.55557, -0.83147 2.750, -0.38268, -0.92388 2.875, -0.19509, -0.98079 3.000, -0.00000, -1.00000 3.125, -0.19509, -0.98079 3.250, -0.38268, -0.92388 3.375, -0.55557, -0.83147 3.500, -0.70711, -0.70711 3.625, -0.55557, -0.55557 3.750, -0.38268, -0.38268 3.875, -0.19509, -0.19509 |
CL https://golang.org/cl/145920043 mentions this issue. |
Ok, I'm stumped. Here's what I know: * The problem is in sincos_amd64.s. This does not reproduce using the Go Sincos implementation from sincos.go. * The problem arises only in quadrants 1 and 2 (pi/4 < x < 3pi/4). For those quadrants, the sin and cos values are flipped. The calculation appears to be otherwise correct. This implicates the CMPSD instruction at http://golang.org/src/pkg/math/sincos_amd64.s#L103 or its arguments, X0 and X3. And I don't see how either of them might be wrong. |
> Can anybody write a test case that doesn't involve a cgo callback? I tried a few different ways but failed. > I don't understand what the callback has to do with anything. I'm starting to wonder whether this may be fairly PortAudio-specific. From http://portaudio.com/docs/v19-doxydocs/writing_a_callback.html: "Before we begin, it's important to realize that the callback is a delicate place. This is because some systems perform the callback in a special thread, or interrupt handler, and it is rarely treated the same as the rest of your code." It's mostly about priority inversion and preventing audio glitches, but I'm not sure what else might be different... |
CL https://golang.org/cl/151750043 mentions this issue. |
|
This issue was closed by revision 4a8cb4a. Status changed to Fixed. |
wheatman
pushed a commit
to wheatman/go-akaros
that referenced
this issue
Jun 25, 2018
The extra-clever code in Sincos is trying to do
if v&2 == 0 {
mask = 0xffffffffffffffff
} else {
mask = 0
}
It does this by turning v&2 into a float64 X0 and then using
MOVSD $0.0, X3
CMPSD X0, X3, 0
That CMPSD is defined to behave like:
if X0 == X3 {
X3 = 0xffffffffffffffff
} else {
X3 = 0
}
which gives the desired mask in X3. The goal in using the
CMPSD was to avoid a conditional branch.
This code fails when called from a PortAudio callback.
In particular, the failure behavior is exactly as if the
CMPSD always chose the 'true' execution.
Notice that the comparison X0 == X3 is comparing as
floating point values the 64-bit pattern v&2 and the actual
floating point value zero. The only possible values for v&2
are 0x0000000000000000 (floating point zero)
and 0x0000000000000002 (floating point 1e-323, a denormal).
If they are both comparing equal to zero, I conclude that
in a PortAudio callback (whatever that means), the processor
is running in "denormals are zero" mode.
I confirmed this by placing the processor into that mode
and running the test case in the bug; it produces the
incorrect output reported in the bug.
In general, if a Go program changes the floating point math
modes to something other than what Go expects, the math
library is not going to work exactly as intended, so we might
be justified in not fixing this at all.
However, it seems reasonable that the client code might
have expected "denormals are zero" mode to only affect
actual processing of denormals. This code has produced
what is in effect a gratuitous denormal by being extra clever.
There is nothing about the computation being requested
that fundamentally requires a denormal.
It is also easy to do this computation in integer math instead:
mask = ((v&2)>>1)-1
Do that.
For the record, the other math tests that fail if you put the
processor in "denormals are zero" mode are the tests for
Frexp, Ilogb, Ldexp, Logb, Log2, and FloatMinMax, but all
fail processing denormal inputs. Sincos was the only function
for which that mode causes incorrect behavior on non-denormal inputs.
The existing tests check that the new assembly is correct.
There is no test for behavior in "denormals are zero" mode,
because I don't want to add assembly to change that.
Fixes golang#8623.
LGTM=josharian
R=golang-codereviews, josharian
CC=golang-codereviews, iant, r
https://golang.org/cl/151750043
wheatman
pushed a commit
to wheatman/go-akaros
that referenced
this issue
Jun 26, 2018
The extra-clever code in Sincos is trying to do
if v&2 == 0 {
mask = 0xffffffffffffffff
} else {
mask = 0
}
It does this by turning v&2 into a float64 X0 and then using
MOVSD $0.0, X3
CMPSD X0, X3, 0
That CMPSD is defined to behave like:
if X0 == X3 {
X3 = 0xffffffffffffffff
} else {
X3 = 0
}
which gives the desired mask in X3. The goal in using the
CMPSD was to avoid a conditional branch.
This code fails when called from a PortAudio callback.
In particular, the failure behavior is exactly as if the
CMPSD always chose the 'true' execution.
Notice that the comparison X0 == X3 is comparing as
floating point values the 64-bit pattern v&2 and the actual
floating point value zero. The only possible values for v&2
are 0x0000000000000000 (floating point zero)
and 0x0000000000000002 (floating point 1e-323, a denormal).
If they are both comparing equal to zero, I conclude that
in a PortAudio callback (whatever that means), the processor
is running in "denormals are zero" mode.
I confirmed this by placing the processor into that mode
and running the test case in the bug; it produces the
incorrect output reported in the bug.
In general, if a Go program changes the floating point math
modes to something other than what Go expects, the math
library is not going to work exactly as intended, so we might
be justified in not fixing this at all.
However, it seems reasonable that the client code might
have expected "denormals are zero" mode to only affect
actual processing of denormals. This code has produced
what is in effect a gratuitous denormal by being extra clever.
There is nothing about the computation being requested
that fundamentally requires a denormal.
It is also easy to do this computation in integer math instead:
mask = ((v&2)>>1)-1
Do that.
For the record, the other math tests that fail if you put the
processor in "denormals are zero" mode are the tests for
Frexp, Ilogb, Ldexp, Logb, Log2, and FloatMinMax, but all
fail processing denormal inputs. Sincos was the only function
for which that mode causes incorrect behavior on non-denormal inputs.
The existing tests check that the new assembly is correct.
There is no test for behavior in "denormals are zero" mode,
because I don't want to add assembly to change that.
Fixes golang#8623.
LGTM=josharian
R=golang-codereviews, josharian
CC=golang-codereviews, iant, r
https://golang.org/cl/151750043
wheatman
pushed a commit
to wheatman/go-akaros
that referenced
this issue
Jul 9, 2018
The extra-clever code in Sincos is trying to do
if v&2 == 0 {
mask = 0xffffffffffffffff
} else {
mask = 0
}
It does this by turning v&2 into a float64 X0 and then using
MOVSD $0.0, X3
CMPSD X0, X3, 0
That CMPSD is defined to behave like:
if X0 == X3 {
X3 = 0xffffffffffffffff
} else {
X3 = 0
}
which gives the desired mask in X3. The goal in using the
CMPSD was to avoid a conditional branch.
This code fails when called from a PortAudio callback.
In particular, the failure behavior is exactly as if the
CMPSD always chose the 'true' execution.
Notice that the comparison X0 == X3 is comparing as
floating point values the 64-bit pattern v&2 and the actual
floating point value zero. The only possible values for v&2
are 0x0000000000000000 (floating point zero)
and 0x0000000000000002 (floating point 1e-323, a denormal).
If they are both comparing equal to zero, I conclude that
in a PortAudio callback (whatever that means), the processor
is running in "denormals are zero" mode.
I confirmed this by placing the processor into that mode
and running the test case in the bug; it produces the
incorrect output reported in the bug.
In general, if a Go program changes the floating point math
modes to something other than what Go expects, the math
library is not going to work exactly as intended, so we might
be justified in not fixing this at all.
However, it seems reasonable that the client code might
have expected "denormals are zero" mode to only affect
actual processing of denormals. This code has produced
what is in effect a gratuitous denormal by being extra clever.
There is nothing about the computation being requested
that fundamentally requires a denormal.
It is also easy to do this computation in integer math instead:
mask = ((v&2)>>1)-1
Do that.
For the record, the other math tests that fail if you put the
processor in "denormals are zero" mode are the tests for
Frexp, Ilogb, Ldexp, Logb, Log2, and FloatMinMax, but all
fail processing denormal inputs. Sincos was the only function
for which that mode causes incorrect behavior on non-denormal inputs.
The existing tests check that the new assembly is correct.
There is no test for behavior in "denormals are zero" mode,
because I don't want to add assembly to change that.
Fixes golang#8623.
LGTM=josharian
R=golang-codereviews, josharian
CC=golang-codereviews, iant, r
https://golang.org/cl/151750043
wheatman
pushed a commit
to wheatman/go-akaros
that referenced
this issue
Jul 30, 2018
The extra-clever code in Sincos is trying to do
if v&2 == 0 {
mask = 0xffffffffffffffff
} else {
mask = 0
}
It does this by turning v&2 into a float64 X0 and then using
MOVSD $0.0, X3
CMPSD X0, X3, 0
That CMPSD is defined to behave like:
if X0 == X3 {
X3 = 0xffffffffffffffff
} else {
X3 = 0
}
which gives the desired mask in X3. The goal in using the
CMPSD was to avoid a conditional branch.
This code fails when called from a PortAudio callback.
In particular, the failure behavior is exactly as if the
CMPSD always chose the 'true' execution.
Notice that the comparison X0 == X3 is comparing as
floating point values the 64-bit pattern v&2 and the actual
floating point value zero. The only possible values for v&2
are 0x0000000000000000 (floating point zero)
and 0x0000000000000002 (floating point 1e-323, a denormal).
If they are both comparing equal to zero, I conclude that
in a PortAudio callback (whatever that means), the processor
is running in "denormals are zero" mode.
I confirmed this by placing the processor into that mode
and running the test case in the bug; it produces the
incorrect output reported in the bug.
In general, if a Go program changes the floating point math
modes to something other than what Go expects, the math
library is not going to work exactly as intended, so we might
be justified in not fixing this at all.
However, it seems reasonable that the client code might
have expected "denormals are zero" mode to only affect
actual processing of denormals. This code has produced
what is in effect a gratuitous denormal by being extra clever.
There is nothing about the computation being requested
that fundamentally requires a denormal.
It is also easy to do this computation in integer math instead:
mask = ((v&2)>>1)-1
Do that.
For the record, the other math tests that fail if you put the
processor in "denormals are zero" mode are the tests for
Frexp, Ilogb, Ldexp, Logb, Log2, and FloatMinMax, but all
fail processing denormal inputs. Sincos was the only function
for which that mode causes incorrect behavior on non-denormal inputs.
The existing tests check that the new assembly is correct.
There is no test for behavior in "denormals are zero" mode,
because I don't want to add assembly to change that.
Fixes golang#8623.
LGTM=josharian
R=golang-codereviews, josharian
CC=golang-codereviews, iant, r
https://golang.org/cl/151750043
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
go version devel +9985ecf9828f Wed Aug 20 12:58:01 2014 +0400 darwin/amd64 When called from a PortAudio callback, the loop for t := 0.0; t < 1; t += .125 { sin, _ := math.Sincos(t*math.Pi/2) fmt.Println(sin) } outputs 0 0.19509032201612828 0.3826834323650898 0.5555702330196023 0.7071067811865476 0.5555702330196023 0.38268343236508984 0.19509032201612833 whereas I would expect it to print the same thing as when it is not run from the callback: 0 0.19509032201612828 0.3826834323650898 0.5555702330196023 0.7071067811865476 0.8314696123025451 0.9238795325112867 0.9807852804032304 Working code (requires PortAudio): http://play.golang.org/p/XlIDtR6JIFThe text was updated successfully, but these errors were encountered: