From 54fe57bc22f7890810bbddae2499eda8d4acfaef Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Thu, 17 Jun 2021 01:12:23 -0700 Subject: [dev.typeparams] cmd/compile: record writer's stack at export data sync points This CL extends the unified export data format's existing sync mechanism to save writer stacks, controlled by the -d=syncframes debug flag. This allows readers to provide more details when reporting desync errors, which should simplify development of the data format and the various reader/writer implementations. For example, CL 328051 updated reader and writer, but missed making a similar change to the linker (fix in CL 328054). Re-reviewing the CL in isolation after the failure, it was not immediately obvious what was going wrong. But the pair of stack traces below identifies exactly what happened: it should have updated linker.relocFuncExt to write out the new sync marker too. ``` data sync error: package "internal/abi", section 6, index 4, offset 536 found UseReloc, written at: /home/mdempsky/wd/go/src/cmd/compile/internal/noder/encoder.go:221: (*encoder).reloc +0x44 /home/mdempsky/wd/go/src/cmd/compile/internal/noder/linker.go:214: (*linker).relocFuncExt +0x580 /home/mdempsky/wd/go/src/cmd/compile/internal/noder/linker.go:233: (*linker).relocTypeExt +0x234 /home/mdempsky/wd/go/src/cmd/compile/internal/noder/linker.go:161: (*linker).relocObj +0x2198 /home/mdempsky/wd/go/src/cmd/compile/internal/noder/linker.go:64: (*linker).relocIdx +0x196 expected ImplicitTypes, reading at: /home/mdempsky/wd/go/src/cmd/compile/internal/noder/reader.go:796: (*reader).implicitTypes +0x36 /home/mdempsky/wd/go/src/cmd/compile/internal/noder/reader.go:810: (*reader).addBody +0x81 /home/mdempsky/wd/go/src/cmd/compile/internal/noder/reader.go:727: (*reader).funcExt +0x542 /home/mdempsky/wd/go/src/cmd/compile/internal/noder/reader.go:651: (*reader).method +0x324 /home/mdempsky/wd/go/src/cmd/compile/internal/noder/reader.go:557: (*pkgReader).objIdx +0x2704 ``` Change-Id: I911193edd2a965f81b7459f15fb613a773584685 Reviewed-on: https://go-review.googlesource.com/c/go/+/328909 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Trust: Matthew Dempsky Reviewed-by: Cuong Manh Le Reviewed-by: Robert Griesemer --- src/cmd/compile/internal/noder/encoder.go | 83 +++++++++++++++++++++++-------- 1 file changed, 61 insertions(+), 22 deletions(-) (limited to 'src/cmd/compile/internal/noder/encoder.go') diff --git a/src/cmd/compile/internal/noder/encoder.go b/src/cmd/compile/internal/noder/encoder.go index dc288dc29f..d8ab0f6255 100644 --- a/src/cmd/compile/internal/noder/encoder.go +++ b/src/cmd/compile/internal/noder/encoder.go @@ -13,6 +13,7 @@ import ( "go/constant" "io" "math/big" + "runtime" "cmd/compile/internal/base" ) @@ -93,6 +94,8 @@ type encoder struct { relocs []relocEnt data bytes.Buffer + encodingRelocHeader bool + k reloc idx int } @@ -107,6 +110,10 @@ func (w *encoder) flush() int { // TODO(mdempsky): Consider writing these out separately so they're // easier to strip, along with function bodies, so that we can prune // down to just the data that's relevant to go/types. + if w.encodingRelocHeader { + base.Fatalf("encodingRelocHeader already true; recursive flush?") + } + w.encodingRelocHeader = true w.sync(syncRelocs) w.len(len(w.relocs)) for _, rent := range w.relocs { @@ -128,10 +135,58 @@ func (w *encoder) checkErr(err error) { } } +func (w *encoder) rawUvarint(x uint64) { + var buf [binary.MaxVarintLen64]byte + n := binary.PutUvarint(buf[:], x) + _, err := w.data.Write(buf[:n]) + w.checkErr(err) +} + +func (w *encoder) rawVarint(x int64) { + // Zig-zag encode. + ux := uint64(x) << 1 + if x < 0 { + ux = ^ux + } + + w.rawUvarint(ux) +} + +func (w *encoder) rawReloc(r reloc, idx int) int { + // TODO(mdempsky): Use map for lookup. + for i, rent := range w.relocs { + if rent.kind == r && rent.idx == idx { + return i + } + } + + i := len(w.relocs) + w.relocs = append(w.relocs, relocEnt{r, idx}) + return i +} + func (w *encoder) sync(m syncMarker) { - if debug { - err := w.data.WriteByte(byte(m)) - w.checkErr(err) + if !enableSync { + return + } + + // Writing out stack frame string references requires working + // relocations, but writing out the relocations themselves involves + // sync markers. To prevent infinite recursion, we simply trim the + // stack frame for sync markers within the relocation header. + var frames []string + if !w.encodingRelocHeader && base.Debug.SyncFrames > 0 { + pcs := make([]uintptr, base.Debug.SyncFrames) + n := runtime.Callers(2, pcs) + frames = fmtFrames(pcs[:n]...) + } + + // TODO(mdempsky): Save space by writing out stack frames as a + // linked list so we can share common stack frames. + w.rawUvarint(uint64(m)) + w.rawUvarint(uint64(len(frames))) + for _, frame := range frames { + w.rawUvarint(uint64(w.rawReloc(relocString, w.p.stringIdx(frame)))) } } @@ -148,18 +203,12 @@ func (w *encoder) bool(b bool) bool { func (w *encoder) int64(x int64) { w.sync(syncInt64) - var buf [binary.MaxVarintLen64]byte - n := binary.PutVarint(buf[:], x) - _, err := w.data.Write(buf[:n]) - w.checkErr(err) + w.rawVarint(x) } func (w *encoder) uint64(x uint64) { w.sync(syncUint64) - var buf [binary.MaxVarintLen64]byte - n := binary.PutUvarint(buf[:], x) - _, err := w.data.Write(buf[:n]) - w.checkErr(err) + w.rawUvarint(x) } func (w *encoder) len(x int) { assert(x >= 0); w.uint64(uint64(x)) } @@ -168,17 +217,7 @@ func (w *encoder) uint(x uint) { w.uint64(uint64(x)) } func (w *encoder) reloc(r reloc, idx int) { w.sync(syncUseReloc) - - // TODO(mdempsky): Use map for lookup. - for i, rent := range w.relocs { - if rent.kind == r && rent.idx == idx { - w.len(i) - return - } - } - - w.len(len(w.relocs)) - w.relocs = append(w.relocs, relocEnt{r, idx}) + w.len(w.rawReloc(r, idx)) } func (w *encoder) code(c code) { -- cgit v1.3