From d63a9b8a119442c4814c7bd71efde9a48595666a Mon Sep 17 00:00:00 2001 From: Nevkontakte Date: Fri, 2 Aug 2024 23:15:41 +0100 Subject: [PATCH 1/2] Include original function names into source maps. The main change in this commit is an ability to use identifier name mapping, which we use to show original function names in the source map. This addresses the long-standing https://github.com/gopherjs/gopherjs/issues/1085, where GopherJS call stacks were somewhat difficult to interpret due to function name mangling, especially in minified form. Now we emit an additional source map hit with the original function name, which Node is able to pick up. While at it, I moved source map hinting logic into its own package with tests and added some documentation on how it works. Now it should be easy to extend this mechanism for even richer source maps if we want to. --- build/build.go | 47 ++++--- compiler/compiler.go | 89 ++---------- compiler/compiler_test.go | 6 +- compiler/functions.go | 13 +- compiler/package.go | 3 +- compiler/utils.go | 18 ++- internal/sourcemapx/doc.go | 26 ++++ internal/sourcemapx/filter.go | 65 +++++++++ internal/sourcemapx/filter_test.go | 82 +++++++++++ internal/sourcemapx/hint.go | 127 +++++++++++++++++ internal/sourcemapx/hint_test.go | 182 +++++++++++++++++++++++++ internal/sourcemapx/identifier.go | 38 ++++++ internal/sourcemapx/identifier_test.go | 39 ++++++ tool.go | 3 +- 14 files changed, 626 insertions(+), 112 deletions(-) create mode 100644 internal/sourcemapx/doc.go create mode 100644 internal/sourcemapx/filter.go create mode 100644 internal/sourcemapx/filter_test.go create mode 100644 internal/sourcemapx/hint.go create mode 100644 internal/sourcemapx/hint_test.go create mode 100644 internal/sourcemapx/identifier.go create mode 100644 internal/sourcemapx/identifier_test.go diff --git a/build/build.go b/build/build.go index 46786a30b..674a542a7 100644 --- a/build/build.go +++ b/build/build.go @@ -30,6 +30,7 @@ import ( "github.com/gopherjs/gopherjs/compiler/jsFile" "github.com/gopherjs/gopherjs/compiler/sources" "github.com/gopherjs/gopherjs/internal/errorList" + "github.com/gopherjs/gopherjs/internal/sourcemapx" "github.com/gopherjs/gopherjs/internal/testmain" log "github.com/sirupsen/logrus" @@ -1216,9 +1217,9 @@ func (s *Session) ImportResolverFor(srcDir string) func(string) (*compiler.Archi } } -// SourceMappingCallback returns a call back for compiler.SourceMapFilter +// SourceMappingCallback returns a callback for [github.com/gopherjs/gopherjs/compiler.SourceMapFilter] // configured for the current build session. -func (s *Session) SourceMappingCallback(m *sourcemap.Map) func(generatedLine, generatedColumn int, originalPos token.Position) { +func (s *Session) SourceMappingCallback(m *sourcemap.Map) func(generatedLine, generatedColumn int, originalPos token.Position, originalName string) { return NewMappingCallback(m, s.xctx.Env().GOROOT, s.xctx.Env().GOPATH, s.options.MapToLocalDisk) } @@ -1233,7 +1234,7 @@ func (s *Session) WriteCommandPackage(archive *compiler.Archive, pkgObj string) } defer codeFile.Close() - sourceMapFilter := &compiler.SourceMapFilter{Writer: codeFile} + sourceMapFilter := &sourcemapx.Filter{Writer: codeFile} if s.options.CreateMapFile { m := &sourcemap.Map{File: filepath.Base(pkgObj)} mapFile, err := os.Create(pkgObj + ".map") @@ -1258,27 +1259,33 @@ func (s *Session) WriteCommandPackage(archive *compiler.Archive, pkgObj string) } // NewMappingCallback creates a new callback for source map generation. -func NewMappingCallback(m *sourcemap.Map, goroot, gopath string, localMap bool) func(generatedLine, generatedColumn int, originalPos token.Position) { - return func(generatedLine, generatedColumn int, originalPos token.Position) { - if !originalPos.IsValid() { - m.AddMapping(&sourcemap.Mapping{GeneratedLine: generatedLine, GeneratedColumn: generatedColumn}) - return +func NewMappingCallback(m *sourcemap.Map, goroot, gopath string, localMap bool) func(generatedLine, generatedColumn int, originalPos token.Position, originalName string) { + return func(generatedLine, generatedColumn int, originalPos token.Position, originalName string) { + mapping := &sourcemap.Mapping{GeneratedLine: generatedLine, GeneratedColumn: generatedColumn} + + if originalPos.IsValid() { + file := originalPos.Filename + + switch hasGopathPrefix, prefixLen := hasGopathPrefix(file, gopath); { + case localMap: + // no-op: keep file as-is + case hasGopathPrefix: + file = filepath.ToSlash(file[prefixLen+4:]) + case strings.HasPrefix(file, goroot): + file = filepath.ToSlash(file[len(goroot)+4:]) + default: + file = filepath.Base(file) + } + mapping.OriginalFile = file + mapping.OriginalLine = originalPos.Line + mapping.OriginalColumn = originalPos.Column } - file := originalPos.Filename - - switch hasGopathPrefix, prefixLen := hasGopathPrefix(file, gopath); { - case localMap: - // no-op: keep file as-is - case hasGopathPrefix: - file = filepath.ToSlash(file[prefixLen+4:]) - case strings.HasPrefix(file, goroot): - file = filepath.ToSlash(file[len(goroot)+4:]) - default: - file = filepath.Base(file) + if originalName != "" { + mapping.OriginalName = originalName } - m.AddMapping(&sourcemap.Mapping{GeneratedLine: generatedLine, GeneratedColumn: generatedColumn, OriginalFile: file, OriginalLine: originalPos.Line, OriginalColumn: originalPos.Column}) + m.AddMapping(mapping) } } diff --git a/compiler/compiler.go b/compiler/compiler.go index e8264c946..8cebc27ab 100644 --- a/compiler/compiler.go +++ b/compiler/compiler.go @@ -7,7 +7,6 @@ package compiler import ( "bytes" - "encoding/binary" "encoding/gob" "encoding/json" "fmt" @@ -20,6 +19,7 @@ import ( "github.com/gopherjs/gopherjs/compiler/internal/dce" "github.com/gopherjs/gopherjs/compiler/linkname" "github.com/gopherjs/gopherjs/compiler/prelude" + "github.com/gopherjs/gopherjs/internal/sourcemapx" "golang.org/x/tools/go/gcexportdata" ) @@ -108,7 +108,13 @@ func ImportDependencies(archive *Archive, importPkg func(string) (*Archive, erro return deps, nil } -func WriteProgramCode(pkgs []*Archive, w *SourceMapFilter, goVersion string) error { +type dceInfo struct { + decl *Decl + objectFilter string + methodFilter string +} + +func WriteProgramCode(pkgs []*Archive, w *sourcemapx.Filter, goVersion string) error { mainPkg := pkgs[len(pkgs)-1] minify := mainPkg.Minified @@ -165,9 +171,9 @@ func WriteProgramCode(pkgs []*Archive, w *SourceMapFilter, goVersion string) err return nil } -func WritePkgCode(pkg *Archive, dceSelection map[*Decl]struct{}, gls linkname.GoLinknameSet, minify bool, w *SourceMapFilter) error { +func WritePkgCode(pkg *Archive, dceSelection map[*Decl]struct{}, gls linkname.GoLinknameSet, minify bool, w *sourcemapx.Filter) error { if w.MappingCallback != nil && pkg.FileSet != nil { - w.fileSet = pkg.FileSet + w.FileSet = pkg.FileSet } if _, err := w.Write(pkg.IncJSCode); err != nil { return err @@ -316,77 +322,6 @@ func ReadArchive(importPath string, r io.Reader, srcModTime time.Time, imports m } // WriteArchive writes compiled package archive on disk for later reuse. -// -// The passed in buildTime is used to determine if the archive is out-of-date. -// Typically it should be set to the srcModTime or time.Now() but it is exposed for testing purposes. -func WriteArchive(a *Archive, buildTime time.Time, w io.Writer) error { - exportData := new(bytes.Buffer) - if a.Package != nil { - if err := gcexportdata.Write(exportData, nil, a.Package); err != nil { - return fmt.Errorf("failed to write export data: %w", err) - } - } - - encodedFileSet := new(bytes.Buffer) - if a.FileSet != nil { - if err := a.FileSet.Write(json.NewEncoder(encodedFileSet).Encode); err != nil { - return err - } - } - - sa := serializableArchive{ - ImportPath: a.ImportPath, - Name: a.Name, - Imports: a.Imports, - ExportData: exportData.Bytes(), - Declarations: a.Declarations, - IncJSCode: a.IncJSCode, - FileSet: encodedFileSet.Bytes(), - Minified: a.Minified, - GoLinknames: a.GoLinknames, - BuildTime: buildTime, - } - - return gob.NewEncoder(w).Encode(sa) -} - -type SourceMapFilter struct { - Writer io.Writer - MappingCallback func(generatedLine, generatedColumn int, originalPos token.Position) - line int - column int - fileSet *token.FileSet -} - -func (f *SourceMapFilter) Write(p []byte) (n int, err error) { - var n2 int - for { - i := bytes.IndexByte(p, '\b') - w := p - if i != -1 { - w = p[:i] - } - - n2, err = f.Writer.Write(w) - n += n2 - for { - i := bytes.IndexByte(w, '\n') - if i == -1 { - f.column += len(w) - break - } - f.line++ - f.column = 0 - w = w[i+1:] - } - - if err != nil || i == -1 { - return - } - if f.MappingCallback != nil { - f.MappingCallback(f.line+1, f.column, f.fileSet.Position(token.Pos(binary.BigEndian.Uint32(p[i+1:i+5])))) - } - p = p[i+5:] - n += 5 - } +func WriteArchive(a *Archive, w io.Writer) error { + return gob.NewEncoder(w).Encode(a) } diff --git a/compiler/compiler_test.go b/compiler/compiler_test.go index e7add0015..669979cf8 100644 --- a/compiler/compiler_test.go +++ b/compiler/compiler_test.go @@ -9,6 +9,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/gopherjs/gopherjs/internal/sourcemapx" "golang.org/x/tools/go/packages" "github.com/gopherjs/gopherjs/compiler/internal/dce" @@ -846,11 +847,10 @@ func reloadCompiledProject(t *testing.T, archives map[string]*Archive, rootPkgPa // the old recursive archive loading that is no longer used since it // doesn't allow cross package analysis for generings. - buildTime := newTime(5.0) serialized := map[string][]byte{} for path, a := range archives { buf := &bytes.Buffer{} - if err := WriteArchive(a, buildTime, buf); err != nil { + if err := WriteArchive(a, buf); err != nil { t.Fatalf(`failed to write archive for %s: %v`, path, err) } serialized[path] = buf.Bytes() @@ -903,7 +903,7 @@ func renderPackage(t *testing.T, archive *Archive, minify bool) string { buf := &bytes.Buffer{} - if err := WritePkgCode(archive, selection, linkname.GoLinknameSet{}, minify, &SourceMapFilter{Writer: buf}); err != nil { + if err := WritePkgCode(archive, selection, linkname.GoLinknameSet{}, minify, &sourcemapx.Filter{Writer: buf}); err != nil { t.Fatal(err) } diff --git a/compiler/functions.go b/compiler/functions.go index 592992efc..f18e12429 100644 --- a/compiler/functions.go +++ b/compiler/functions.go @@ -16,6 +16,7 @@ import ( "github.com/gopherjs/gopherjs/compiler/internal/analysis" "github.com/gopherjs/gopherjs/compiler/internal/typeparams" "github.com/gopherjs/gopherjs/compiler/typesutil" + "github.com/gopherjs/gopherjs/internal/sourcemapx" ) // nestedFunctionContext creates a new nested context for a function corresponding @@ -64,7 +65,11 @@ func (fc *funcContext) nestedFunctionContext(info *analysis.FuncInfo, inst typep if recvType := typesutil.RecvType(sig); recvType != nil { funcRef = recvType.Obj().Name() + midDot + funcRef } - c.funcRef = c.newVariable(funcRef, true /*pkgLevel*/) + c.funcRef = sourcemapx.Identifier{ + Name: c.newVariable(funcRef, true /*pkgLevel*/), + OriginalName: o.FullName(), + OriginalPos: o.Pos(), + } return c } @@ -299,11 +304,11 @@ func (fc *funcContext) translateFunctionBody(typ *ast.FuncType, recv *ast.Ident, localVars = append(localVars, "$r") // funcRef identifies the function object itself, so it doesn't need to be saved // or restored. - localVars = removeMatching(localVars, fc.funcRef) + localVars = removeMatching(localVars, fc.funcRef.Name) // If a blocking function is being resumed, initialize local variables from the saved context. localVarDefs = fmt.Sprintf("var {%s, $c} = $restore(this, {%s});\n", strings.Join(localVars, ", "), strings.Join(args, ", ")) // If the function gets blocked, save local variables for future. - saveContext := fmt.Sprintf("var $f = {$blk: "+fc.funcRef+", $c: true, $r, %s};", strings.Join(fc.localVars, ", ")) + saveContext := fmt.Sprintf("var $f = {$blk: %s, $c: true, $r, %s};", fc.funcRef, strings.Join(fc.localVars, ", ")) suffix = " " + saveContext + "return $f;" + suffix } else if len(fc.localVars) > 0 { @@ -351,5 +356,5 @@ func (fc *funcContext) translateFunctionBody(typ *ast.FuncType, recv *ast.Ident, fc.pkgCtx.escapingVars = prevEV - return fmt.Sprintf("function %s(%s) {\n%s%s}", fc.funcRef, strings.Join(args, ", "), bodyOutput, fc.Indentation(0)) + return fmt.Sprintf("%sfunction %s(%s) {\n%s%s}", fc.funcRef.EncodeHint(), fc.funcRef, strings.Join(args, ", "), bodyOutput, fc.Indentation(0)) } diff --git a/compiler/package.go b/compiler/package.go index 8f336130d..0838c14d1 100644 --- a/compiler/package.go +++ b/compiler/package.go @@ -15,6 +15,7 @@ import ( "github.com/gopherjs/gopherjs/compiler/sources" "github.com/gopherjs/gopherjs/compiler/typesutil" "github.com/gopherjs/gopherjs/internal/errorList" + "github.com/gopherjs/gopherjs/internal/sourcemapx" ) // pkgContext maintains compiler context for a specific package. @@ -60,7 +61,7 @@ type funcContext struct { // "function" keyword in the generated code). This identifier can be used // within the function scope to reference the function object. It will also // appear in the stack trace. - funcRef string + funcRef sourcemapx.Identifier // Surrounding package context. pkgCtx *pkgContext // Function context, surrounding this function definition. For package-level diff --git a/compiler/utils.go b/compiler/utils.go index 83b826ce2..4d1ade057 100644 --- a/compiler/utils.go +++ b/compiler/utils.go @@ -2,7 +2,6 @@ package compiler import ( "bytes" - "encoding/binary" "errors" "fmt" "go/ast" @@ -21,6 +20,7 @@ import ( "github.com/gopherjs/gopherjs/compiler/internal/analysis" "github.com/gopherjs/gopherjs/compiler/internal/typeparams" "github.com/gopherjs/gopherjs/compiler/typesutil" + "github.com/gopherjs/gopherjs/internal/sourcemapx" ) // We use this character as a separator in synthetic identifiers instead of a @@ -71,8 +71,13 @@ func (fc *funcContext) SetPos(pos token.Pos) { func (fc *funcContext) writePos() { if fc.posAvailable { fc.posAvailable = false - fc.Write([]byte{'\b'}) - binary.Write(fc, binary.BigEndian, uint32(fc.pos)) + h := sourcemapx.Hint{} + if err := h.Pack(fc.pos); err != nil { + panic(bailout(fmt.Errorf("failed to pack source map position: %w", err))) + } + if _, err := h.WriteTo(fc); err != nil { + panic(bailout(fmt.Errorf("failed to write source map hint: %w", err))) + } } } @@ -832,7 +837,7 @@ func getJsTag(tag string) string { } func needsSpace(c byte) bool { - return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' || c == '$' + return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' || c == '$' || c == '\b' } func removeWhitespace(b []byte, minify bool) []byte { @@ -845,8 +850,9 @@ func removeWhitespace(b []byte, minify bool) []byte { for len(b) > 0 { switch b[0] { case '\b': - out = append(out, b[:5]...) - b = b[5:] + _, length := sourcemapx.ReadHint(b) + out = append(out, b[:length]...) + b = b[length:] continue case ' ', '\t', '\n': if (!needsSpace(previous) || !needsSpace(b[1])) && !(previous == '-' && b[1] == '-') { diff --git a/internal/sourcemapx/doc.go b/internal/sourcemapx/doc.go new file mode 100644 index 000000000..20e7f8eac --- /dev/null +++ b/internal/sourcemapx/doc.go @@ -0,0 +1,26 @@ +// Package sourcemapx contains utilities for passing source map information +// around, intended to work with github.com/neelance/sourcemap. +// +// GopherJS code generator outputs hints about correspondence between the +// generated code and original sources inline. Such hints are marked by the +// special `\b` (0x08) magic byte, followed by a variable-length sequence of +// bytes, which can be extracted from the byte slice using ReadHint() function. +// +// '\b' was chosen as a magic symbol because it would never occur unescaped in +// the generated code, other than when explicitly inserted by the source mapping +// hint. See Hint type documentation for the details of the encoded format. +// +// The hinting mechanism is designed to be extensible, the Hint type able to +// wrap different types containing different information: +// +// - go/token.Pos indicates position in the original source the current +// location in the generated code corresponds to. +// - Identifier maps a JS identifier to the original Go identifier it +// represents. +// +// More types may be added in future if necessary. +// +// Filter type is used to extract the hints from the written code stream and +// pass them into source map generator. It also ensures that the encoded inline +// hints don't make it into the final output, since they are not valid JS. +package sourcemapx diff --git a/internal/sourcemapx/filter.go b/internal/sourcemapx/filter.go new file mode 100644 index 000000000..73f0f8faf --- /dev/null +++ b/internal/sourcemapx/filter.go @@ -0,0 +1,65 @@ +package sourcemapx + +import ( + "bytes" + "fmt" + "go/token" + "io" +) + +// Filter implements io.Writer which extracts source map hints from the written +// stream and passed them to the MappingCallback if it's not nil. Encoded hints +// are always filtered out of the output stream. +type Filter struct { + Writer io.Writer + FileSet *token.FileSet + MappingCallback func(generatedLine, generatedColumn int, originalPos token.Position, originalName string) + + line int + column int +} + +func (f *Filter) Write(p []byte) (n int, err error) { + var n2 int + for { + i := FindHint(p) + w := p + if i != -1 { + w = p[:i] + } + + n2, err = f.Writer.Write(w) + n += n2 + for { + i := bytes.IndexByte(w, '\n') + if i == -1 { + f.column += len(w) + break + } + f.line++ + f.column = 0 + w = w[i+1:] + } + + if err != nil || i == -1 { + return + } + h, length := ReadHint(p[i:]) + if f.MappingCallback != nil { + value, err := h.Unpack() + if err != nil { + panic(fmt.Errorf("failed to unpack source map hint: %w", err)) + } + switch value := value.(type) { + case token.Pos: + f.MappingCallback(f.line+1, f.column, f.FileSet.Position(value), "") + case Identifier: + f.MappingCallback(f.line+1, f.column, f.FileSet.Position(value.OriginalPos), value.OriginalName) + default: + panic(fmt.Errorf("unexpected source map hint type: %T", value)) + } + } + p = p[i+length:] + n += length + } +} diff --git a/internal/sourcemapx/filter_test.go b/internal/sourcemapx/filter_test.go new file mode 100644 index 000000000..3509527a1 --- /dev/null +++ b/internal/sourcemapx/filter_test.go @@ -0,0 +1,82 @@ +package sourcemapx + +import ( + "bytes" + "fmt" + "go/token" + "io" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestFilter(t *testing.T) { + type entry struct { + GenLine int + GenCol int + OrigPos token.Position + OrigName string + } + entries := []entry{} + + code := &bytes.Buffer{} + + filter := &Filter{ + Writer: code, + MappingCallback: func(generatedLine, generatedColumn int, originalPos token.Position, originalName string) { + entries = append(entries, entry{ + GenLine: generatedLine, + GenCol: generatedColumn, + OrigPos: originalPos, + OrigName: originalName, + }) + }, + FileSet: token.NewFileSet(), + } + + { + f := filter.FileSet.AddFile("foo.go", filter.FileSet.Base(), 42) + f.AddLine(0) + f.AddLine(10) + f.AddLine(30) + } + writeHint(t, filter, token.NoPos) + fmt.Fprintln(filter, "Hello") + writeHint(t, filter, token.Pos(16)) + fmt.Fprintln(filter, "World") + + ident := Identifier{ + Name: "foo$1", + OriginalName: "main.Foo", + OriginalPos: token.Pos(32), + } + fmt.Fprintf(filter, "var x = %sfunction %s() {};\n", ident.EncodeHint(), ident) + + wantCode := `Hello +World +var x = function foo$1() {}; +` + if diff := cmp.Diff(wantCode, code.String()); diff != "" { + t.Errorf("Generated code differs from expected (-want,+got):\n%s", diff) + } + + wantEntries := []entry{ + {GenLine: 1}, + {GenLine: 2, OrigPos: token.Position{Filename: "foo.go", Line: 2, Column: 6, Offset: 15}}, + {GenLine: 3, GenCol: 8, OrigPos: token.Position{Filename: "foo.go", Line: 3, Column: 2, Offset: 31}, OrigName: "main.Foo"}, + } + if diff := cmp.Diff(wantEntries, entries); diff != "" { + t.Errorf("Source map entries differ from expected (-want,+got):\n%s", diff) + } +} + +func writeHint(t *testing.T, w io.Writer, value any) { + t.Helper() + hint := Hint{} + if err := hint.Pack(value); err != nil { + t.Fatalf("Got: hint.Pack(%#v) returned error: %s. Want: no error.", value, err) + } + if _, err := hint.WriteTo(w); err != nil { + t.Fatalf("Got: hint.WriteTo() returned error: %s. Want: no error.", err) + } +} diff --git a/internal/sourcemapx/hint.go b/internal/sourcemapx/hint.go new file mode 100644 index 000000000..481cad81a --- /dev/null +++ b/internal/sourcemapx/hint.go @@ -0,0 +1,127 @@ +package sourcemapx + +import ( + "bytes" + "encoding/binary" + "encoding/gob" + "fmt" + "go/token" + "io" + "reflect" +) + +// A magic byte in the generated code output that indicates a beginning of a +// source map hint. The character has been chosen because it should never show +// up in valid generated code unescaped, other than for source map hint purposes. +const HintMagic byte = '\b' + +// Hint is a container for a sourcemap hint that can be embedded into the +// generated code stream. Payload size and semantics depend on the nature of the +// hint. +// +// Within the stream, the hint is encoded in the following binary format: +// - magic: 0x08 - ASCII backspace, magic symbol indicating the beginning of the hint; +// - size: 16 bit, big endian unsigned int - the size of the payload. +// - payload: [size]byte - the payload of the hint. +type Hint struct { + Payload []byte +} + +// FindHint returns the lowest index in the byte slice where a source map Hint +// is embedded or -1 if it isn't found. Invariant: if FindHint(b) != -1 then +// b[FindHint(b)] == '\b'. +func FindHint(b []byte) int { + return bytes.IndexByte(b, HintMagic) +} + +// ReadHint reads the Hint from the beginning of the byte slice and returns +// the hint and the number of bytes in the slice it occupies. The caller is +// expected to find the location of the hint using FindHint prior to calling +// this function. +// +// Returned hint payload does not share backing array with b. +// +// Function panics if: +// - b[0] != '\b' +// - len(b) < size + 3 +func ReadHint(b []byte) (h Hint, length int) { + if len(b) < 3 { + panic(fmt.Errorf("byte slice too short to contain hint header: len(b) = %d", len(b))) + } + if b[0] != HintMagic { + panic(fmt.Errorf("byte slice doesn't start with magic 0x%x: b[0] = 0x%x", HintMagic, b[0])) + } + size := int(binary.BigEndian.Uint16(b[1:3])) + if len(b) < size+3 { + panic(fmt.Errorf("byte slice it too short to contain hint payload: len(b) = %d, expected hint size: %d", len(b), size+3)) + } + + h.Payload = make([]byte, size) + copy(h.Payload, b[3:]) + return h, size + 3 +} + +// WriteTo the encoded hint into the output stream. Panics if payload is longer +// than 0xFFFF bytes. +func (h *Hint) WriteTo(w io.Writer) (int64, error) { + if len(h.Payload) > 0xFFFF { + panic(fmt.Errorf("hint payload may not be longer than %d bytes, got: %d", 0xFFFF, len(h.Payload))) + } + encoded := []byte{HintMagic} + encoded = binary.BigEndian.AppendUint16(encoded, uint16(len(h.Payload))) + encoded = append(encoded, h.Payload...) + + n, err := w.Write(encoded) + if err != nil { + return int64(n), fmt.Errorf("failed to write hint: %w", err) + } + + return int64(n), nil +} + +// Pack the given value into hint's payload. +// +// Supported types: go/token.Pos. +// +// The first byte of the payload will indicate the encoded type, and the rest +// is an opaque, type-dependent binary representation of the type. +func (h *Hint) Pack(value any) error { + payload := &bytes.Buffer{} + // Write type flag. + switch value.(type) { + case token.Pos: + payload.WriteByte(1) + case Identifier: + payload.WriteByte(2) + default: + return fmt.Errorf("unsupported hint payload type %T", value) + } + + if err := gob.NewEncoder(payload).Encode(value); err != nil { + return fmt.Errorf("failed to encode hint payload: %w", err) + } + + h.Payload = payload.Bytes() + return nil +} + +// Unpack and return hint's payload, previously packed by Pack(). +func (h *Hint) Unpack() (any, error) { + if len(h.Payload) < 1 { + return nil, fmt.Errorf("payload is too short to contain type flag") + } + var value any + switch h.Payload[0] { + case 1: + v := token.NoPos + value = &v + case 2: + value = &Identifier{} + default: + return nil, fmt.Errorf("unsupported hint payload type flag: %d", h.Payload[0]) + } + if err := gob.NewDecoder(bytes.NewReader(h.Payload[1:])).Decode(value); err != nil { + return nil, fmt.Errorf("failed to decode hint payload as %T: %w", value, err) + } + return reflect.ValueOf(value).Elem().Interface(), nil +} diff --git a/internal/sourcemapx/hint_test.go b/internal/sourcemapx/hint_test.go new file mode 100644 index 000000000..9b20a1de9 --- /dev/null +++ b/internal/sourcemapx/hint_test.go @@ -0,0 +1,182 @@ +package sourcemapx + +import ( + "bytes" + "fmt" + "go/token" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" +) + +var encodedHint = []byte{ + 104, 101, 108, 108, 111, 44, 32, 119, 111, 114, 108, 100, 33, // "hello, world!" + HintMagic, // Magic. + 0x00, 0x04, // Size. + 0x01, 0x02, 0x03, 0x04, // Payload. + 103, 111, 112, 104, 101, 114, 115, // "gophers" +} + +func TestFindHint(t *testing.T) { + t.Run("found", func(t *testing.T) { + got := FindHint(encodedHint) + want := 13 + if got != want { + t.Errorf("Got: FindHint(encodedHint) = %d. Want: %d.", got, want) + } + if magic := encodedHint[got]; magic != HintMagic { + t.Errorf("Got: magic at hint position: %x. Want: %x.", magic, HintMagic) + } + }) + + t.Run("not found", func(t *testing.T) { + got := FindHint(encodedHint[14:]) // Slice past the hint location. + want := -1 + if got != want { + t.Errorf("Got: FindHint(encodedHint) = %d. Want: %d.", got, want) + } + }) +} + +func TestReadHint(t *testing.T) { + t.Run("success", func(t *testing.T) { + tests := []struct { + descr string + bytes []byte + wantHint Hint + wantLength int + }{{ + descr: "1234", + bytes: encodedHint[13:], + wantHint: Hint{Payload: []byte{1, 2, 3, 4}}, + wantLength: 7, + }, { + descr: "empty", + bytes: []byte{HintMagic, 0x00, 0x00}, + wantHint: Hint{Payload: []byte{}}, + wantLength: 3, + }} + + for _, test := range tests { + t.Run(test.descr, func(t *testing.T) { + // Copy input data to avoid clobbering it further in the test. + b := make([]byte, len(test.bytes)) + copy(b, test.bytes) + + hint, length := ReadHint(b) + if length != test.wantLength { + t.Errorf("Got: read hint length = %d. Want: %d.", length, test.wantLength) + } + // Zero out input bytes to make sure returned hint is not backed by the + // same memory. + for i := range b { + b[i] = 0 + } + if diff := cmp.Diff(test.wantHint, hint); diff != "" { + t.Errorf("ReadHint() returned diff (-want,+got):\n%s", diff) + } + }) + } + }) + + t.Run("panic", func(t *testing.T) { + tests := []struct { + descr string + bytes []byte + panic string + }{{ + descr: "incomplete header", + bytes: []byte{HintMagic, 0x00}, + panic: "too short to contain hint header", + }, { + descr: "incomplete payload", + bytes: []byte{HintMagic, 0x00, 0x01}, + panic: "too short to contain hint payload", + }, { + descr: "wrong magic", + bytes: []byte{'a', 0x00, 0x01, 0x00}, + panic: "doesn't start with magic", + }} + + for _, test := range tests { + t.Run(test.descr, func(t *testing.T) { + defer func() { + err := recover() + if err == nil { + t.Fatalf("Got: no panic. Expected a panic.") + } + if !strings.Contains(fmt.Sprint(err), test.panic) { + t.Errorf("Got panic: %v. Expected to contain: %s.", err, test.panic) + } + }() + + ReadHint(test.bytes) + }) + } + }) +} + +func TestHintWrite(t *testing.T) { + tests := []struct { + descr string + hint Hint + want []byte + }{{ + descr: "empty", + hint: Hint{}, + want: []byte{HintMagic, 0x00, 0x00}, + }, { + descr: "1234", + hint: Hint{Payload: []byte{1, 2, 3, 4}}, + want: []byte{HintMagic, 0x00, 0x04, 1, 2, 3, 4}, + }} + + for _, test := range tests { + t.Run(test.descr, func(t *testing.T) { + buf := &bytes.Buffer{} + if _, err := test.hint.WriteTo(buf); err != nil { + t.Fatalf("Got: hint.Write() returned error: %s. Want: no error.", err) + } + if diff := cmp.Diff(test.want, buf.Bytes()); diff != "" { + t.Fatalf("%#v.Write() returned diff (-want,+got):\n%s", test.hint, diff) + } + }) + } +} + +func TestHintPack(t *testing.T) { + tests := []struct { + descr string + value any + }{{ + descr: "empty position", + value: token.NoPos, + }, { + descr: "non empty position", + value: token.Pos(42), + }, { + descr: "identifier", + value: Identifier{ + Name: "foo$1", + OriginalName: "foo", + OriginalPos: token.Pos(42), + }, + }} + + for _, test := range tests { + t.Run(test.descr, func(t *testing.T) { + h := Hint{} + if err := h.Pack(test.value); err != nil { + t.Fatalf("h.Pack(%#v) returned error: %s. Want: no error.", test.value, err) + } + unpacked, err := h.Unpack() + if err != nil { + t.Fatalf("h.Unpack() returned error: %s. Want: no error.", err) + } + if diff := cmp.Diff(test.value, unpacked); diff != "" { + t.Errorf("Unpacked value doesn't match the original (-want,+got):\n%s", diff) + } + }) + } +} diff --git a/internal/sourcemapx/identifier.go b/internal/sourcemapx/identifier.go new file mode 100644 index 000000000..6b24a0f15 --- /dev/null +++ b/internal/sourcemapx/identifier.go @@ -0,0 +1,38 @@ +package sourcemapx + +import ( + "fmt" + "go/token" + "strings" +) + +// Identifier represents a generated code identifier with a the associated +// original identifier information, which can be used to produce a source map. +// +// This allows us to map a JS function or variable name back to the original Go +// symbol name. +type Identifier struct { + Name string // Identifier to use in the generated code. + OriginalName string // Original identifier name. + OriginalPos token.Pos // Original identifier position. +} + +// String returns generated code identifier name. +func (i Identifier) String() string { + return i.Name +} + +// EncodeHint returns a string with an encoded source map hint. The hint can be +// inserted into the generated code to be later extracted by the SourceMapFilter +// to produce a source map. +func (i Identifier) EncodeHint() string { + buf := &strings.Builder{} + h := Hint{} + if err := h.Pack(i); err != nil { + panic(fmt.Errorf("failed to pack identifier source map hint: %w", err)) + } + if _, err := h.WriteTo(buf); err != nil { + panic(fmt.Errorf("failed to write source map hint into a buffer: %w", err)) + } + return buf.String() +} diff --git a/internal/sourcemapx/identifier_test.go b/internal/sourcemapx/identifier_test.go new file mode 100644 index 000000000..215166451 --- /dev/null +++ b/internal/sourcemapx/identifier_test.go @@ -0,0 +1,39 @@ +package sourcemapx + +import ( + "go/token" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestIdentifier_String(t *testing.T) { + ident := Identifier{ + Name: "Foo$1", + OriginalName: "Foo", + OriginalPos: token.Pos(42), + } + + got := ident.String() + if got != ident.Name { + t.Errorf("Got: ident.String() = %q. Want: %q.", got, ident.Name) + } +} + +func TestIdentifier_EncodeHint(t *testing.T) { + original := Identifier{ + Name: "Foo$1", + OriginalName: "Foo", + OriginalPos: token.Pos(42), + } + + encoded := original.EncodeHint() + hint, _ := ReadHint([]byte(encoded)) + decoded, err := hint.Unpack() + if err != nil { + t.Fatalf("Got: hint.Unpack() returned error: %s. Want: no error.", err) + } + if diff := cmp.Diff(original, decoded); diff != "" { + t.Fatalf("Decoded hint differs from the original (-want,+got):\n%s", diff) + } +} diff --git a/tool.go b/tool.go index 46d6a6edc..d5f2be1d5 100644 --- a/tool.go +++ b/tool.go @@ -27,6 +27,7 @@ import ( "github.com/gopherjs/gopherjs/build/cache" "github.com/gopherjs/gopherjs/compiler" "github.com/gopherjs/gopherjs/internal/errorList" + "github.com/gopherjs/gopherjs/internal/sourcemapx" "github.com/gopherjs/gopherjs/internal/sysutil" "github.com/neelance/sourcemap" log "github.com/sirupsen/logrus" @@ -647,7 +648,7 @@ func (fs serveCommandFileSystem) Open(requestName string) (http.File, error) { return err } - sourceMapFilter := &compiler.SourceMapFilter{Writer: buf} + sourceMapFilter := &sourcemapx.Filter{Writer: buf} m := &sourcemap.Map{File: base + ".js"} sourceMapFilter.MappingCallback = s.SourceMappingCallback(m) From 059be373e688a48b6ded75b3c126152ac9746dfa Mon Sep 17 00:00:00 2001 From: Nevkontakte Date: Sat, 3 Aug 2024 19:26:47 +0100 Subject: [PATCH 2/2] Update and re-enable tests that rely on Go function names. The tests that were previously failing should work again with the new function name source maps. There are only two caveats at this point: - For some reasons a function that's currently executing deferrals doesn't map to its original name. - Synthetic names for literal functions differ in format slightly from vanilla Go, which still breaks the test in net/http. The difference is minor enough it's not forth fixing though. --- compiler/functions.go | 12 +++--- compiler/natives/src/net/http/server_test.go | 5 ++- compiler/natives/src/reflect/reflect.go | 33 --------------- compiler/natives/src/reflect/reflect_test.go | 20 ---------- compiler/utils.go | 4 +- tests/gorepo/run.go | 4 +- tests/runtime_test.go | 42 +++++++++++++------- 7 files changed, 39 insertions(+), 81 deletions(-) diff --git a/compiler/functions.go b/compiler/functions.go index f18e12429..6fafed803 100644 --- a/compiler/functions.go +++ b/compiler/functions.go @@ -61,13 +61,11 @@ func (fc *funcContext) nestedFunctionContext(info *analysis.FuncInfo, inst typep // Synthesize an identifier by which the function may reference itself. Since // it appears in the stack trace, it's useful to include the receiver type in // it. - funcRef := o.Name() - if recvType := typesutil.RecvType(sig); recvType != nil { - funcRef = recvType.Obj().Name() + midDot + funcRef - } + funcRef := strings.ReplaceAll(o.Name(), ".", midDot) c.funcRef = sourcemapx.Identifier{ - Name: c.newVariable(funcRef, true /*pkgLevel*/), - OriginalName: o.FullName(), + Name: c.newVariable(funcRef, true /*pkgLevel*/), + // o.FullName() decorates pointer receivers as `(*T).method`, we want simply `T.method`. + OriginalName: strings.NewReplacer("(", "", ")", "", "*", "").Replace(o.FullName()), OriginalPos: o.Pos(), } @@ -269,7 +267,7 @@ func (fc *funcContext) translateFunctionBody(typ *ast.FuncType, recv *ast.Ident, fc.translateStmtList(body.List) if len(fc.Flattened) != 0 && !astutil.EndsWithReturn(body.List) { - fc.translateStmt(&ast.ReturnStmt{}, nil) + fc.translateStmt(&ast.ReturnStmt{Return: body.Rbrace}, nil) } })) diff --git a/compiler/natives/src/net/http/server_test.go b/compiler/natives/src/net/http/server_test.go index f55704dcf..1f988c699 100644 --- a/compiler/natives/src/net/http/server_test.go +++ b/compiler/natives/src/net/http/server_test.go @@ -6,5 +6,8 @@ package http_test import "testing" func TestTimeoutHandlerSuperfluousLogs(t *testing.T) { - t.Skip("https://github.com/gopherjs/gopherjs/issues/1085") + // The test expects nested anonymous functions to be named "Foo.func1.2", + // bug GopherJS generates "Foo.func1.func2". Otherwise the test works as + // expected. + t.Skip("GopherJS uses different synthetic function names.") } diff --git a/compiler/natives/src/reflect/reflect.go b/compiler/natives/src/reflect/reflect.go index 81f4c7b08..87969a6b3 100644 --- a/compiler/natives/src/reflect/reflect.go +++ b/compiler/natives/src/reflect/reflect.go @@ -5,7 +5,6 @@ package reflect import ( "errors" - "runtime" "strconv" "unsafe" @@ -1774,38 +1773,6 @@ func stringsHasPrefix(s, prefix string) bool { return len(s) >= len(prefix) && s[:len(prefix)] == prefix } -func valueMethodName() string { - var pc [5]uintptr - n := runtime.Callers(1, pc[:]) - frames := runtime.CallersFrames(pc[:n]) - valueTyp := TypeOf(Value{}) - var frame runtime.Frame - for more := true; more; { - frame, more = frames.Next() - name := frame.Function - // Function name extracted from the call stack can be different from - // vanilla Go, so is not prefixed by "reflect.Value." as needed by the original. - // See https://cs.opensource.google/go/go/+/refs/tags/go1.19.13:src/reflect/value.go;l=173-191 - // This workaround may become obsolete after - // https://github.com/gopherjs/gopherjs/issues/1085 is resolved. - - methodName := name - if idx := stringsLastIndex(name, '.'); idx >= 0 { - methodName = name[idx+1:] - } - - // Since function name in the call stack doesn't contain receiver name, - // we are looking for the first exported function name that matches a - // known Value method. - if _, ok := valueTyp.MethodByName(methodName); ok { - if len(methodName) > 0 && 'A' <= methodName[0] && methodName[0] <= 'Z' { - return `reflect.Value.` + methodName - } - } - } - return "unknown method" -} - func verifyNotInHeapPtr(p uintptr) bool { // Go runtime uses this method to make sure that a uintptr won't crash GC if // interpreted as a heap pointer. This is not relevant for GopherJS, so we can diff --git a/compiler/natives/src/reflect/reflect_test.go b/compiler/natives/src/reflect/reflect_test.go index 4c0bcd0be..79bbe5385 100644 --- a/compiler/natives/src/reflect/reflect_test.go +++ b/compiler/natives/src/reflect/reflect_test.go @@ -298,23 +298,3 @@ func TestIssue50208(t *testing.T) { func TestStructOfTooLarge(t *testing.T) { t.Skip("This test is dependent on field alignment to determine if a struct size would exceed virtual address space.") } - -func TestSetLenCap(t *testing.T) { - t.Skip("Test depends on call stack function names: https://github.com/gopherjs/gopherjs/issues/1085") -} - -func TestSetPanic(t *testing.T) { - t.Skip("Test depends on call stack function names: https://github.com/gopherjs/gopherjs/issues/1085") -} - -func TestCallPanic(t *testing.T) { - t.Skip("Test depends on call stack function names: https://github.com/gopherjs/gopherjs/issues/1085") -} - -func TestValuePanic(t *testing.T) { - t.Skip("Test depends on call stack function names: https://github.com/gopherjs/gopherjs/issues/1085") -} - -func TestSetIter(t *testing.T) { - t.Skip("Test depends on call stack function names: https://github.com/gopherjs/gopherjs/issues/1085") -} diff --git a/compiler/utils.go b/compiler/utils.go index 4d1ade057..2804c7535 100644 --- a/compiler/utils.go +++ b/compiler/utils.go @@ -359,10 +359,10 @@ func (fc *funcContext) newLitFuncName() string { if fc.instance.Object != nil { if recvType := typesutil.RecvType(fc.sig.Sig); recvType != nil { name.WriteString(recvType.Obj().Name()) - name.WriteString(midDot) + name.WriteString(".") } name.WriteString(fc.instance.Object.Name()) - name.WriteString(midDot) + name.WriteString(".") } fmt.Fprintf(name, "func%d", fc.funcLitCounter) return name.String() diff --git a/tests/gorepo/run.go b/tests/gorepo/run.go index 6720f50d7..1592fcd8b 100644 --- a/tests/gorepo/run.go +++ b/tests/gorepo/run.go @@ -98,7 +98,6 @@ var knownFails = map[string]failReason{ "fixedbugs/issue19246.go": {desc: "expected nil pointer dereference panic"}, // Issue https://golang.org/issues/19246: Failed to evaluate some zero-sized values when converting them to interfaces. // These are new tests in Go 1.10. - "fixedbugs/issue21879.go": {desc: "incorrect output related to runtime.Callers, runtime.CallersFrames, etc."}, "fixedbugs/issue21887.go": {desc: "incorrect output (although within spec, not worth fixing) for println(^uint64(0)). got: { '$high': 4294967295, '$low': 4294967295, '$val': [Circular] } want: 18446744073709551615"}, "fixedbugs/issue22660.go": {category: notApplicable, desc: "test of gc compiler, uses os/exec.Command"}, "fixedbugs/issue23305.go": {desc: "GopherJS fails to compile println(0xffffffff), maybe because 32-bit arch"}, @@ -125,8 +124,7 @@ var knownFails = map[string]failReason{ "fixedbugs/issue24491a.go": {category: notApplicable, desc: "tests interaction between unsafe and GC; uses runtime.SetFinalizer()"}, "fixedbugs/issue24491b.go": {category: notApplicable, desc: "tests interaction between unsafe and GC; uses runtime.SetFinalizer()"}, "fixedbugs/issue29504.go": {category: notApplicable, desc: "requires source map support beyond what GopherJS currently provides"}, - // This test incorrectly passes because main function's name is returned as "main" and not "main.main". Even number of bugs cancel each other out ¯\_(ツ)_/¯ - // "fixedbugs/issue29735.go": {category: usesUnsupportedPackage, desc: "GopherJS only supports runtime.FuncForPC() with position counters previously returned by runtime.Callers() or runtime.Caller()"}, + "fixedbugs/issue29735.go": {category: lowLevelRuntimeDifference, desc: "GopherJS only supports runtime.FuncForPC() with position counters previously returned by runtime.Callers() or runtime.Caller()"}, "fixedbugs/issue30116.go": {desc: "GopherJS doesn't specify the array/slice index selector in the out-of-bounds message"}, "fixedbugs/issue30116u.go": {desc: "GopherJS doesn't specify the array/slice index selector in the out-of-bounds message"}, "fixedbugs/issue34395.go": {category: neverTerminates, desc: "https://github.com/gopherjs/gopherjs/issues/1007"}, diff --git a/tests/runtime_test.go b/tests/runtime_test.go index 12f0b34c3..4f1ba1e9c 100644 --- a/tests/runtime_test.go +++ b/tests/runtime_test.go @@ -101,21 +101,11 @@ func (c *callStack) capture() { } func TestCallers(t *testing.T) { - got := callStack{} - // Some of the GopherJS function names don't match upstream Go, or even the // function names in the Go source when minified. // Until https://github.com/gopherjs/gopherjs/issues/1085 is resolved, the // mismatch is difficult to avoid, but we can at least use "masked" frames to // make sure the number of frames matches expected. - want := callStack{ - masked("runtime.Callers"), - masked("github.com/gopherjs/gopherjs/tests.(*callerNames).capture"), - masked("github.com/gopherjs/gopherjs/tests.TestCallers.func{1,2}"), - masked("testing.tRunner"), - "runtime.goexit", - } - opts := cmp.Comparer(func(a, b funcName) bool { if a == masked("") || b == masked("") { return true @@ -124,6 +114,15 @@ func TestCallers(t *testing.T) { }) t.Run("Normal", func(t *testing.T) { + got := callStack{} + want := callStack{ + "runtime.Callers", + "github.com/gopherjs/gopherjs/tests.callStack.capture", + "github.com/gopherjs/gopherjs/tests.TestCallers.func2", + "testing.tRunner", + "runtime.goexit", + } + got.capture() if diff := cmp.Diff(want, got, opts); diff != "" { t.Errorf("runtime.Callers() returned a diff (-want,+got):\n%s", diff) @@ -131,6 +130,18 @@ func TestCallers(t *testing.T) { }) t.Run("Deferred", func(t *testing.T) { + got := callStack{} + want := callStack{ + "runtime.Callers", + "github.com/gopherjs/gopherjs/tests.callStack.capture", + // For some reason function epilog where deferred calls are invoked doesn't + // get source-mapped to the original source properly, which causes node + // not to map the function name to the original. + masked("github.com/gopherjs/gopherjs/tests.TestCallers.func3"), + "testing.tRunner", + "runtime.goexit", + } + defer func() { if diff := cmp.Diff(want, got, opts); diff != "" { t.Errorf("runtime.Callers() returned a diff (-want,+got):\n%s", diff) @@ -140,17 +151,18 @@ func TestCallers(t *testing.T) { }) t.Run("Recover", func(t *testing.T) { + got := callStack{} defer func() { recover() got.capture() want := callStack{ - masked("runtime.Callers"), - masked("github.com/gopherjs/gopherjs/tests.(*callerNames).capture"), - masked("github.com/gopherjs/gopherjs/tests.TestCallers.func3.1"), + "runtime.Callers", + "github.com/gopherjs/gopherjs/tests.callStack.capture", + "github.com/gopherjs/gopherjs/tests.TestCallers.func4.func1", "runtime.gopanic", - masked("github.com/gopherjs/gopherjs/tests.TestCallers.func{1,2}"), - masked("testing.tRunner"), + "github.com/gopherjs/gopherjs/tests.TestCallers.func4", + "testing.tRunner", "runtime.goexit", } if diff := cmp.Diff(want, got, opts); diff != "" {