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: "runtime: bad pointer in frame" in riscv64 with complier optimizations #51101

Closed
piggynl opened this issue Feb 9, 2022 · 12 comments
Labels
arch-riscv Issues solely affecting the riscv64 architecture. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@piggynl
Copy link

piggynl commented Feb 9, 2022

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

$ go version
go version go1.17.6 linux/riscv64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="riscv64"
GOBIN=""
GOCACHE="/home/piggy/.cache/go-build"
GOENV="/home/piggy/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="riscv64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/piggy/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/piggy/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_riscv64"
GOVCS=""
GOVERSION="go1.17.6"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2469834381=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Reproduce: (you should have Node.js and Yarn pre-installed)

$ uname -m
riscv64

$ # build esbuild
$ git clone https://github.com/evanw/esbuild.git
$ cd esbuild
$ CGO_ENABLED=0 go build ./cmd/esbuild -o esbuild
$ cd ..

$ # get input files for esbuild
$ git clone https://github.com/tridactyl/tridactyl.git
$ cd tridactyl
$ yarn upgrade esbuild@^0.14.21 # make yarn happy (esbuild added support for riscv64 in 0.14.21)
$ yarn install
$ ../esbuild/esbuild --bundle src/background.ts --outfile=/dev/null 2>&1 | tee buildlog.txt

What did you expect to see?

esbuild works as intended, like in amd64.

What did you see instead?

runtime: bad pointer in frame github.com/evanw/esbuild/internal/js_parser.(*parser).visitBinding at 0x3f9cc244b8: 0xc0
fatal error: invalid pointer found on stack

runtime stack:
runtime.throw({0x4c9114, 0x1e})
        runtime/panic.go:1198 +0x60 fp=0x3f9c185830 sp=0x3f9c185808 pc=0x4c6a8
runtime.adjustpointers(0x3f9cc24470, 0x3f9c185910, 0x3f9c185d10, {0x752c80, 0x77ae00})
        runtime/stack.go:617 +0x224 fp=0x3f9c185878 sp=0x3f9c185830 pc=0x69204
runtime.adjustframe(0x3f9c185c28, 0x3f9c185d10)
        runtime/stack.go:659 +0xf8 fp=0x3f9c185948 sp=0x3f9c185878 pc=0x69300
runtime.gentraceback(0xffffffffffffffff, 0xffffffffffffffff, 0x0, 0x3f9c99cea0, 0x0, 0x0, 0x7fffffff, 0x4e2f50, 0x3f9c185d10, 0x0)
        runtime/traceback.go:350 +0x7e4 fp=0x3f9c185c80 sp=0x3f9c185948 pc=0x76604
runtime.copystack(0x3f9c99cea0, 0x10000)
        runtime/stack.go:918 +0x230 fp=0x3f9c185e30 sp=0x3f9c185c80 pc=0x69a28
runtime.newstack()
        runtime/stack.go:1097 +0x4b8 fp=0x3f9c185fd0 sp=0x3f9c185e30 pc=0x69fc0
runtime.morestack()
        runtime/asm_riscv64.s:202 +0x70 fp=0x3f9c185fd8 sp=0x3f9c185fd0 pc=0x80a68

goroutine 128 [copystack]:
runtime.growslice(0x478360, {0x3f9c9d8210, 0x2, 0x2}, 0x3)
        runtime/slice.go:162 +0x10 fp=0x3f9cc243a0 sp=0x3f9cc243a0 pc=0x676a8
github.com/evanw/esbuild/internal/js_parser.(*parser).recordDeclaredSymbol(...)
        github.com/evanw/esbuild/internal/js_parser/js_parser.go:8448
github.com/evanw/esbuild/internal/js_parser.(*parser).visitBinding(0x3f9c9f8800, {{0x564e00, 0x3f9c90d160}, {0x559}}, {0x3f9cc0e6a0})
        github.com/evanw/esbuild/internal/js_parser/js_parser.go:8463 +0x148 fp=0x3f9cc245f8 sp=0x3f9cc243a0 pc=0x2c57a8
github.com/evanw/esbuild/internal/js_parser.(*parser).visitArgs(0x3f9c9f8800, {0x3f9c912ff0, 0x1, 0x1}, {{0x3f9c90fb48, 0x1, 0x1}, 0x0, 0x0})
        github.com/evanw/esbuild/internal/js_parser/js_parser.go:10527 +0x238 fp=0x3f9cc247a0 sp=0x3f9cc245f8 pc=0x2d9970
... some frames omitted ...
github.com/evanw/esbuild/internal/bundler.parseFile({{0x5725c8, 0x3f9c00e380}, {0x3f9c0b6930, 0x3f9c00a180, 0x3f9c00a198, 0x3f9c01e1e0, 0x3}, {0x56ca80, 0x3f9c06b500}, 0x3f9c050360, ...})
        github.com/evanw/esbuild/internal/bundler/bundler.go:185 +0xce0 fp=0x3f9cc2bbe8 sp=0x3f9cc29910 pc=0x38d188
runtime.goexit()
        runtime/asm_riscv64.s:507 +0x4 fp=0x3f9cc2bbe8 sp=0x3f9cc2bbe8 pc=0x82904
created by github.com/evanw/esbuild/internal/bundler.(*scanner).maybeParseFile
        github.com/evanw/esbuild/internal/bundler/bundler.go:1189 +0x838

... and stack traces of other goroutines

The full output: http://fars.ee/4nkQ


I've tested the build options below. It seems the bug is caused by the optimizer?

Build Options Result
(default) esbuild crashes
-gcflags=all="-N" esbuild works fine
-gcflags=all="-l" esbuild works fine
-gcflags=all="-N -l" esbuild works fine

Also, I'm working on a minimal reproduce.

@mengzhuo
Copy link
Contributor

The js_parser is using unsafe to do pointer operation.
It looks like an esbuild bug to me.

@evanw
Copy link

evanw commented Feb 10, 2022

Is this mostly deterministic? Does this always or almost always happen? If so, you could try just removing the unsafe code to see if it still happens. There is only a small amount of unsafe code in js_parser.go:

 // The name is temporarily stored in the ref until the scope traversal pass
 // happens, at which point a symbol will be generated and the ref will point
 // to the symbol instead.
 //
 // The scope traversal pass will reconstruct the name using one of two methods.
 // In the common case, the name is a slice of the file itself. In that case we
 // can just store the slice and not need to allocate any extra memory. In the
 // rare case, the name is an externally-allocated string. In that case we store
 // an index to the string and use that index during the scope traversal pass.
 func (p *parser) storeNameInRef(name string) js_ast.Ref {
-  c := (*reflect.StringHeader)(unsafe.Pointer(&p.source.Contents))
-  n := (*reflect.StringHeader)(unsafe.Pointer(&name))
-
-  // Is the data in "name" a subset of the data in "p.source.Contents"?
-  if n.Data >= c.Data && n.Data+uintptr(n.Len) < c.Data+uintptr(c.Len) {
-    // The name is a slice of the file contents, so we can just reference it by
-    // length and don't have to allocate anything. This is the common case.
-    //
-    // It's stored as a negative value so we'll crash if we try to use it. That
-    // way we'll catch cases where we've forgotten to call loadNameFromRef().
-    // The length is the negative part because we know it's non-zero.
-    return js_ast.Ref{SourceIndex: -uint32(n.Len), InnerIndex: uint32(n.Data - c.Data)}
-  } else {
     // The name is some memory allocated elsewhere. This is either an inline
     // string constant in the parser or an identifier with escape sequences
     // in the source code, which is very unusual. Stash it away for later.
     // This uses allocations but it should hopefully be very uncommon.
     ref := js_ast.Ref{SourceIndex: 0x80000000, InnerIndex: uint32(len(p.allocatedNames))}
     p.allocatedNames = append(p.allocatedNames, name)
     return ref
-  }
 }

The unsafe code is used to recover the index and length of a substring within the string. Reusing the memory with the unsafe code saves about 20mb of memory in esbuild's primary benchmark but it's not necessary for esbuild to function, and can be commented out. If the problem still happens with that commented out then it's not due to unsafe.

@evanw
Copy link

evanw commented Feb 10, 2022

Update: I just removed use of the unsafe package on esbuild's master branch. So you should be able to try it again without needing to apply the code modification I described above.

@piggynl
Copy link
Author

piggynl commented Feb 10, 2022

Hi @mengzhuo,

After removing the usage of unsafe package, esbuild still crashes with runtime: bad pointer in frame. This happens with the patch in this issue and with evanw/esbuild@4db57c1, on a SiFive Unmatched RISC-V development board and in qemu-riscv64.


Is this mostly deterministic? Does this always or almost always happen?

Yes. This always happens.


To whom wants to test but without a development board (cc @evanw),

This wiki page Setup Arch Linux RISC-V Development Environment can helps you setup an environment with QEMU. After the setup, run pacman -S git go nodejs yarn (and gdb?), then it will be ready to reproduce.

@mengzhuo
Copy link
Contributor

mengzhuo commented Feb 10, 2022

Updated: I got the same failure that just run js_parser test.


Could you provide a mininal test (without yarn/nodejs parts) ? I can't install yarn on my unmatched box.

@mengzhuo
Copy link
Contributor

Kindly cc @mknyszek @thanm @cherrymui

@cherrymui cherrymui added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. arch-riscv Issues solely affecting the riscv64 architecture. labels Feb 10, 2022
@cherrymui cherrymui added this to the Backlog milestone Feb 10, 2022
@cherrymui
Copy link
Member

The bad pointer is from a subtraction of two (valid) pointer values, from

(EqPtr x y) => (SEQZ (SUB <x.Type> x y))

The temporary value (SUB <x.Type> x y) should not be a pointer type, as it is the difference of two pointers and may not point to valid memory. Will send a CL.

@gopherbot
Copy link

Change https://go.dev/cl/385655 mentions this issue: cmd/compile: correct type of pointer difference on RISCV64

@cherrymui
Copy link
Member

@gopherbot please backport this to previous releases. This can cause runtime crashes.

@gopherbot
Copy link

Backport issue(s) opened: #51198 (for 1.16), #51199 (for 1.17).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 14, 2022
@dmitshur dmitshur modified the milestones: Backlog, Go1.18 Feb 14, 2022
@gopherbot
Copy link

Change https://go.dev/cl/386474 mentions this issue: [release-branch.go1.17] cmd/compile: correct type of pointer difference on RISCV64

@gopherbot
Copy link

Change https://go.dev/cl/386475 mentions this issue: [release-branch.go1.16] cmd/compile: correct type of pointer difference on RISCV64

gopherbot pushed a commit that referenced this issue Feb 18, 2022
…ce on RISCV64

Pointer comparison is lowered to the following on RISCV64

(EqPtr x y) => (SEQZ (SUB <x.Type> x y))

The difference of two pointers (the SUB) should not be pointer
type. Otherwise it can cause the GC to find a bad pointer.

Updates #51101.
Fixes #51198.

Change-Id: I7e73c2155c36ff403c032981a9aa9cccbfdf0f64
Reviewed-on: https://go-review.googlesource.com/c/go/+/385655
Trust: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit 1ed30ca)
Reviewed-on: https://go-review.googlesource.com/c/go/+/386475
gopherbot pushed a commit that referenced this issue Feb 18, 2022
…ce on RISCV64

Pointer comparison is lowered to the following on RISCV64

(EqPtr x y) => (SEQZ (SUB <x.Type> x y))

The difference of two pointers (the SUB) should not be pointer
type. Otherwise it can cause the GC to find a bad pointer.

Updates #51101.
Fixes #51199.

Change-Id: I7e73c2155c36ff403c032981a9aa9cccbfdf0f64
Reviewed-on: https://go-review.googlesource.com/c/go/+/385655
Trust: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit 1ed30ca)
Reviewed-on: https://go-review.googlesource.com/c/go/+/386474
This was referenced Apr 24, 2022
@golang golang locked and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-riscv Issues solely affecting the riscv64 architecture. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants