cmd/compile: enable typechecking tracing causes side effects #33658
Comments
andybons
added
the
NeedsInvestigation
|
Sounds like |
|
Change https://golang.org/cl/190537 mentions this issue: |
|
To recap some of the discussion on CL 190537 so far, the root cause issue here is that tracing calls fmt.Printf on types; and when printing interface types, we call Type.Fields(), which calls Dowidth, which tries to flatten an interface, if it hasn't already. In the context of tracing the typechecking of this causes a problem because:
I suggested @cuonglm make a change that would avoid expanding the iface in fmt.go if it's not already expanded, but this led to problems with swt.go and typehash. To fix this, I think we need better discipline about making sure Dowidth is only called whenever it's actually safe to do so. |
|
|
@mdempsky Ah, never mind, it's not complicated as I thought, updated the CL. |
|
@cuonglm By better discipline, I mean structuring the code to avoid calling functions when it's unsafe (or potentially unsafe) to do so. Generally that means reducing the number of flags and special cases. |
|
@mdempsky My latest patch in the CL actually add another flag to Type 😄 dowidth is used in many places, at least in align.go, both checkwidth and expandiface will call dowidth, Also, dowidth is called inside package types, IIUC, we can't know the current context of typecheck when we are here. |
Right. That's why I said on the CL that I think fixing this is going to be more involved than I expected. |
|
Change https://golang.org/cl/192721 mentions this issue: |
|
Change https://golang.org/cl/192724 mentions this issue: |
|
@mdempsky just read CL 192721, if it went in, we can now derfercheckwidth and resumecheckwidth inside traceprint right? |
I tried and it doesn't work. The thing is that calling types.Fields will always trigger Dowidth, then for TINTER, Dowidth always call expandiface. So defercheckwidth won't help in this case. |
|
@cuonglm Thanks for the test report. I'm not surprised it still doesn't work. The couple of CLs I've mailed so far are just some low hanging fruit while I think about what needs to be done. |
This CL extends {defer,resume}checkwidth to support nesting, which
simplifies usage.
Updates #33658.
Change-Id: Ib3ffb8a7cabfae2cbeba74e21748c228436f4726
Reviewed-on: https://go-review.googlesource.com/c/go/+/192721
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
While here, change the params to be easier to understand: "t" is now always the type being updated, and "underlying" is now used to represent the underlying type. Updates #33658. Change-Id: Iabb64414ca3abaa8c780e4c9093e0c77b76fabf9 Reviewed-on: https://go-review.googlesource.com/c/go/+/192724 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Robert Griesemer <gri@golang.org>
This CL extends {defer,resume}checkwidth to support nesting, which
simplifies usage.
Updates golang#33658.
Change-Id: Ib3ffb8a7cabfae2cbeba74e21748c228436f4726
Reviewed-on: https://go-review.googlesource.com/c/go/+/192721
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
While here, change the params to be easier to understand: "t" is now always the type being updated, and "underlying" is now used to represent the underlying type. Updates golang#33658. Change-Id: Iabb64414ca3abaa8c780e4c9093e0c77b76fabf9 Reviewed-on: https://go-review.googlesource.com/c/go/+/192724 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Robert Griesemer <gri@golang.org>
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
Build go from source with compiler typechecking tracing enable (Debugging #31872 (comment)).
What did you expect to see?
Same compiler crashes, the same as when no typechecking tracing.
What did you see instead?
Compiler doesn't crash, produce different error.
The text was updated successfully, but these errors were encountered: