-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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: provide error message for giant strings instead of crashing #16394
Comments
Is this a regression? |
@dr2chase If you're asking me: I don't know. |
Update: I changed each string from 20 escaped bytes to 1000 and now |
Okay. I'm back. It exited with:
|
Can you provide the script used to generate the file? |
Am I correct that this is
followed by a single gigantic string constant broken into the +-concatenation of over 3 million strings? Have you tried it as a single string constant? It's clearly machine-generated, and this is a little outside the norm even for machine-generated code. |
Workaround: emit it as a single line.
Code to rewrite into a single string constant:
|
It is machine generated so yes, I can make it a single line. It was just an experiment, though. I thought this would be of interest. I'll let you know if the single line doesn't work for some reason. |
This is not a regression. Go 1.5 and 1.6 fail in the same way. Go 1.1 through 1.4 (before the compiler was itself written in Go) simply fail with segmentation violations. |
It may be that this file is too large to compile, but the compiler should fail with a better error message. |
Related: #7755. See also the discussion in #7754. I don't see a reasonable approach here for getting good error messages other than setting an arbitrary max string size, the way we eventually declare other constants too large. The fix would need to be in several places—first encounter, the OADDSTR case in const.go, maybe others. |
I guess I think that even a crash saying "compiler is out of memory" would be better than a crash saying "goroutine stack exceeds 1000000000-byte limit". |
That error is a throw, not a panic, so we can't do an easy defer/recover. I suppose we could add recursion depth tracking to treecopy. But maybe the best plan for this particular problem is changing the AST for OADD to be like OADDSTR—instead of Left+Right, flatten and use List to store all the Nodes to be added. This is one of many parts of the AST that would benefit from flattening. cc @mdempsky who I believe has been thinking about the future of the AST. |
It's not an issue of string size, it is instead a problem with the size of the 3-million deep expression tree, of the form "a0"+"a1"+"a2"+...+"a3,000,000". If you convert that into the single string that it could be, it compiles in a few seconds. It's machine-generated code, I recommend fixing the machine-generator. It's also possible that judicious use of parentheses could get past this, along the lines of:
|
That kinda seems like a lazy excuse, shifting the problem to the user. |
I think @dr2chase's recommendation should be considered as a workaround for the problem for now, not a permanent solution. We should still fix this problem if we can. |
Flattening OADD as suggested would solve the deep tree. I'm happy to look at it during 1.8 if @mdempsky thinks it won't be wasted work. |
Reassociating OADD is okay if the type is not floating point or complex. |
Perhaps this issue is a duplicate of older issue #1700? |
The compiler-internal syntax package can parse table.go (in cmd/compile/internal/syntax: go test -run Parse -src=$HOME/tmp/table.go). The (first) problem in the compiler is that the conversion to nodes requires recursion which leads to stack overflow. It may be possible to avoid that recursion, but that may still cause problems down the road due to sheer size. Recognizing string concatenation in the parser (and doing the concatenation there) may be possible but is a bit tricky. Bumping forward to 1.10. This is an esoteric corner case. |
The compiler has trouble compiling the concatenation of many strings, s0 + s1 + s2 + ... + sN, for large N. We insert redundant, explicit parentheses to work around that, lowering the N at any given step. This bug is golang/go#16394 and the fix is taken from golang/text@5c6cf4f9a2. Fixes #8
Change https://golang.org/cl/73880 mentions this issue: |
Change https://golang.org/cl/76450 mentions this issue: |
This should be reasonably fixed now. During noding, we now specially recognize expressions with concatenated string literals like
and efficiently collapse them down into
|
@mdempsky is that done for constants in general when noding, or just as a special case for strings to avoid the internal compiler crash later? I would assume that constant folding happens at a later stage. |
@mvdan During noding, we only special case string literal concatenation. General constant folding (including string concatenation involving named string constants or parenthesized subexpressions) is still done during typechecking. |
Please answer these questions before submitting your issue. Thanks!
go version
)?go version devel +fca9fc5 Thu Jun 16 19:45:33 2016 +0000 darwin/amd64
go env
)?$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/bmizerany"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/dx/jcs03lw901vc05cjjqyjf8y00000gn/T/go-build243723307=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
I generated this file (165mb) and ran
go build
:https://drive.google.com/open?id=0BwlBkts6X4NueEgzUVZiLUJwYms
A successful build.
This
The text was updated successfully, but these errors were encountered: