Skip to content
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: bounds check not removed after "if index < length" #15074

Closed
rasky opened this issue Apr 2, 2016 · 6 comments
Closed

cmd/compile: bounds check not removed after "if index < length" #15074

rasky opened this issue Apr 2, 2016 · 6 comments

Comments

@rasky
Copy link
Member

rasky commented Apr 2, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    go version devel +6a0bb87 Sat Apr 2 02:08:59 2016 +0000 darwin/amd64
  2. What operating system and processor architecture are you using (go env)?
    GOARCH="amd64"
    GOBIN=""
    GOEXE=""
    GOHOSTARCH="amd64"
    GOHOSTOS="darwin"
    GOOS="darwin"
    GOPATH="/Users/rasky/Sources/go"
    GORACE=""
    GOROOT="/usr/local/Cellar/go/1.6/libexec"
    GOTOOLDIR="/usr/local/Cellar/go/1.6/libexec/pkg/tool/darwin_amd64"
    GO15VENDOREXPERIMENT="1"
    CC="clang"
    GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fno-common"
    CXX="clang++"
    CGO_ENABLED="1"
  3. What did you do?
package main

import (
    "os"
    "strconv"
)

var table = [8]uint32{
    1 << 30, 1 << 30,
    1 << 29, 1 << 29,
    1 << 31, 1 << 31,
    1 << 28, 1 << 28,
}

var value uint32

//go:noinline
func Bound1(cond uint32) bool {
    if cond < 8 {
        v := value
        if cond&1 != 0 {
            v = ^v
        }
        return v&table[cond] != 0   // bound check should be removed here
    }
    return false
}

func main() {
    val, _ := strconv.Atoi(os.Args[1])
    Bound1(uint32(val))
}
  1. What did you expect to see?
    Bound check removed
  2. What did you see instead?
TEXT main.Bound1(SB) /Users/rasky/Sources/go/src/ndsemu/bugs/bound1.go
    bound1.go:18    0x2040  65488b0c25a0080000  GS MOVQ GS:0x8a0, CX
    bound1.go:18    0x2049  483b6110        CMPQ 0x10(CX), SP
    bound1.go:18    0x204d  7642            JBE 0x2091
    bound1.go:19    0x204f  8b4c2408        MOVL 0x8(SP), CX
    bound1.go:19    0x2053  83f908          CMPL $0x8, CX
    bound1.go:19    0x2056  7333            JAE 0x208b
    bound1.go:15    0x2058  8b1542aa0b00        MOVL 0xbaa42(IP), DX
    bound1.go:21    0x205e  f7c101000000        TESTL $0x1, CX
    bound1.go:21    0x2064  7402            JE 0x2068
    bound1.go:22    0x2066  f7d2            NOTL DX
    bound1.go:24    0x2068  8bc9            MOVL CX, CX
    bound1.go:24    0x206a  4883f908        CMPQ $0x8, CX
    bound1.go:24    0x206e  7314            JAE 0x2084
    bound1.go:24    0x2070  488d0529e00900      LEAQ 0x9e029(IP), AX
    bound1.go:24    0x2077  8b0c88          MOVL 0(AX)(CX*4), CX
    bound1.go:24    0x207a  85d1            TESTL DX, CX
    bound1.go:24    0x207c  0f95c0          SETNE AL
    bound1.go:24    0x207f  88442410        MOVL AL, 0x10(SP)
    bound1.go:24    0x2083  c3          RET
    bound1.go:24    0x2084  e857e30100      CALL runtime.panicindex(SB)
    bound1.go:24    0x2089  0f0b            UD2
    bound1.go:26    0x208b  c644241000      MOVL $0x0, 0x10(SP)
    bound1.go:26    0x2090  c3          RET
    bound1.go:18    0x2091  e81a1b0400      CALL runtime.morestack_noctxt(SB)
    bound1.go:18    0x2096  eba8            JMP main.Bound1(SB)
    bound1.go:18    0x2098  cc          INT $0x3
    bound1.go:18    0x2099  cc          INT $0x3
    bound1.go:18    0x209a  cc          INT $0x3
    bound1.go:18    0x209b  cc          INT $0x3
    bound1.go:18    0x209c  cc          INT $0x3
    bound1.go:18    0x209d  cc          INT $0x3
    bound1.go:18    0x209e  cc          INT $0x3
    bound1.go:18    0x209f  cc          INT $0x3
@rasky
Copy link
Member Author

rasky commented Apr 2, 2016

/cc @brtzsnr -- i thought latest improvements should manage to remove this

@brtzsnr
Copy link
Contributor

brtzsnr commented Apr 2, 2016

It doesn't transcend the zeroextension (MOVL CX, CX). Where does it show up?

@rasky
Copy link
Member Author

rasky commented Apr 2, 2016

Sorry I don't understand the question. You mean which codebase is this from? I just spotted it while analyzing code generation to search for optimization opportunities related to bound checking.

@brtzsnr
Copy link
Contributor

brtzsnr commented Apr 2, 2016

Thanks. It's good to keep track of which cases we miss.

@josharian josharian changed the title Boundcheck not removed cmd/compile: bounds check not removed Apr 3, 2016
@bradfitz bradfitz added this to the Unplanned milestone Apr 7, 2016
@rasky rasky changed the title cmd/compile: bounds check not removed cmd/compile: bounds check not removed after "if index < length" Aug 22, 2017
@gopherbot
Copy link

Change https://golang.org/cl/174704 mentions this issue: cmd/compile: handle sign/zero extensions in prove, via update method

@zdjones
Copy link
Contributor

zdjones commented Jul 26, 2019

https://golang.org/cl/174704 makes prove transcend this zero extension, and fixes the issue. Should be going in 1.14.

tomocy pushed a commit to tomocy/go that referenced this issue Sep 1, 2019
Array accesses with index types smaller than the machine word size may
involve a sign or zero extension of the index value before bounds
checking. Currently, this defeats prove because the facts about the
original index value don't flow through the sign/zero extension.

This CL fixes this by looking back through value-preserving sign/zero
extensions when adding facts via Update and, where appropriate, applying
the same facts using the pre-extension value. This fix is enhanced by
also looking back through value-preserving extensions within
ft.isNonNegative to infer whether the extended value is known to be
non-negative. Without this additional isNonNegative enhancement, this
logic is rendered significantly less effective by the limitation
discussed in the next paragraph.

In Update, the application of facts to pre-extension values is limited
to cases where the domain of the new fact is consistent with the type of
the pre-extension value. There may be cases where this cross-domain
passing of facts is valid, but distinguishing them from the invalid
cases is difficult for me to reason about and to implement.
Assessing which cases to allow requires details about the context and
inferences behind the fact being applied which are not available
within Update. Additional difficulty arises from the fact that the SSA
does not curently differentiate extensions added by the compiler for
indexing operations, extensions added by the compiler for implicit
conversions, or explicit extensions from the source.

Examples of some cases that would need to be filtered correctly for
cross-domain facts:

(1) A uint8 is zero-extended to int for indexing (a value-preserving
zeroExt). When, if ever, can signed domain facts learned about the int be
applied to the uint8?

(2) An int8 is sign-extended to int16 (value-preserving) for an equality
comparison. Equality comparison facts are currently always learned in both
the signed and unsigned domains. When, if ever, can the unsigned facts
learned about the int16, from the int16 != int16 comparison, be applied
to the original int8?

This is an alternative to CL 122695 and CL 174309. Compared to CL 122695,
this CL differs in that the facts added about the pre-extension value will
pass through the Update method, where additional inferences are processed
(e.g. fence-post implications, see golang#29964). CL 174309 is limited to bounds
checks, so is narrower in application, and makes the code harder to read.

Fixes golang#26292.
Fixes golang#29964.
Fixes golang#15074

Removes 238 bounds checks from std/cmd.

Change-Id: I1f87c32ee672bfb8be397b27eab7a4c2f304893f
Reviewed-on: https://go-review.googlesource.com/c/go/+/174704
Run-TryBot: Zach Jones <zachj1@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Giovanni Bajo <rasky@develer.com>
t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
Array accesses with index types smaller than the machine word size may
involve a sign or zero extension of the index value before bounds
checking. Currently, this defeats prove because the facts about the
original index value don't flow through the sign/zero extension.

This CL fixes this by looking back through value-preserving sign/zero
extensions when adding facts via Update and, where appropriate, applying
the same facts using the pre-extension value. This fix is enhanced by
also looking back through value-preserving extensions within
ft.isNonNegative to infer whether the extended value is known to be
non-negative. Without this additional isNonNegative enhancement, this
logic is rendered significantly less effective by the limitation
discussed in the next paragraph.

In Update, the application of facts to pre-extension values is limited
to cases where the domain of the new fact is consistent with the type of
the pre-extension value. There may be cases where this cross-domain
passing of facts is valid, but distinguishing them from the invalid
cases is difficult for me to reason about and to implement.
Assessing which cases to allow requires details about the context and
inferences behind the fact being applied which are not available
within Update. Additional difficulty arises from the fact that the SSA
does not curently differentiate extensions added by the compiler for
indexing operations, extensions added by the compiler for implicit
conversions, or explicit extensions from the source.

Examples of some cases that would need to be filtered correctly for
cross-domain facts:

(1) A uint8 is zero-extended to int for indexing (a value-preserving
zeroExt). When, if ever, can signed domain facts learned about the int be
applied to the uint8?

(2) An int8 is sign-extended to int16 (value-preserving) for an equality
comparison. Equality comparison facts are currently always learned in both
the signed and unsigned domains. When, if ever, can the unsigned facts
learned about the int16, from the int16 != int16 comparison, be applied
to the original int8?

This is an alternative to CL 122695 and CL 174309. Compared to CL 122695,
this CL differs in that the facts added about the pre-extension value will
pass through the Update method, where additional inferences are processed
(e.g. fence-post implications, see golang#29964). CL 174309 is limited to bounds
checks, so is narrower in application, and makes the code harder to read.

Fixes golang#26292.
Fixes golang#29964.
Fixes golang#15074

Removes 238 bounds checks from std/cmd.

Change-Id: I1f87c32ee672bfb8be397b27eab7a4c2f304893f
Reviewed-on: https://go-review.googlesource.com/c/go/+/174704
Run-TryBot: Zach Jones <zachj1@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Giovanni Bajo <rasky@develer.com>
@golang golang locked and limited conversation to collaborators Aug 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants