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

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

I think panicSlice is in the same situation as panicIndex.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

commented Apr 3, 2019

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

@randall77

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

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

@gopherbot gopherbot closed this in 48ef010 Apr 3, 2019
@odeke-em

This comment has been minimized.

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.