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/internal/obj/x86, cmd/compile/internal/ssa: Possible lack of handling for panicIndex #31219

Closed
nsajko opened this issue Apr 3, 2019 · 6 comments

Comments

@nsajko
Copy link
Contributor

nsajko commented Apr 3, 2019

What version of Go are you using (go version)?

$ go version
go version devel +56517216c0 Tue Apr 2 05:45:33 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

Not applicable, I think.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/tmp/freedesktopCache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/nsajko"
GOPROXY=""
GORACE=""
GOROOT="/home/nsajko/goNew"
GOTMPDIR=""
GOTOOLDIR="/home/nsajko/goNew/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/nsajko/goNew/src/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build609693791=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Notice that panicindex was renamed to panicIndex since the release. Then I found that references to the old panicindex still exist in the tip.

What did you expect to see?

I am guessing either panicIndex should be handled together with panicindex in the below files, or instead of panicindex. Sorry if this is not a bug, I am just guessing.

What did you see instead?

src/cmd/internal/obj/x86/obj6.go: case "runtime.panicindex", "runtime.panicslice", "runtime.panicdivide", "runtime.panicwrap", "runtime.panicshift":
src/cmd/compile/internal/ssa/rewrite.go: case "runtime.racefuncenter", "runtime.racefuncexit", "runtime.panicindex",
src/cmd/compile/internal/ssa/rewrite.go: "runtime.panicslice", "runtime.panicdivide", "runtime.panicwrap",

@nsajko
Copy link
Contributor Author

nsajko commented Apr 3, 2019

I think panicSlice is in the same situation as panicIndex.

@randall77
Copy link
Contributor

Thanks. I will fix these. Both are optimizations that may not trigger because of the mismatch.

@randall77 randall77 self-assigned this Apr 3, 2019
@odeke-em
Copy link
Member

odeke-em commented Apr 3, 2019

Thank you for this report @nsajko!

@randall77 thanks for taking this on! I've crafted for you a suggestion of a unit test that
we can use to ensure we never regress after this is corrected.
Please perhaps consider adding this to cmd/compile/internal/ssa/rewrite_test.go

type stringerPlaceHolder string

func (sph stringerPlaceHolder) String() string {
        return string(sph)
}

func TestNeedRaceCleanup_OpStaticCall(t *testing.T) {
        mustTrigger := []string{
                "runtime.racefuncenter", "runtime.racefuncexit",
                "runtime.panicIndex", "runtime.panicSlice", "runtime.panicdivide",
                "runtime.panicwrap", "runtime.panicshift",
        }
        t.Run("OpStaticCall", func(tt *testing.T) {
                testNeedRaceCleanup(tt, OpStaticCall, mustTrigger, true)
        })
        t.Run("OpClosureCall", func(tt *testing.T) {
                testNeedRaceCleanup(tt, OpClosureCall, mustTrigger, false)
        })
        t.Run("OpInterCall", func(tt *testing.T) {
                testNeedRaceCleanup(tt, OpInterCall, mustTrigger, false)
        })
}

func testNeedRaceCleanup(t *testing.T, op Op, names []string, want bool) {
        for _, symName := range names {
                v := &Value{
                        Block: &Block{
                                Func: &Func{
                                        Config: &Config{Race: true},
                                        Blocks: []*Block{
                                                {
                                                        Values: []*Value{
                                                                {
                                                                        Aux: stringerPlaceHolder(symName),
                                                                        Op:  op,
                                                                },
                                                        },
                                                },
                                        },
                                },
                        },
                }
                sym := stringerPlaceHolder("runtime.racefuncexit")
                got := needRaceCleanup(sym, v)
                if got != want {
                        t.Errorf("needRaceCleanup(%q)=%t want %t", symName, got, want)
                }
        }
}

@gopherbot
Copy link

Change https://golang.org/cl/170639 mentions this issue: cmd/compile: handle new panicindex/slice names in optimizations

@randall77
Copy link
Contributor

@odeke-em Thanks for the test, but I think I found a better way to test using our codegen framework.

@odeke-em
Copy link
Member

odeke-em commented Apr 3, 2019

@randall77 cool, thanks for showing me that! I might be using the codegen framework in the near future and also writing such tests.

@golang golang locked and limited conversation to collaborators Apr 2, 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

4 participants