New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmd/compile: miscompilation of some arithmetic and conditionals on arm #42876
Comments
This seems to have come from this gcc test: /* The FR-V port used to fail this test because the andcc patterns
wrongly claimed to set the C and V flags. */
#include <limits.h>
int x[4] = { INT_MIN / 2, INT_MAX, 2, 4 };
int
main ()
{
if (x[0] < x[1])
if ((x[2] & x[3]) < 0)
abort ();
exit (0);
} |
Here's a simpler example: package main
var x = [4]int32{-0x7fffffff, 0x7fffffff, 2, 4}
func main() {
if x[0] > x[1] {
panic("fail 1")
}
if x[2]&x[3] < 0 {
panic("fail 2") // Fails here
}
} It looks like the basic issue is that the first comparison uses |
I've never looked at this compiler before, so excuse the noise if this is way off, but: src/cmd/compile/internal/ssa/gen/ARM.rules:
|
I've contacted the origin auther @benshi001 about this. |
So, I "fixed" it: diff --git a/src/cmd/compile/internal/ssa/gen/ARM.rules b/src/cmd/compile/internal/ssa/gen/ARM.rules
index 946acd4ccc..61a410dad3 100644
--- a/src/cmd/compile/internal/ssa/gen/ARM.rules
+++ b/src/cmd/compile/internal/ssa/gen/ARM.rules
@@ -1369,7 +1369,6 @@
(LE (CMPconst [0] l:(ADDshiftLLreg x y z)) yes no) && l.Uses==1 => (LEnoov (CMNshiftLLreg x y z) yes no)
(LE (CMPconst [0] l:(ADDshiftRLreg x y z)) yes no) && l.Uses==1 => (LEnoov (CMNshiftRLreg x y z) yes no)
(LE (CMPconst [0] l:(ADDshiftRAreg x y z)) yes no) && l.Uses==1 => (LEnoov (CMNshiftRAreg x y z) yes no)
-(LT (CMPconst [0] l:(AND x y)) yes no) && l.Uses==1 => (LT (TST x y) yes no)
(LT (CMPconst [0] l:(ANDconst [c] x)) yes no) && l.Uses==1 => (LT (TSTconst [c] x) yes no)
(LT (CMPconst [0] l:(ANDshiftLL x y [c])) yes no) && l.Uses==1 => (LT (TSTshiftLL x y [c]) yes no)
(LT (CMPconst [0] l:(ANDshiftRL x y [c])) yes no) && l.Uses==1 => (LT (TSTshiftRL x y [c]) yes no)
diff --git a/src/cmd/compile/internal/ssa/rewriteARM.go b/src/cmd/compile/internal/ssa/rewriteARM.go
index 47fd0a94cc..bca648e9ad 100644
--- a/src/cmd/compile/internal/ssa/rewriteARM.go
+++ b/src/cmd/compile/internal/ssa/rewriteARM.go
@@ -20226,34 +20226,6 @@ func rewriteBlockARM(b *Block) bool {
b.resetWithControl(BlockARMLTnoov, v0)
return true
}
- // match: (LT (CMPconst [0] l:(AND x y)) yes no)
- // cond: l.Uses==1
- // result: (LT (TST x y) yes no)
- for b.Controls[0].Op == OpARMCMPconst {
- v_0 := b.Controls[0]
- if auxIntToInt32(v_0.AuxInt) != 0 {
- break
- }
- l := v_0.Args[0]
- if l.Op != OpARMAND {
- break
- }
- _ = l.Args[1]
- l_0 := l.Args[0]
- l_1 := l.Args[1]
- for _i0 := 0; _i0 <= 1; _i0, l_0, l_1 = _i0+1, l_1, l_0 {
- x := l_0
- y := l_1
- if !(l.Uses == 1) {
- continue
- }
- v0 := b.NewValue0(v_0.Pos, OpARMTST, types.TypeFlags)
- v0.AddArg2(x, y)
- b.resetWithControl(BlockARMLT, v0)
- return true
- }
- break
- }
// match: (LT (CMPconst [0] l:(ANDconst [c] x)) yes no)
// cond: l.Uses==1
// result: (LT (TSTconst [c] x) yes no)
This is obviously an incomplete fix, but it does generate code that actually works (for that one case): Before my fix:
After my fix:
Basically, the bug is that the original replacement rules assumed that So, the cudgel fix might be just deleting all those errant replacement rules. |
Change https://golang.org/cl/274026 mentions this issue: |
I think I've got something working and posted on Gerrit already, but I'd appreciate your review and comments you might have. Feel free to jump in: https://golang.org/cl/274026 |
@gopherbot please backport this because compiler bugs in bitmath seem worrisome |
Backport issue(s) opened: #42929 (for 1.14), #42930 (for 1.15). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
Sorry for the delay, I was working on other project. you've submit the CL, which LGTM. |
@zx2c4 , would you mind preparing backport CLs for 1.15 and 1.14? |
I can backport it. But... I had started doing that before because I initially thought it was a very worrying bug. Indeed it smells Very Very Bad. What if crypto code hits this! What about security code! What if!! So I added a line to the compiler to panic if it ever got to generating the bad code, and then scripted walking through github and building all of the go code I could find, sorted by repo popularity. I let this run for a few hours and... nothing triggered it. Maybe it's not as big of a deal as previously thought, then? |
Change https://golang.org/cl/282432 mentions this issue: |
@zx2c4, would you mind preparing a backport CLs for 1.14? |
Change https://golang.org/cl/289009 mentions this issue: |
…on arm These replacement rules assume that TST and TEQ set V. But TST and TEQ do not set V. This is a problem because instructions like LT are actually checking for N!=V. But with TST and TEQ not setting V, LT doesn't do anything meaningful. It's possible to construct trivial miscompilations from this, such as: package main var x = [4]int32{-0x7fffffff, 0x7fffffff, 2, 4} func main() { if x[0] > x[1] { panic("fail 1") } if x[2]&x[3] < 0 { panic("fail 2") // Fails here } } That first comparison sets V, via the CMP that subtracts the values causing the overflow. Then the second comparison operation thinks that it uses the result of TST, when it actually uses the V from CMP. Before this fix: TST R0, R1 BLT loc_6C164 After this fix: TST R0, R1 BMI loc_6C164 The BMI instruction checks the N flag, which TST sets. This commit fixes the issue by using [LG][TE]noov instead of vanilla [LG][TE], and also adds a test case for the direct issue. Updates #42876. Fixes #42930. Change-Id: I13c62c88d18574247ad002b671b38d2d0b0fc6fa Reviewed-on: https://go-review.googlesource.com/c/go/+/282432 Trust: Jason A. Donenfeld <Jason@zx2c4.com> Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com>
@FiloSottile suggested that a recent mailing list post be moved to an issue, so here goes:
Jan Mercl reports that this code will miscompile on ARM:
Running the same code on 386 vs arm shows success on 386 and panics on arm. I've confirmed this is the case with 1.15.5, and Meng Zhuo confirmed it to be happening on 1.13, so it is at least a somewhat oldish bug.
The current issue title is vague because I haven't looked into what's actually happening yet. And I haven't yet assessed the security implications, either, but one can start imagining worrisome corner cases.
CC @rsc @cherrymui @ianlancetaylor @aclements
The text was updated successfully, but these errors were encountered: