From 0da04a662a8e46f688bc65bf4fc5440226babe59 Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Wed, 10 Mar 2021 16:06:47 -0500 Subject: [PATCH 01/12] [release-branch.go1.16] runtime, time: disable preemption in addtimer The timerpMask optimization updates a mask of Ps (potentially) containing timers in pidleget / pidleput. For correctness, it depends on the assumption that new timers can only be added to a P's own heap. addtimer violates this assumption if it is preempted after computing pp. That G may then run on a different P, but adding a timer to the original P's heap. Avoid this by disabling preemption while pp is in use. Other uses of doaddtimer should be OK: * moveTimers: always moves to the current P's heap * modtimer, cleantimers, addAdjustedTimers, runtimer: does not add net new timers to the heap while locked For #44868 Fixes #44869 Change-Id: I4a5d080865e854931d0a3a09a51ca36879101d72 Reviewed-on: https://go-review.googlesource.com/c/go/+/300610 Trust: Michael Pratt Run-TryBot: Michael Pratt Reviewed-by: Michael Knyszek Reviewed-by: Ian Lance Taylor TryBot-Result: Go Bot (cherry picked from commit aa26687e457d825fc9c580e8c029b768e0e70d38) Reviewed-on: https://go-review.googlesource.com/c/go/+/300611 --- src/runtime/time.go | 5 +++++ src/time/sleep_test.go | 16 ++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/src/runtime/time.go b/src/runtime/time.go index 8ab2a03430..dee6a674e4 100644 --- a/src/runtime/time.go +++ b/src/runtime/time.go @@ -263,6 +263,9 @@ func addtimer(t *timer) { when := t.when + // Disable preemption while using pp to avoid changing another P's heap. + mp := acquirem() + pp := getg().m.p.ptr() lock(&pp.timersLock) cleantimers(pp) @@ -270,6 +273,8 @@ func addtimer(t *timer) { unlock(&pp.timersLock) wakeNetPoller(when) + + releasem(mp) } // doaddtimer adds t to the current P's heap. diff --git a/src/time/sleep_test.go b/src/time/sleep_test.go index 084ac33f51..6ee0631a85 100644 --- a/src/time/sleep_test.go +++ b/src/time/sleep_test.go @@ -511,6 +511,22 @@ func TestZeroTimerStopPanics(t *testing.T) { tr.Stop() } +// Test that zero duration timers aren't missed by the scheduler. Regression test for issue 44868. +func TestZeroTimer(t *testing.T) { + if testing.Short() { + t.Skip("-short") + } + + for i := 0; i < 1000000; i++ { + s := Now() + ti := NewTimer(0) + <-ti.C + if diff := Since(s); diff > 2*Second { + t.Errorf("Expected time to get value from Timer channel in less than 2 sec, took %v", diff) + } + } +} + // Benchmark timer latency when the thread that creates the timer is busy with // other work and the timers must be serviced by other threads. // https://golang.org/issue/38860 From f39c4deee812b577ffb84b78e62ce4392d2baeb1 Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Fri, 12 Mar 2021 13:48:32 -0500 Subject: [PATCH 02/12] [release-branch.go1.16] cmd/go: fix godoc formatting for text from 'go help install' Fixes #44860 Change-Id: I5a12c6437a91ce59307483ffcc70e084edc32197 Reviewed-on: https://go-review.googlesource.com/c/go/+/301329 Trust: Jay Conrod Run-TryBot: Jay Conrod Reviewed-by: Bryan C. Mills Reviewed-by: Dmitri Shuralyov TryBot-Result: Go Bot (cherry picked from commit 86bbf4beee276b9a7f9a427a9d1a9277bd904709) Reviewed-on: https://go-review.googlesource.com/c/go/+/301429 --- src/cmd/go/alldocs.go | 20 ++++++++++++-------- src/cmd/go/internal/work/build.go | 20 ++++++++++++-------- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/src/cmd/go/alldocs.go b/src/cmd/go/alldocs.go index e7c63f0749..84f89c9d2d 100644 --- a/src/cmd/go/alldocs.go +++ b/src/cmd/go/alldocs.go @@ -692,18 +692,22 @@ // arguments must satisfy the following constraints: // // - Arguments must be package paths or package patterns (with "..." wildcards). -// They must not be standard packages (like fmt), meta-patterns (std, cmd, -// all), or relative or absolute file paths. +// They must not be standard packages (like fmt), meta-patterns (std, cmd, +// all), or relative or absolute file paths. +// // - All arguments must have the same version suffix. Different queries are not -// allowed, even if they refer to the same version. +// allowed, even if they refer to the same version. +// // - All arguments must refer to packages in the same module at the same version. +// // - No module is considered the "main" module. If the module containing -// packages named on the command line has a go.mod file, it must not contain -// directives (replace and exclude) that would cause it to be interpreted -// differently than if it were the main module. The module must not require -// a higher version of itself. +// packages named on the command line has a go.mod file, it must not contain +// directives (replace and exclude) that would cause it to be interpreted +// differently than if it were the main module. The module must not require +// a higher version of itself. +// // - Package path arguments must refer to main packages. Pattern arguments -// will only match main packages. +// will only match main packages. // // If the arguments don't have version suffixes, "go install" may run in // module-aware mode or GOPATH mode, depending on the GO111MODULE environment diff --git a/src/cmd/go/internal/work/build.go b/src/cmd/go/internal/work/build.go index 780d639c5d..f024b07b22 100644 --- a/src/cmd/go/internal/work/build.go +++ b/src/cmd/go/internal/work/build.go @@ -482,18 +482,22 @@ To eliminate ambiguity about which module versions are used in the build, the arguments must satisfy the following constraints: - Arguments must be package paths or package patterns (with "..." wildcards). - They must not be standard packages (like fmt), meta-patterns (std, cmd, - all), or relative or absolute file paths. +They must not be standard packages (like fmt), meta-patterns (std, cmd, +all), or relative or absolute file paths. + - All arguments must have the same version suffix. Different queries are not - allowed, even if they refer to the same version. +allowed, even if they refer to the same version. + - All arguments must refer to packages in the same module at the same version. + - No module is considered the "main" module. If the module containing - packages named on the command line has a go.mod file, it must not contain - directives (replace and exclude) that would cause it to be interpreted - differently than if it were the main module. The module must not require - a higher version of itself. +packages named on the command line has a go.mod file, it must not contain +directives (replace and exclude) that would cause it to be interpreted +differently than if it were the main module. The module must not require +a higher version of itself. + - Package path arguments must refer to main packages. Pattern arguments - will only match main packages. +will only match main packages. If the arguments don't have version suffixes, "go install" may run in module-aware mode or GOPATH mode, depending on the GO111MODULE environment From 902d16e97bffe3faf49f48e67c43f7a0827f862c Mon Sep 17 00:00:00 2001 From: Tao Qingyun Date: Thu, 18 Mar 2021 00:10:38 +0000 Subject: [PATCH 03/12] [release-branch.go1.16] testing: update helperNames just before checking it parent's helperNames has not been set when frameSkip called, moving helperNames initilazing to frameSkip. For #44887 Fixes #44888 Change-Id: I5107c5951033e5e47d1ac441eac3ba5344a7bdc0 GitHub-Last-Rev: 44b90b2e2eeca8e2bb4a2084ec6fdd279c88f76d GitHub-Pull-Request: golang/go#45071 Reviewed-on: https://go-review.googlesource.com/c/go/+/302469 Trust: Cherry Zhang Reviewed-by: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Go Bot (cherry picked from commit 67048432026062a98a3937a865aeb05a398148c5) Reviewed-on: https://go-review.googlesource.com/c/go/+/303189 Trust: Ian Lance Taylor Reviewed-by: Emmanuel Odeke --- src/testing/helper_test.go | 32 ++++++++++++++++++++++++++++++++ src/testing/testing.go | 15 +++++++-------- 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/src/testing/helper_test.go b/src/testing/helper_test.go index 8858196cf0..b27fd62ee8 100644 --- a/src/testing/helper_test.go +++ b/src/testing/helper_test.go @@ -71,6 +71,38 @@ func TestTBHelperParallel(t *T) { } } +func TestTBHelperLineNumer(t *T) { + var buf bytes.Buffer + ctx := newTestContext(1, newMatcher(regexp.MatchString, "", "")) + t1 := &T{ + common: common{ + signal: make(chan bool), + w: &buf, + }, + context: ctx, + } + t1.Run("Test", func(t *T) { + helperA := func(t *T) { + t.Helper() + t.Run("subtest", func(t *T) { + t.Helper() + t.Fatal("fatal error message") + }) + } + helperA(t) + }) + + want := "helper_test.go:92: fatal error message" + got := "" + lines := strings.Split(strings.TrimSpace(buf.String()), "\n") + if len(lines) > 0 { + got = strings.TrimSpace(lines[len(lines)-1]) + } + if got != want { + t.Errorf("got output:\n\n%v\nwant:\n\n%v", got, want) + } +} + type noopWriter int func (nw *noopWriter) Write(b []byte) (int, error) { return len(b), nil } diff --git a/src/testing/testing.go b/src/testing/testing.go index 80354d5ce8..dec39d24da 100644 --- a/src/testing/testing.go +++ b/src/testing/testing.go @@ -509,6 +509,13 @@ func (c *common) frameSkip(skip int) runtime.Frame { } return prevFrame } + // If more helper PCs have been added since we last did the conversion + if c.helperNames == nil { + c.helperNames = make(map[string]struct{}) + for pc := range c.helperPCs { + c.helperNames[pcToName(pc)] = struct{}{} + } + } if _, ok := c.helperNames[frame.Function]; !ok { // Found a frame that wasn't inside a helper function. return frame @@ -521,14 +528,6 @@ func (c *common) frameSkip(skip int) runtime.Frame { // and inserts the final newline if needed and indentation spaces for formatting. // This function must be called with c.mu held. func (c *common) decorate(s string, skip int) string { - // If more helper PCs have been added since we last did the conversion - if c.helperNames == nil { - c.helperNames = make(map[string]struct{}) - for pc := range c.helperPCs { - c.helperNames[pcToName(pc)] = struct{}{} - } - } - frame := c.frameSkip(skip) file := frame.File line := frame.Line From 33fb47921f65d941b232da2ab542191c9978b4dc Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Wed, 3 Mar 2021 16:30:22 -0500 Subject: [PATCH 04/12] [release-branch.go1.16] cmd/go/internal/modfetch: detect and recover from missing ziphash file Previously, if an extracted module directory existed in the module cache, but the corresponding ziphash file did not, if the sum was missing from go.sum, we would not verify the sum. This caused 'go get' not to write missing sums. 'go build' in readonly mode (now the default) checks for missing sums and doesn't attempt to fetch modules that can't be verified against go.sum. With this change, when requesting the module directory with modfetch.DownloadDir, if the ziphash file is missing, the go command will re-hash the zip without downloading or re-extracting it again. Note that the go command creates the ziphash file before the module directory, but another program could remove it separately, and it might not be present after a crash. Fixes #44812 Change-Id: I64551e048a3ba17d069de1ec123d5b8b2757543c Reviewed-on: https://go-review.googlesource.com/c/go/+/298352 Trust: Jay Conrod Run-TryBot: Jay Conrod TryBot-Result: Go Bot Reviewed-by: Bryan C. Mills (cherry picked from commit 302a400316319501748c0f034464fa70e7815272) Reviewed-on: https://go-review.googlesource.com/c/go/+/298851 --- src/cmd/go/internal/modfetch/cache.go | 17 ++++ src/cmd/go/internal/modfetch/fetch.go | 77 ++++++++++++------- .../script/mod_get_missing_ziphash.txt | 55 +++++++++++++ src/cmd/go/testdata/script/mod_verify.txt | 7 +- 4 files changed, 125 insertions(+), 31 deletions(-) create mode 100644 src/cmd/go/testdata/script/mod_get_missing_ziphash.txt diff --git a/src/cmd/go/internal/modfetch/cache.go b/src/cmd/go/internal/modfetch/cache.go index 3a2ff63721..07e046c8cb 100644 --- a/src/cmd/go/internal/modfetch/cache.go +++ b/src/cmd/go/internal/modfetch/cache.go @@ -84,6 +84,7 @@ func DownloadDir(m module.Version) (string, error) { return "", err } + // Check whether the directory itself exists. dir := filepath.Join(cfg.GOMODCACHE, enc+"@"+encVer) if fi, err := os.Stat(dir); os.IsNotExist(err) { return dir, err @@ -92,6 +93,9 @@ func DownloadDir(m module.Version) (string, error) { } else if !fi.IsDir() { return dir, &DownloadDirPartialError{dir, errors.New("not a directory")} } + + // Check if a .partial file exists. This is created at the beginning of + // a download and removed after the zip is extracted. partialPath, err := CachePath(m, "partial") if err != nil { return dir, err @@ -101,6 +105,19 @@ func DownloadDir(m module.Version) (string, error) { } else if !os.IsNotExist(err) { return dir, err } + + // Check if a .ziphash file exists. It should be created before the + // zip is extracted, but if it was deleted (by another program?), we need + // to re-calculate it. + ziphashPath, err := CachePath(m, "ziphash") + if err != nil { + return dir, err + } + if _, err := os.Stat(ziphashPath); os.IsNotExist(err) { + return dir, &DownloadDirPartialError{dir, errors.New("ziphash file is missing")} + } else if err != nil { + return dir, err + } return dir, nil } diff --git a/src/cmd/go/internal/modfetch/fetch.go b/src/cmd/go/internal/modfetch/fetch.go index c55c3cf253..eb7d30e0ab 100644 --- a/src/cmd/go/internal/modfetch/fetch.go +++ b/src/cmd/go/internal/modfetch/fetch.go @@ -170,13 +170,16 @@ func DownloadZip(ctx context.Context, mod module.Version) (zipfile string, err e if err != nil { return cached{"", err} } + ziphashfile := zipfile + "hash" - // Skip locking if the zipfile already exists. + // Return without locking if the zip and ziphash files exist. if _, err := os.Stat(zipfile); err == nil { - return cached{zipfile, nil} + if _, err := os.Stat(ziphashfile); err == nil { + return cached{zipfile, nil} + } } - // The zip file does not exist. Acquire the lock and create it. + // The zip or ziphash file does not exist. Acquire the lock and create them. if cfg.CmdName != "mod download" { fmt.Fprintf(os.Stderr, "go: downloading %s %s\n", mod.Path, mod.Version) } @@ -186,14 +189,6 @@ func DownloadZip(ctx context.Context, mod module.Version) (zipfile string, err e } defer unlock() - // Double-check that the zipfile was not created while we were waiting for - // the lock. - if _, err := os.Stat(zipfile); err == nil { - return cached{zipfile, nil} - } - if err := os.MkdirAll(filepath.Dir(zipfile), 0777); err != nil { - return cached{"", err} - } if err := downloadZip(ctx, mod, zipfile); err != nil { return cached{"", err} } @@ -206,6 +201,25 @@ func downloadZip(ctx context.Context, mod module.Version, zipfile string) (err e ctx, span := trace.StartSpan(ctx, "modfetch.downloadZip "+zipfile) defer span.Done() + // Double-check that the zipfile was not created while we were waiting for + // the lock in DownloadZip. + ziphashfile := zipfile + "hash" + var zipExists, ziphashExists bool + if _, err := os.Stat(zipfile); err == nil { + zipExists = true + } + if _, err := os.Stat(ziphashfile); err == nil { + ziphashExists = true + } + if zipExists && ziphashExists { + return nil + } + + // Create parent directories. + if err := os.MkdirAll(filepath.Dir(zipfile), 0777); err != nil { + return err + } + // Clean up any remaining tempfiles from previous runs. // This is only safe to do because the lock file ensures that their // writers are no longer active. @@ -217,6 +231,12 @@ func downloadZip(ctx context.Context, mod module.Version, zipfile string) (err e } } + // If the zip file exists, the ziphash file must have been deleted + // or lost after a file system crash. Re-hash the zip without downloading. + if zipExists { + return hashZip(mod, zipfile, ziphashfile) + } + // From here to the os.Rename call below is functionally almost equivalent to // renameio.WriteToFile, with one key difference: we want to validate the // contents of the file (by hashing it) before we commit it. Because the file @@ -289,15 +309,7 @@ func downloadZip(ctx context.Context, mod module.Version, zipfile string) (err e } // Hash the zip file and check the sum before renaming to the final location. - hash, err := dirhash.HashZip(f.Name(), dirhash.DefaultHash) - if err != nil { - return err - } - if err := checkModSum(mod, hash); err != nil { - return err - } - - if err := renameio.WriteFile(zipfile+"hash", []byte(hash), 0666); err != nil { + if err := hashZip(mod, f.Name(), ziphashfile); err != nil { return err } if err := os.Rename(f.Name(), zipfile); err != nil { @@ -309,6 +321,22 @@ func downloadZip(ctx context.Context, mod module.Version, zipfile string) (err e return nil } +// hashZip reads the zip file opened in f, then writes the hash to ziphashfile, +// overwriting that file if it exists. +// +// If the hash does not match go.sum (or the sumdb if enabled), hashZip returns +// an error and does not write ziphashfile. +func hashZip(mod module.Version, zipfile, ziphashfile string) error { + hash, err := dirhash.HashZip(zipfile, dirhash.DefaultHash) + if err != nil { + return err + } + if err := checkModSum(mod, hash); err != nil { + return err + } + return renameio.WriteFile(ziphashfile, []byte(hash), 0666) +} + // makeDirsReadOnly makes a best-effort attempt to remove write permissions for dir // and its transitive contents. func makeDirsReadOnly(dir string) { @@ -452,11 +480,6 @@ func HaveSum(mod module.Version) bool { // checkMod checks the given module's checksum. func checkMod(mod module.Version) { - if cfg.GOMODCACHE == "" { - // Do not use current directory. - return - } - // Do the file I/O before acquiring the go.sum lock. ziphash, err := CachePath(mod, "ziphash") if err != nil { @@ -464,10 +487,6 @@ func checkMod(mod module.Version) { } data, err := renameio.ReadFile(ziphash) if err != nil { - if errors.Is(err, fs.ErrNotExist) { - // This can happen if someone does rm -rf GOPATH/src/cache/download. So it goes. - return - } base.Fatalf("verifying %v", module.VersionError(mod, err)) } h := strings.TrimSpace(string(data)) diff --git a/src/cmd/go/testdata/script/mod_get_missing_ziphash.txt b/src/cmd/go/testdata/script/mod_get_missing_ziphash.txt new file mode 100644 index 0000000000..8f6793edf5 --- /dev/null +++ b/src/cmd/go/testdata/script/mod_get_missing_ziphash.txt @@ -0,0 +1,55 @@ +# Test that if the module cache contains an extracted source directory but not +# a ziphash, 'go build' complains about a missing sum, and 'go get' adds +# the sum. Verifies #44749. + +# With a tidy go.sum, go build succeeds. This also populates the module cache. +cp go.sum.tidy go.sum +go build -n use +env GOPROXY=off +env GOSUMDB=off + +# Control case: if we delete the hash for rsc.io/quote v1.5.2, +# 'go build' reports an error. 'go get' adds the sum. +cp go.sum.bug go.sum +! go build -n use +stderr '^use.go:3:8: missing go.sum entry for module providing package rsc.io/quote \(imported by use\); to add:\n\tgo get use$' +go get -d use +cmp go.sum go.sum.tidy +go build -n use + +# If we delete the hash *and* the ziphash file, we should see the same behavior. +cp go.sum.bug go.sum +rm $WORK/gopath/pkg/mod/cache/download/rsc.io/quote/@v/v1.5.2.ziphash +! go build -n use +stderr '^use.go:3:8: missing go.sum entry for module providing package rsc.io/quote \(imported by use\); to add:\n\tgo get use$' +go get -d use +cmp go.sum go.sum.tidy +go build -n use + +-- go.mod -- +module use + +go 1.17 + +require rsc.io/quote v1.5.2 +-- go.sum.tidy -- +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c h1:pvCbr/wm8HzDD3fVywevekufpn6tCGPY3spdHeZJEsw= +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= +rsc.io/quote v1.5.2 h1:3fEykkD9k7lYzXqCYrwGAf7iNhbk4yCjHmKBN9td4L0= +rsc.io/quote v1.5.2/go.mod h1:LzX7hefJvL54yjefDEDHNONDjII0t9xZLPXsUe+TKr0= +rsc.io/sampler v1.3.0 h1:HLGR/BgEtI3r0uymSP/nl2uPLsUnNJX8toRyhfpBTII= +rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= +rsc.io/testonly v1.0.0 h1:K/VWHdO+Jv7woUXG0GzVNx1czBXUt3Ib1deaMn+xk64= +rsc.io/testonly v1.0.0/go.mod h1:OqmGbIFOcF+XrFReLOGZ6BhMM7uMBiQwZsyNmh74SzY= +-- go.sum.bug -- +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c h1:pvCbr/wm8HzDD3fVywevekufpn6tCGPY3spdHeZJEsw= +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= +rsc.io/quote v1.5.2/go.mod h1:LzX7hefJvL54yjefDEDHNONDjII0t9xZLPXsUe+TKr0= +rsc.io/sampler v1.3.0 h1:HLGR/BgEtI3r0uymSP/nl2uPLsUnNJX8toRyhfpBTII= +rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= +rsc.io/testonly v1.0.0 h1:K/VWHdO+Jv7woUXG0GzVNx1czBXUt3Ib1deaMn+xk64= +rsc.io/testonly v1.0.0/go.mod h1:OqmGbIFOcF+XrFReLOGZ6BhMM7uMBiQwZsyNmh74SzY= +-- use.go -- +package use + +import _ "rsc.io/quote" diff --git a/src/cmd/go/testdata/script/mod_verify.txt b/src/cmd/go/testdata/script/mod_verify.txt index 43812d069f..b5106659a9 100644 --- a/src/cmd/go/testdata/script/mod_verify.txt +++ b/src/cmd/go/testdata/script/mod_verify.txt @@ -48,10 +48,13 @@ go mod tidy grep '^rsc.io/quote v1.1.0/go.mod ' go.sum grep '^rsc.io/quote v1.1.0 ' go.sum -# sync should ignore missing ziphash; verify should not +# verify should fail on a missing ziphash. tidy should restore it. rm $GOPATH/pkg/mod/cache/download/rsc.io/quote/@v/v1.1.0.ziphash -go mod tidy ! go mod verify +stderr '^rsc.io/quote v1.1.0: missing ziphash: open '$GOPATH'[/\\]pkg[/\\]mod[/\\]cache[/\\]download[/\\]rsc.io[/\\]quote[/\\]@v[/\\]v1.1.0.ziphash' +go mod tidy +exists $GOPATH/pkg/mod/cache/download/rsc.io/quote/@v/v1.1.0.ziphash +go mod verify # Packages below module root should not be mentioned in go.sum. rm go.sum From ac59d7abb9ab5ec31cb0fe1eaccbdf3b17edbde0 Mon Sep 17 00:00:00 2001 From: Cherry Zhang Date: Fri, 12 Mar 2021 23:21:09 -0500 Subject: [PATCH 05/12] [release-branch.go1.16] cmd/compile, cmd/link: dynamically export writable static tmps Static tmps are private to a package, but with plugins a package can be shared among multiple DSOs. They need to have a consistent view of the static tmps, especially for writable ones. So export them. (Read-only static tmps have the same values anyway, so it doesn't matter. Also Mach-O doesn't support dynamically exporting read-only symbols anyway.) Updates #44956. Fixes #45030. Change-Id: I921e25b7ab73cd5d5347800eccdb7931e3448779 Reviewed-on: https://go-review.googlesource.com/c/go/+/301793 Trust: Cherry Zhang Run-TryBot: Cherry Zhang TryBot-Result: Go Bot Reviewed-by: Than McIntosh (cherry picked from commit de012bc095359e1b552d4ea6fb6b2995f3ab04f5) Reviewed-on: https://go-review.googlesource.com/c/go/+/302449 --- misc/cgo/testplugin/plugin_test.go | 7 +++ .../testdata/issue44956/base/base.go | 7 +++ .../testplugin/testdata/issue44956/main.go | 47 +++++++++++++++++++ .../testplugin/testdata/issue44956/plugin1.go | 9 ++++ .../testplugin/testdata/issue44956/plugin2.go | 11 +++++ src/cmd/compile/internal/gc/sinit.go | 4 +- src/cmd/link/internal/ld/symtab.go | 13 +++-- 7 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 misc/cgo/testplugin/testdata/issue44956/base/base.go create mode 100644 misc/cgo/testplugin/testdata/issue44956/main.go create mode 100644 misc/cgo/testplugin/testdata/issue44956/plugin1.go create mode 100644 misc/cgo/testplugin/testdata/issue44956/plugin2.go diff --git a/misc/cgo/testplugin/plugin_test.go b/misc/cgo/testplugin/plugin_test.go index 2d991012c8..8869528015 100644 --- a/misc/cgo/testplugin/plugin_test.go +++ b/misc/cgo/testplugin/plugin_test.go @@ -209,3 +209,10 @@ func TestMethod2(t *testing.T) { goCmd(t, "build", "-o", "method2.exe", "./method2/main.go") run(t, "./method2.exe") } + +func TestIssue44956(t *testing.T) { + goCmd(t, "build", "-buildmode=plugin", "-o", "issue44956p1.so", "./issue44956/plugin1.go") + goCmd(t, "build", "-buildmode=plugin", "-o", "issue44956p2.so", "./issue44956/plugin2.go") + goCmd(t, "build", "-o", "issue44956.exe", "./issue44956/main.go") + run(t, "./issue44956.exe") +} diff --git a/misc/cgo/testplugin/testdata/issue44956/base/base.go b/misc/cgo/testplugin/testdata/issue44956/base/base.go new file mode 100644 index 0000000000..609aa0dff4 --- /dev/null +++ b/misc/cgo/testplugin/testdata/issue44956/base/base.go @@ -0,0 +1,7 @@ +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package base + +var X = &map[int]int{123: 456} diff --git a/misc/cgo/testplugin/testdata/issue44956/main.go b/misc/cgo/testplugin/testdata/issue44956/main.go new file mode 100644 index 0000000000..287a60585e --- /dev/null +++ b/misc/cgo/testplugin/testdata/issue44956/main.go @@ -0,0 +1,47 @@ +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Issue 44956: writable static temp is not exported correctly. +// In the test below, package base is +// +// X = &map{...} +// +// which compiles to +// +// X = &stmp // static +// stmp = makemap(...) // in init function +// +// plugin1 and plugin2 both import base. plugin1 doesn't use +// base.X, so that symbol is deadcoded in plugin1. +// +// plugin1 is loaded first. base.init runs at that point, which +// initialize base.stmp. +// +// plugin2 is then loaded. base.init already ran, so it doesn't run +// again. When base.stmp is not exported, plugin2's base.X points to +// its own private base.stmp, which is not initialized, fail. + +package main + +import "plugin" + +func main() { + _, err := plugin.Open("issue44956p1.so") + if err != nil { + panic("FAIL") + } + + p2, err := plugin.Open("issue44956p2.so") + if err != nil { + panic("FAIL") + } + f, err := p2.Lookup("F") + if err != nil { + panic("FAIL") + } + x := f.(func() *map[int]int)() + if x == nil || (*x)[123] != 456 { + panic("FAIL") + } +} diff --git a/misc/cgo/testplugin/testdata/issue44956/plugin1.go b/misc/cgo/testplugin/testdata/issue44956/plugin1.go new file mode 100644 index 0000000000..499fa31abf --- /dev/null +++ b/misc/cgo/testplugin/testdata/issue44956/plugin1.go @@ -0,0 +1,9 @@ +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import _ "testplugin/issue44956/base" + +func main() {} diff --git a/misc/cgo/testplugin/testdata/issue44956/plugin2.go b/misc/cgo/testplugin/testdata/issue44956/plugin2.go new file mode 100644 index 0000000000..a73542ca71 --- /dev/null +++ b/misc/cgo/testplugin/testdata/issue44956/plugin2.go @@ -0,0 +1,11 @@ +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import "testplugin/issue44956/base" + +func F() *map[int]int { return base.X } + +func main() {} diff --git a/src/cmd/compile/internal/gc/sinit.go b/src/cmd/compile/internal/gc/sinit.go index 212fcc022d..4d0837bc74 100644 --- a/src/cmd/compile/internal/gc/sinit.go +++ b/src/cmd/compile/internal/gc/sinit.go @@ -363,15 +363,15 @@ func staticname(t *types.Type) *Node { n := newname(lookup(fmt.Sprintf("%s%d", obj.StaticNamePref, statuniqgen))) statuniqgen++ addvar(n, t, PEXTERN) - n.Sym.Linksym().Set(obj.AttrLocal, true) return n } -// readonlystaticname returns a name backed by a (writable) static data symbol. +// readonlystaticname returns a name backed by a read-only static data symbol. func readonlystaticname(t *types.Type) *Node { n := staticname(t) n.MarkReadonly() n.Sym.Linksym().Set(obj.AttrContentAddressable, true) + n.Sym.Linksym().Set(obj.AttrLocal, true) return n } diff --git a/src/cmd/link/internal/ld/symtab.go b/src/cmd/link/internal/ld/symtab.go index c98e4de03f..f54cf9ea2f 100644 --- a/src/cmd/link/internal/ld/symtab.go +++ b/src/cmd/link/internal/ld/symtab.go @@ -31,6 +31,7 @@ package ld import ( + "cmd/internal/obj" "cmd/internal/objabi" "cmd/link/internal/loader" "cmd/link/internal/sym" @@ -102,10 +103,14 @@ func putelfsym(ctxt *Link, x loader.Sym, typ elf.SymType, curbind elf.SymBind) { elfshnum = xosect.Elfsect.(*ElfShdr).shnum } + sname := ldr.SymExtname(x) + // One pass for each binding: elf.STB_LOCAL, elf.STB_GLOBAL, // maybe one day elf.STB_WEAK. bind := elf.STB_GLOBAL - if ldr.IsFileLocal(x) || ldr.AttrVisibilityHidden(x) || ldr.AttrLocal(x) { + if ldr.IsFileLocal(x) && !isStaticTmp(sname) || ldr.AttrVisibilityHidden(x) || ldr.AttrLocal(x) { + // Static tmp is package local, but a package can be shared among multiple DSOs. + // They need to have a single view of the static tmp that are writable. bind = elf.STB_LOCAL } @@ -140,8 +145,6 @@ func putelfsym(ctxt *Link, x loader.Sym, typ elf.SymType, curbind elf.SymBind) { other |= 3 << 5 } - sname := ldr.SymExtname(x) - // When dynamically linking, we create Symbols by reading the names from // the symbol tables of the shared libraries and so the names need to // match exactly. Tools like DTrace will have to wait for now. @@ -823,3 +826,7 @@ func setCarrierSize(typ sym.SymKind, sz int64) { } CarrierSymByType[typ].Size = sz } + +func isStaticTmp(name string) bool { + return strings.Contains(name, "."+obj.StaticNamePref) +} From 9c7463ca903863a0c67b3f76454b37da0a400c13 Mon Sep 17 00:00:00 2001 From: Cherry Zhang Date: Tue, 16 Feb 2021 10:20:58 -0500 Subject: [PATCH 06/12] [release-branch.go1.16] cmd/link: generate trampoline for inter-dependent packages Currently, in the trampoline generation pass we expect packages are laid out in dependency order, so a cross-package jump always has a known target address so we can check if a trampoline is needed. With linknames, there can be cycles in the package dependency graph, making this algorithm no longer work. For them, as the target address is unkown we conservatively generate a trampoline. This may generate unnecessary trampolines (if the packages turn out laid together), but package cycles are extremely rare so this is fine. Updates #44639. Fixes #44640. Change-Id: I2dc2998edacbda27d726fc79452313a21d07787a Reviewed-on: https://go-review.googlesource.com/c/go/+/292490 Trust: Cherry Zhang Reviewed-by: Than McIntosh (cherry picked from commit 098504c73ff6ece19566a1ac811ceed73be7c81d) Reviewed-on: https://go-review.googlesource.com/c/go/+/296909 Run-TryBot: Cherry Zhang TryBot-Result: Go Bot --- src/cmd/link/internal/arm/asm.go | 16 +++++++++++----- src/cmd/link/internal/ld/data.go | 12 +++++------- src/cmd/link/internal/ppc64/asm.go | 12 +++++++++--- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/cmd/link/internal/arm/asm.go b/src/cmd/link/internal/arm/asm.go index 755b472694..03caeae7be 100644 --- a/src/cmd/link/internal/arm/asm.go +++ b/src/cmd/link/internal/arm/asm.go @@ -370,10 +370,16 @@ func trampoline(ctxt *ld.Link, ldr *loader.Loader, ri int, rs, s loader.Sym) { r := relocs.At(ri) switch r.Type() { case objabi.R_CALLARM: - // r.Add is the instruction - // low 24-bit encodes the target address - t := (ldr.SymValue(rs) + int64(signext24(r.Add()&0xffffff)*4) - (ldr.SymValue(s) + int64(r.Off()))) / 4 - if t > 0x7fffff || t < -0x800000 || (*ld.FlagDebugTramp > 1 && ldr.SymPkg(s) != ldr.SymPkg(rs)) { + var t int64 + // ldr.SymValue(rs) == 0 indicates a cross-package jump to a function that is not yet + // laid out. Conservatively use a trampoline. This should be rare, as we lay out packages + // in dependency order. + if ldr.SymValue(rs) != 0 { + // r.Add is the instruction + // low 24-bit encodes the target address + t = (ldr.SymValue(rs) + int64(signext24(r.Add()&0xffffff)*4) - (ldr.SymValue(s) + int64(r.Off()))) / 4 + } + if t > 0x7fffff || t < -0x800000 || ldr.SymValue(rs) == 0 || (*ld.FlagDebugTramp > 1 && ldr.SymPkg(s) != ldr.SymPkg(rs)) { // direct call too far, need to insert trampoline. // look up existing trampolines first. if we found one within the range // of direct call, we can reuse it. otherwise create a new one. @@ -445,7 +451,7 @@ func gentramp(arch *sys.Arch, linkmode ld.LinkMode, ldr *loader.Loader, tramp *l arch.ByteOrder.PutUint32(P[8:], o3) tramp.SetData(P) - if linkmode == ld.LinkExternal { + if linkmode == ld.LinkExternal || ldr.SymValue(target) == 0 { r, _ := tramp.AddRel(objabi.R_ADDR) r.SetOff(8) r.SetSiz(4) diff --git a/src/cmd/link/internal/ld/data.go b/src/cmd/link/internal/ld/data.go index 6013e0ab0a..52035e9630 100644 --- a/src/cmd/link/internal/ld/data.go +++ b/src/cmd/link/internal/ld/data.go @@ -106,14 +106,12 @@ func trampoline(ctxt *Link, s loader.Sym) { } rs = ldr.ResolveABIAlias(rs) if ldr.SymValue(rs) == 0 && (ldr.SymType(rs) != sym.SDYNIMPORT && ldr.SymType(rs) != sym.SUNDEFEXT) { - if ldr.SymPkg(rs) != ldr.SymPkg(s) { - if !isRuntimeDepPkg(ldr.SymPkg(s)) || !isRuntimeDepPkg(ldr.SymPkg(rs)) { - ctxt.Errorf(s, "unresolved inter-package jump to %s(%s) from %s", ldr.SymName(rs), ldr.SymPkg(rs), ldr.SymPkg(s)) - } - // runtime and its dependent packages may call to each other. - // they are fine, as they will be laid down together. + if ldr.SymPkg(rs) == ldr.SymPkg(s) { + continue // symbols in the same package are laid out together + } + if isRuntimeDepPkg(ldr.SymPkg(s)) && isRuntimeDepPkg(ldr.SymPkg(rs)) { + continue // runtime packages are laid out together } - continue } thearch.Trampoline(ctxt, ldr, ri, rs, s) diff --git a/src/cmd/link/internal/ppc64/asm.go b/src/cmd/link/internal/ppc64/asm.go index 5bf3898eb9..602f0b5299 100644 --- a/src/cmd/link/internal/ppc64/asm.go +++ b/src/cmd/link/internal/ppc64/asm.go @@ -656,13 +656,19 @@ func trampoline(ctxt *ld.Link, ldr *loader.Loader, ri int, rs, s loader.Sym) { relocs := ldr.Relocs(s) r := relocs.At(ri) - t := ldr.SymValue(rs) + r.Add() - (ldr.SymValue(s) + int64(r.Off())) + var t int64 + // ldr.SymValue(rs) == 0 indicates a cross-package jump to a function that is not yet + // laid out. Conservatively use a trampoline. This should be rare, as we lay out packages + // in dependency order. + if ldr.SymValue(rs) != 0 { + t = ldr.SymValue(rs) + r.Add() - (ldr.SymValue(s) + int64(r.Off())) + } switch r.Type() { case objabi.R_CALLPOWER: // If branch offset is too far then create a trampoline. - if (ctxt.IsExternal() && ldr.SymSect(s) != ldr.SymSect(rs)) || (ctxt.IsInternal() && int64(int32(t<<6)>>6) != t) || (*ld.FlagDebugTramp > 1 && ldr.SymPkg(s) != ldr.SymPkg(rs)) { + if (ctxt.IsExternal() && ldr.SymSect(s) != ldr.SymSect(rs)) || (ctxt.IsInternal() && int64(int32(t<<6)>>6) != t) || ldr.SymValue(rs) == 0 || (*ld.FlagDebugTramp > 1 && ldr.SymPkg(s) != ldr.SymPkg(rs)) { var tramp loader.Sym for i := 0; ; i++ { @@ -749,7 +755,7 @@ func gentramp(ctxt *ld.Link, ldr *loader.Loader, tramp *loader.SymbolBuilder, ta // With external linking, the target address must be // relocated using LO and HA - if ctxt.IsExternal() { + if ctxt.IsExternal() || ldr.SymValue(target) == 0 { r, _ := tramp.AddRel(objabi.R_ADDRPOWER) r.SetOff(0) r.SetSiz(8) // generates 2 relocations: HA + LO From 1d967ab95c43b6b8810ca53d6a18aff85e59a3b6 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Tue, 26 Jan 2021 21:14:43 -0500 Subject: [PATCH 07/12] [release-branch.go1.16] build: set GOPATH consistently in run.bash, run.bat, run.rc We used to clear GOPATH in all the build scripts. Clearing GOPATH is misleading at best, since you just end up with the default GOPATH (%USERPROFILE%\go on Windows). Unless that's your GOROOT, in which case you end up with a fatal error from the go command (#43938). run.bash changed to setting GOPATH=/dev/null, which has no clear analogue on Windows. run.rc still clears GOPATH. Change them all to set GOPATH to a non-existent directory /nonexist-gopath or c:\nonexist-gopath. For #45238. Fixes #45240. Change-Id: I51edd66d37ff6a891b0d0541d91ecba97fbbb03d Reviewed-on: https://go-review.googlesource.com/c/go/+/288818 Trust: Russ Cox Trust: Jason A. Donenfeld Reviewed-by: Cherry Zhang Reviewed-by: Alex Brainman Reviewed-by: Jason A. Donenfeld (cherry picked from commit bb6efb96092cc8ae398c29e3b052a0051c746f88) Reviewed-on: https://go-review.googlesource.com/c/go/+/304772 Run-TryBot: Dmitri Shuralyov TryBot-Result: Go Bot Reviewed-by: Alexander Rakoczy Reviewed-by: Carlos Amedee Trust: Carlos Amedee --- src/run.bash | 10 +--------- src/run.bat | 4 +--- src/run.rc | 9 ++++----- 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/src/run.bash b/src/run.bash index 706b4b60ee..2123c509f8 100755 --- a/src/run.bash +++ b/src/run.bash @@ -23,15 +23,7 @@ fi eval $(../bin/go env) export GOROOT # The api test requires GOROOT to be set, so set it to match ../bin/go. - -# We disallow local import for non-local packages, if $GOROOT happens -# to be under $GOPATH, then some tests below will fail. $GOPATH needs -# to be set to a non-empty string, else Go will set a default value -# that may also conflict with $GOROOT. The $GOPATH value doesn't need -# to point to an actual directory, it just needs to pass the semantic -# checks performed by Go. Use $GOROOT to define $GOPATH so that we -# don't blunder into a user-defined symbolic link. -export GOPATH=/dev/null +export GOPATH=/nonexist-gopath unset CDPATH # in case user has it set export GOBIN=$GOROOT/bin # Issue 14340 diff --git a/src/run.bat b/src/run.bat index c299671c13..edcaf52659 100644 --- a/src/run.bat +++ b/src/run.bat @@ -18,9 +18,7 @@ setlocal set GOBUILDFAIL=0 -:: we disallow local import for non-local packages, if %GOROOT% happens -:: to be under %GOPATH%, then some tests below will fail -set GOPATH= +set GOPATH=c:\nonexist-gopath :: Issue 14340: ignore GOBIN during all.bat. set GOBIN= set GOFLAGS= diff --git a/src/run.rc b/src/run.rc index ab7abfa991..a7b4801207 100755 --- a/src/run.rc +++ b/src/run.rc @@ -12,10 +12,9 @@ if(! test -f ../bin/go){ eval `{../bin/go env} -GOPATH = () # we disallow local import for non-local packages, if $GOROOT happens - # to be under $GOPATH, then some tests below will fail -GOBIN = () # Issue 14340 -GOFLAGS = () -GO111MODULE = () +GOPATH=/nonexist-gopath +GOBIN=() # Issue 14340 +GOFLAGS=() +GO111MODULE=() exec ../bin/go tool dist test -rebuild $* From 2940614c63ed1351c47038db4ec613a37d1e72fd Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Tue, 9 Mar 2021 17:23:04 -0500 Subject: [PATCH 08/12] [release-branch.go1.16] cmd/go: allow '+' in package import paths in module mode This change upgrades x/mod to pull in the fix from CL 300152. Updates #44776. Fixes #44885. Change-Id: I273f41df2abfff76d91315b7f19fce851c8770d8 Reviewed-on: https://go-review.googlesource.com/c/go/+/300176 Trust: Bryan C. Mills Run-TryBot: Bryan C. Mills Reviewed-by: Jay Conrod TryBot-Result: Go Bot (cherry picked from commit d33e2192a71c33a604af247161ba1d2c1969e4c7) Reviewed-on: https://go-review.googlesource.com/c/go/+/300153 --- src/cmd/go.mod | 2 +- src/cmd/go.sum | 4 +- .../testdata/script/mod_invalid_path_plus.txt | 32 ++++++++++++++++ .../vendor/golang.org/x/mod/module/module.go | 38 +++++++++++++++---- src/cmd/vendor/modules.txt | 2 +- 5 files changed, 66 insertions(+), 12 deletions(-) create mode 100644 src/cmd/go/testdata/script/mod_invalid_path_plus.txt diff --git a/src/cmd/go.mod b/src/cmd/go.mod index 35582f3975..70b1b0690b 100644 --- a/src/cmd/go.mod +++ b/src/cmd/go.mod @@ -6,7 +6,7 @@ require ( github.com/google/pprof v0.0.0-20201203190320-1bf35d6f28c2 golang.org/x/arch v0.0.0-20201008161808-52c3e6f60cff golang.org/x/crypto v0.0.0-20201016220609-9e8e0b390897 - golang.org/x/mod v0.4.2-0.20210302225053-d515b24adc21 + golang.org/x/mod v0.4.2-0.20210325185522-dbbbf8a3c6ea golang.org/x/sys v0.0.0-20201204225414-ed752295db88 // indirect golang.org/x/tools v0.0.0-20210107193943-4ed967dd8eff ) diff --git a/src/cmd/go.sum b/src/cmd/go.sum index 10340d441f..edda18b526 100644 --- a/src/cmd/go.sum +++ b/src/cmd/go.sum @@ -14,8 +14,8 @@ golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPh golang.org/x/crypto v0.0.0-20201016220609-9e8e0b390897 h1:pLI5jrR7OSLijeIDcmRxNmw2api+jEfxLoykJVice/E= golang.org/x/crypto v0.0.0-20201016220609-9e8e0b390897/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= -golang.org/x/mod v0.4.2-0.20210302225053-d515b24adc21 h1:FnWKa8BJXkVQ+16E52jkfBOJPx2cG8y/6X376nOgSM4= -golang.org/x/mod v0.4.2-0.20210302225053-d515b24adc21/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= +golang.org/x/mod v0.4.2-0.20210325185522-dbbbf8a3c6ea h1:zAn46O7Vmm6KdLXx+635hPZSArrt/wNctv4Ab70Jw3k= +golang.org/x/mod v0.4.2-0.20210325185522-dbbbf8a3c6ea/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU= diff --git a/src/cmd/go/testdata/script/mod_invalid_path_plus.txt b/src/cmd/go/testdata/script/mod_invalid_path_plus.txt new file mode 100644 index 0000000000..2f2488f01b --- /dev/null +++ b/src/cmd/go/testdata/script/mod_invalid_path_plus.txt @@ -0,0 +1,32 @@ +# https://golang.org/issue/44776 +# The '+' character should be disallowed in module paths, but allowed in package +# paths within valid modules. + +go get -d example.net/cmd +go list example.net/cmd/x++ + +! go list -versions -m 'example.net/bad++' +stderr '^go list -m: module example.net/bad\+\+: malformed module path "example.net/bad\+\+": invalid char ''\+''$' + +# TODO(bcmills): 'go get -d example.net/cmd/x++' should also work, but currently +# it does not. This might be fixed by https://golang.org/cl/297891. +! go get -d example.net/cmd/x++ +stderr '^go get: malformed module path "example.net/cmd/x\+\+": invalid char ''\+''$' + +-- go.mod -- +module example.com/m + +go 1.16 + +replace ( + example.net/cmd => ./cmd +) + +-- cmd/go.mod -- +module example.net/cmd + +go 1.16 +-- cmd/x++/main.go -- +package main + +func main() {} diff --git a/src/cmd/vendor/golang.org/x/mod/module/module.go b/src/cmd/vendor/golang.org/x/mod/module/module.go index 272baeef17..0e03014837 100644 --- a/src/cmd/vendor/golang.org/x/mod/module/module.go +++ b/src/cmd/vendor/golang.org/x/mod/module/module.go @@ -224,12 +224,16 @@ func firstPathOK(r rune) bool { 'a' <= r && r <= 'z' } -// pathOK reports whether r can appear in an import path element. +// modPathOK reports whether r can appear in a module path element. // Paths can be ASCII letters, ASCII digits, and limited ASCII punctuation: - . _ and ~. -// This matches what "go get" has historically recognized in import paths. +// +// This matches what "go get" has historically recognized in import paths, +// and avoids confusing sequences like '%20' or '+' that would change meaning +// if used in a URL. +// // TODO(rsc): We would like to allow Unicode letters, but that requires additional // care in the safe encoding (see "escaped paths" above). -func pathOK(r rune) bool { +func modPathOK(r rune) bool { if r < utf8.RuneSelf { return r == '-' || r == '.' || r == '_' || r == '~' || '0' <= r && r <= '9' || @@ -239,6 +243,17 @@ func pathOK(r rune) bool { return false } +// modPathOK reports whether r can appear in a package import path element. +// +// Import paths are intermediate between module paths and file paths: we allow +// disallow characters that would be confusing or ambiguous as arguments to +// 'go get' (such as '@' and ' ' ), but allow certain characters that are +// otherwise-unambiguous on the command line and historically used for some +// binary names (such as '++' as a suffix for compiler binaries and wrappers). +func importPathOK(r rune) bool { + return modPathOK(r) || r == '+' +} + // fileNameOK reports whether r can appear in a file name. // For now we allow all Unicode letters but otherwise limit to pathOK plus a few more punctuation characters. // If we expand the set of allowed characters here, we have to @@ -394,12 +409,19 @@ func checkElem(elem string, kind pathKind) error { if elem[len(elem)-1] == '.' { return fmt.Errorf("trailing dot in path element") } - charOK := pathOK - if kind == filePath { - charOK = fileNameOK - } for _, r := range elem { - if !charOK(r) { + ok := false + switch kind { + case modulePath: + ok = modPathOK(r) + case importPath: + ok = importPathOK(r) + case filePath: + ok = fileNameOK(r) + default: + panic(fmt.Sprintf("internal error: invalid kind %v", kind)) + } + if !ok { return fmt.Errorf("invalid char %q", r) } } diff --git a/src/cmd/vendor/modules.txt b/src/cmd/vendor/modules.txt index 10842768a8..ae2c0a00cc 100644 --- a/src/cmd/vendor/modules.txt +++ b/src/cmd/vendor/modules.txt @@ -28,7 +28,7 @@ golang.org/x/arch/x86/x86asm golang.org/x/crypto/ed25519 golang.org/x/crypto/ed25519/internal/edwards25519 golang.org/x/crypto/ssh/terminal -# golang.org/x/mod v0.4.2-0.20210302225053-d515b24adc21 +# golang.org/x/mod v0.4.2-0.20210325185522-dbbbf8a3c6ea ## explicit golang.org/x/mod/internal/lazyregexp golang.org/x/mod/modfile From 3a45c13094e2bf1ac1430ce6af8b8be80bf0938e Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Tue, 23 Mar 2021 14:48:47 -0700 Subject: [PATCH 09/12] [release-branch.go1.16] cmd/compile: disable shortcircuit optimization for intertwined phi values We need to be careful that when doing value graph surgery, we not re-substitute a value that has already been substituted. That can lead to confusing a previous iteration's value with the current iteration's value. The simple fix in this CL just aborts the optimization if it detects intertwined phis (a phi which is the argument to another phi). It might be possible to keep the optimization with a more complicated CL, but: 1) This CL is clearly safe to backport. 2) There were no instances of this abort triggering in all.bash, prior to the test introduced in this CL. Fixes #45192 Change-Id: I2411dca03948653c053291f6829a76bec0c32330 Reviewed-on: https://go-review.googlesource.com/c/go/+/304251 Trust: Keith Randall Trust: Josh Bleecher Snyder Run-TryBot: Keith Randall TryBot-Result: Go Bot Reviewed-by: Josh Bleecher Snyder (cherry picked from commit 771c57e68ed5ef2bbb0eafc0d48419f59d143932) Reviewed-on: https://go-review.googlesource.com/c/go/+/304530 --- src/cmd/compile/internal/ssa/shortcircuit.go | 18 ++++++++++++ test/fixedbugs/issue45175.go | 29 ++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 test/fixedbugs/issue45175.go diff --git a/src/cmd/compile/internal/ssa/shortcircuit.go b/src/cmd/compile/internal/ssa/shortcircuit.go index 7b4ee2e81c..4dd86ec74f 100644 --- a/src/cmd/compile/internal/ssa/shortcircuit.go +++ b/src/cmd/compile/internal/ssa/shortcircuit.go @@ -138,6 +138,24 @@ func shortcircuitBlock(b *Block) bool { if len(b.Values) != nval+nOtherPhi { return false } + if nOtherPhi > 0 { + // Check for any phi which is the argument of another phi. + // These cases are tricky, as substitutions done by replaceUses + // are no longer trivial to do in any ordering. See issue 45175. + m := make(map[*Value]bool, 1+nOtherPhi) + for _, v := range b.Values { + if v.Op == OpPhi { + m[v] = true + } + } + for v := range m { + for _, a := range v.Args { + if a != v && m[a] { + return false + } + } + } + } // Locate index of first const phi arg. cidx := -1 diff --git a/test/fixedbugs/issue45175.go b/test/fixedbugs/issue45175.go new file mode 100644 index 0000000000..02dfe8a0a9 --- /dev/null +++ b/test/fixedbugs/issue45175.go @@ -0,0 +1,29 @@ +// run + +// Copyright 2021 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +//go:noinline +func f(c bool) int { + b := true + x := 0 + y := 1 + for b { + b = false + y = x + x = 2 + if c { + return 3 + } + } + return y +} + +func main() { + if got := f(false); got != 0 { + panic(got) + } +} From 887c0d890fd33a72cc1de6316252ba37649145cb Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Thu, 11 Mar 2021 12:28:45 -0500 Subject: [PATCH 10/12] [release-branch.go1.16] runtime: non-strict InlTreeIndex lookup in Frames.Next When using cgo, some of the frames can be provided by cgoTraceback, a cgo-provided function to generate C tracebacks. Unlike Go tracebacks, cgoTraceback has no particular guarantees that it produces valid tracebacks. If one of the (invalid) frames happens to put the PC in the alignment region at the end of a function (filled with int 3's on amd64), then Frames.Next will find a valid funcInfo for the PC, but pcdatavalue will panic because PCDATA doesn't cover this PC. Tolerate this case by doing a non-strict PCDATA lookup. We'll still show a bogus frame, but at least avoid throwing. For #44971 Fixes #45303 Change-Id: I9eed728470d6f264179a7615bd19845c941db78c Reviewed-on: https://go-review.googlesource.com/c/go/+/301369 Trust: Michael Pratt Run-TryBot: Michael Pratt TryBot-Result: Go Bot Reviewed-by: Cherry Zhang (cherry picked from commit e4a4161f1f3157550846e1b6bd4fe83aae15778e) Reviewed-on: https://go-review.googlesource.com/c/go/+/305889 --- src/runtime/symtab.go | 4 +- src/runtime/symtab_test.go | 85 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/src/runtime/symtab.go b/src/runtime/symtab.go index 7667f23f1d..09225fb03a 100644 --- a/src/runtime/symtab.go +++ b/src/runtime/symtab.go @@ -102,7 +102,9 @@ func (ci *Frames) Next() (frame Frame, more bool) { name := funcname(funcInfo) if inldata := funcdata(funcInfo, _FUNCDATA_InlTree); inldata != nil { inltree := (*[1 << 20]inlinedCall)(inldata) - ix := pcdatavalue(funcInfo, _PCDATA_InlTreeIndex, pc, nil) + // Non-strict as cgoTraceback may have added bogus PCs + // with a valid funcInfo but invalid PCDATA. + ix := pcdatavalue1(funcInfo, _PCDATA_InlTreeIndex, pc, nil, false) if ix >= 0 { // Note: entry is not modified. It always refers to a real frame, not an inlined one. f = nil diff --git a/src/runtime/symtab_test.go b/src/runtime/symtab_test.go index 01e5002659..ffa07c7f3a 100644 --- a/src/runtime/symtab_test.go +++ b/src/runtime/symtab_test.go @@ -8,6 +8,7 @@ import ( "runtime" "strings" "testing" + "unsafe" ) func TestCaller(t *testing.T) { @@ -165,3 +166,87 @@ func TestNilName(t *testing.T) { t.Errorf("Name() = %q, want %q", got, "") } } + +var dummy int + +func inlined() { + // Side effect to prevent elimination of this entire function. + dummy = 42 +} + +// A function with an InlTree. Returns a PC within the function body. +// +// No inline to ensure this complete function appears in output. +// +//go:noinline +func tracebackFunc(t *testing.T) uintptr { + // This body must be more complex than a single call to inlined to get + // an inline tree. + inlined() + inlined() + + // Acquire a PC in this function. + pc, _, _, ok := runtime.Caller(0) + if !ok { + t.Fatalf("Caller(0) got ok false, want true") + } + + return pc +} + +// Test that CallersFrames handles PCs in the alignment region between +// functions (int 3 on amd64) without crashing. +// +// Go will never generate a stack trace containing such an address, as it is +// not a valid call site. However, the cgo traceback function passed to +// runtime.SetCgoTraceback may not be completely accurate and may incorrect +// provide PCs in Go code or the alignement region between functions. +// +// Go obviously doesn't easily expose the problematic PCs to running programs, +// so this test is a bit fragile. Some details: +// +// * tracebackFunc is our target function. We want to get a PC in the +// alignment region following this function. This function also has other +// functions inlined into it to ensure it has an InlTree (this was the source +// of the bug in issue 44971). +// +// * We acquire a PC in tracebackFunc, walking forwards until FuncForPC says +// we're in a new function. The last PC of the function according to FuncForPC +// should be in the alignment region (assuming the function isn't already +// perfectly aligned). +// +// This is a regression test for issue 44971. +func TestFunctionAlignmentTraceback(t *testing.T) { + pc := tracebackFunc(t) + + // Double-check we got the right PC. + f := runtime.FuncForPC(pc) + if !strings.HasSuffix(f.Name(), "tracebackFunc") { + t.Fatalf("Caller(0) = %+v, want tracebackFunc", f) + } + + // Iterate forward until we find a different function. Back up one + // instruction is (hopefully) an alignment instruction. + for runtime.FuncForPC(pc) == f { + pc++ + } + pc-- + + // Is this an alignment region filler instruction? We only check this + // on amd64 for simplicity. If this function has no filler, then we may + // get a false negative, but will never get a false positive. + if runtime.GOARCH == "amd64" { + code := *(*uint8)(unsafe.Pointer(pc)) + if code != 0xcc { // INT $3 + t.Errorf("PC %v code got %#x want 0xcc", pc, code) + } + } + + // Finally ensure that Frames.Next doesn't crash when processing this + // PC. + frames := runtime.CallersFrames([]uintptr{pc}) + frame, _ := frames.Next() + if frame.Func != f { + t.Errorf("frames.Next() got %+v want %+v", frame.Func, f) + } +} From 96139f25993f3d8122e27a6fec877a4d4f69f83b Mon Sep 17 00:00:00 2001 From: Pat Gavlin Date: Fri, 26 Mar 2021 14:46:37 +0000 Subject: [PATCH 11/12] [release-branch.go1.16] cmd/compile: fix long RMW bit operations on AMD64 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Under certain circumstances, the existing rules for bit operations can produce code that writes beyond its intended bounds. For example, consider the following code: func repro(b []byte, addr, bit int32) { _ = b[3] v := uint32(b[0]) | uint32(b[1])<<8 | uint32(b[2])<<16 | uint32(b[3])<<24 | 1<<(bit&31) b[0] = byte(v) b[1] = byte(v >> 8) b[2] = byte(v >> 16) b[3] = byte(v >> 24) } Roughly speaking: 1. The expression `1 << (bit & 31)` is rewritten into `(SHLL 1 bit)` 2. The expression `uint32(b[0]) | uint32(b[1])<<8 | uint32(b[2])<<16 | uint32(b[3])<<24` is rewritten into `(MOVLload &b[0])` 3. The statements `b[0] = byte(v) ... b[3] = byte(v >> 24)` are rewritten into `(MOVLstore &b[0], v)` 4. `(ORL (SHLL 1, bit) (MOVLload &b[0]))` is rewritten into `(BTSL (MOVLload &b[0]) bit)`. This is a valid transformation because the destination is a register: in this case, the bit offset is masked by the number of bits in the destination register. This is identical to the masking performed by `SHL`. 5. `(MOVLstore &b[0] (BTSL (MOVLload &b[0]) bit))` is rewritten into `(BTSLmodify &b[0] bit)`. This is an invalid transformation because the destination is memory: in this case, the bit offset is not masked, and the chosen instruction may write outside its intended 32-bit location. These changes fix the invalid rewrite performed in step (5) by explicitly maksing the bit offset operand to `BT(S|R|C)(L|Q)modify`. In the example above, the adjusted rules produce `(BTSLmodify &b[0] (ANDLconst [31] bit))` in step (5). These changes also add several new rules to rewrite bit sets, toggles, and clears that are rooted at `(OR|XOR|AND)(L|Q)modify` operators into appropriate `BT(S|R|C)(L|Q)modify` operators. These rules catch cases where `MOV(L|Q)store ((OR|XOR|AND)(L|Q) ...)` is rewritten to `(OR|XOR|AND)(L|Q)modify` before the `(OR|XOR|AND)(L|Q) ...` can be rewritten to `BT(S|R|C)(L|Q) ...`. Overall, compilecmp reports small improvements in code size on darwin/amd64 when the changes to the compiler itself are exlcuded: file before after Δ % runtime.s 536464 536412 -52 -0.010% bytes.s 32629 32593 -36 -0.110% strings.s 44565 44529 -36 -0.081% os/signal.s 7967 7959 -8 -0.100% cmd/vendor/golang.org/x/sys/unix.s 81686 81678 -8 -0.010% math/big.s 188235 188253 +18 +0.010% cmd/link/internal/loader.s 89295 89056 -239 -0.268% cmd/link/internal/ld.s 633551 633232 -319 -0.050% cmd/link/internal/arm.s 18934 18928 -6 -0.032% cmd/link/internal/arm64.s 31814 31801 -13 -0.041% cmd/link/internal/riscv64.s 7347 7345 -2 -0.027% cmd/compile/internal/ssa.s 4029173 4033066 +3893 +0.097% total 21298280 21301472 +3192 +0.015% Fixes #45253 Change-Id: I2e560548b515865129e1724e150e30540e9d29ce GitHub-Last-Rev: ab94ede1d097f920a9d1d3da403c8e4a3d8f6d44 GitHub-Pull-Request: golang/go#45242 Reviewed-on: https://go-review.googlesource.com/c/go/+/305069 Trust: Emmanuel Odeke Run-TryBot: Emmanuel Odeke TryBot-Result: Go Bot Reviewed-by: Keith Randall --- src/cmd/compile/internal/ssa/gen/AMD64.rules | 24 +- src/cmd/compile/internal/ssa/gen/AMD64Ops.go | 5 + src/cmd/compile/internal/ssa/rewriteAMD64.go | 229 +++++++++++++++++-- test/codegen/bits.go | 10 +- 4 files changed, 244 insertions(+), 24 deletions(-) diff --git a/src/cmd/compile/internal/ssa/gen/AMD64.rules b/src/cmd/compile/internal/ssa/gen/AMD64.rules index a866a967b9..5de1e1ec31 100644 --- a/src/cmd/compile/internal/ssa/gen/AMD64.rules +++ b/src/cmd/compile/internal/ssa/gen/AMD64.rules @@ -623,6 +623,14 @@ // Recognize bit setting (a |= 1< (BTS(Q|L) x y) (XOR(Q|L) (SHL(Q|L) (MOV(Q|L)const [1]) y) x) => (BTC(Q|L) x y) +(ORLmodify [off] {sym} ptr s:(SHLL (MOVLconst [1]) x) mem) => + (BTSLmodify [off] {sym} ptr (ANDLconst [31] x) mem) +(ORQmodify [off] {sym} ptr s:(SHLQ (MOVQconst [1]) x) mem) => + (BTSQmodify [off] {sym} ptr (ANDQconst [63] x) mem) +(XORLmodify [off] {sym} ptr s:(SHLL (MOVLconst [1]) x) mem) => + (BTCLmodify [off] {sym} ptr (ANDLconst [31] x) mem) +(XORQmodify [off] {sym} ptr s:(SHLQ (MOVQconst [1]) x) mem) => + (BTCQmodify [off] {sym} ptr (ANDQconst [63] x) mem) // Convert ORconst into BTS, if the code gets smaller, with boundary being // (ORL $40,AX is 3 bytes, ORL $80,AX is 6 bytes). @@ -645,6 +653,10 @@ => (BTRQconst [int8(log64(^c))] x) (ANDL (MOVLconst [c]) x) && isUint32PowerOfTwo(int64(^c)) && uint64(^c) >= 128 => (BTRLconst [int8(log32(^c))] x) +(ANDLmodify [off] {sym} ptr (NOTL s:(SHLL (MOVLconst [1]) x)) mem) => + (BTRLmodify [off] {sym} ptr (ANDLconst [31] x) mem) +(ANDQmodify [off] {sym} ptr (NOTQ s:(SHLQ (MOVQconst [1]) x)) mem) => + (BTRQmodify [off] {sym} ptr (ANDQconst [63] x) mem) // Special-case bit patterns on first/last bit. // generic.rules changes ANDs of high-part/low-part masks into a couple of shifts, @@ -2050,11 +2062,15 @@ ((ADD|SUB|MUL|DIV)SD x l:(MOVSDload [off] {sym} ptr mem)) && canMergeLoadClobber(v, l, x) && clobber(l) => ((ADD|SUB|MUL|DIV)SDload x [off] {sym} ptr mem) ((ADD|SUB|MUL|DIV)SS x l:(MOVSSload [off] {sym} ptr mem)) && canMergeLoadClobber(v, l, x) && clobber(l) => ((ADD|SUB|MUL|DIV)SSload x [off] {sym} ptr mem) (MOVLstore {sym} [off] ptr y:((ADD|AND|OR|XOR)Lload x [off] {sym} ptr mem) mem) && y.Uses==1 && clobber(y) => ((ADD|AND|OR|XOR)Lmodify [off] {sym} ptr x mem) -(MOVLstore {sym} [off] ptr y:((ADD|SUB|AND|OR|XOR|BTC|BTR|BTS)L l:(MOVLload [off] {sym} ptr mem) x) mem) && y.Uses==1 && l.Uses==1 && clobber(y, l) => - ((ADD|SUB|AND|OR|XOR|BTC|BTR|BTS)Lmodify [off] {sym} ptr x mem) +(MOVLstore {sym} [off] ptr y:((ADD|SUB|AND|OR|XOR)L l:(MOVLload [off] {sym} ptr mem) x) mem) && y.Uses==1 && l.Uses==1 && clobber(y, l) => + ((ADD|SUB|AND|OR|XOR)Lmodify [off] {sym} ptr x mem) +(MOVLstore {sym} [off] ptr y:((BTC|BTR|BTS)L l:(MOVLload [off] {sym} ptr mem) x) mem) && y.Uses==1 && l.Uses==1 && clobber(y, l) => + ((BTC|BTR|BTS)Lmodify [off] {sym} ptr (ANDLconst [31] x) mem) (MOVQstore {sym} [off] ptr y:((ADD|AND|OR|XOR)Qload x [off] {sym} ptr mem) mem) && y.Uses==1 && clobber(y) => ((ADD|AND|OR|XOR)Qmodify [off] {sym} ptr x mem) -(MOVQstore {sym} [off] ptr y:((ADD|SUB|AND|OR|XOR|BTC|BTR|BTS)Q l:(MOVQload [off] {sym} ptr mem) x) mem) && y.Uses==1 && l.Uses==1 && clobber(y, l) => - ((ADD|SUB|AND|OR|XOR|BTC|BTR|BTS)Qmodify [off] {sym} ptr x mem) +(MOVQstore {sym} [off] ptr y:((ADD|SUB|AND|OR|XOR)Q l:(MOVQload [off] {sym} ptr mem) x) mem) && y.Uses==1 && l.Uses==1 && clobber(y, l) => + ((ADD|SUB|AND|OR|XOR)Qmodify [off] {sym} ptr x mem) +(MOVQstore {sym} [off] ptr y:((BTC|BTR|BTS)Q l:(MOVQload [off] {sym} ptr mem) x) mem) && y.Uses==1 && l.Uses==1 && clobber(y, l) => + ((BTC|BTR|BTS)Qmodify [off] {sym} ptr (ANDQconst [63] x) mem) // Merge ADDQconst and LEAQ into atomic loads. (MOV(Q|L|B)atomicload [off1] {sym} (ADDQconst [off2] ptr) mem) && is32Bit(int64(off1)+int64(off2)) => diff --git a/src/cmd/compile/internal/ssa/gen/AMD64Ops.go b/src/cmd/compile/internal/ssa/gen/AMD64Ops.go index de5372670b..a87581b68f 100644 --- a/src/cmd/compile/internal/ssa/gen/AMD64Ops.go +++ b/src/cmd/compile/internal/ssa/gen/AMD64Ops.go @@ -358,6 +358,11 @@ func init() { {name: "BTSQconst", argLength: 1, reg: gp11, asm: "BTSQ", resultInArg0: true, clobberFlags: true, aux: "Int8"}, // set bit auxint in arg0, 0 <= auxint < 64 // direct bit operation on memory operand + // + // Note that these operations do not mask the bit offset (arg1), and will write beyond their expected + // bounds if that argument is larger than 64/32 (for BT*Q and BT*L, respectively). If the compiler + // cannot prove that arg1 is in range, it must be explicitly masked (see e.g. the patterns that produce + // BT*modify from (MOVstore (BT* (MOVLload ptr mem) x) mem)). {name: "BTCQmodify", argLength: 3, reg: gpstore, asm: "BTCQ", aux: "SymOff", typ: "Mem", clobberFlags: true, faultOnNilArg0: true, symEffect: "Read,Write"}, // complement bit arg1 in 64-bit arg0+auxint+aux, arg2=mem {name: "BTCLmodify", argLength: 3, reg: gpstore, asm: "BTCL", aux: "SymOff", typ: "Mem", clobberFlags: true, faultOnNilArg0: true, symEffect: "Read,Write"}, // complement bit arg1 in 32-bit arg0+auxint+aux, arg2=mem {name: "BTSQmodify", argLength: 3, reg: gpstore, asm: "BTSQ", aux: "SymOff", typ: "Mem", clobberFlags: true, faultOnNilArg0: true, symEffect: "Read,Write"}, // set bit arg1 in 64-bit arg0+auxint+aux, arg2=mem diff --git a/src/cmd/compile/internal/ssa/rewriteAMD64.go b/src/cmd/compile/internal/ssa/rewriteAMD64.go index 75d4ff7357..8272a406d9 100644 --- a/src/cmd/compile/internal/ssa/rewriteAMD64.go +++ b/src/cmd/compile/internal/ssa/rewriteAMD64.go @@ -2998,6 +2998,36 @@ func rewriteValueAMD64_OpAMD64ANDLmodify(v *Value) bool { v_2 := v.Args[2] v_1 := v.Args[1] v_0 := v.Args[0] + b := v.Block + // match: (ANDLmodify [off] {sym} ptr (NOTL s:(SHLL (MOVLconst [1]) x)) mem) + // result: (BTRLmodify [off] {sym} ptr (ANDLconst [31] x) mem) + for { + off := auxIntToInt32(v.AuxInt) + sym := auxToSym(v.Aux) + ptr := v_0 + if v_1.Op != OpAMD64NOTL { + break + } + s := v_1.Args[0] + if s.Op != OpAMD64SHLL { + break + } + t := s.Type + x := s.Args[1] + s_0 := s.Args[0] + if s_0.Op != OpAMD64MOVLconst || auxIntToInt32(s_0.AuxInt) != 1 { + break + } + mem := v_2 + v.reset(OpAMD64BTRLmodify) + v.AuxInt = int32ToAuxInt(off) + v.Aux = symToAux(sym) + v0 := b.NewValue0(v.Pos, OpAMD64ANDLconst, t) + v0.AuxInt = int32ToAuxInt(31) + v0.AddArg(x) + v.AddArg3(ptr, v0, mem) + return true + } // match: (ANDLmodify [off1] {sym} (ADDQconst [off2] base) val mem) // cond: is32Bit(int64(off1)+int64(off2)) // result: (ANDLmodify [off1+off2] {sym} base val mem) @@ -3377,6 +3407,36 @@ func rewriteValueAMD64_OpAMD64ANDQmodify(v *Value) bool { v_2 := v.Args[2] v_1 := v.Args[1] v_0 := v.Args[0] + b := v.Block + // match: (ANDQmodify [off] {sym} ptr (NOTQ s:(SHLQ (MOVQconst [1]) x)) mem) + // result: (BTRQmodify [off] {sym} ptr (ANDQconst [63] x) mem) + for { + off := auxIntToInt32(v.AuxInt) + sym := auxToSym(v.Aux) + ptr := v_0 + if v_1.Op != OpAMD64NOTQ { + break + } + s := v_1.Args[0] + if s.Op != OpAMD64SHLQ { + break + } + t := s.Type + x := s.Args[1] + s_0 := s.Args[0] + if s_0.Op != OpAMD64MOVQconst || auxIntToInt64(s_0.AuxInt) != 1 { + break + } + mem := v_2 + v.reset(OpAMD64BTRQmodify) + v.AuxInt = int32ToAuxInt(off) + v.Aux = symToAux(sym) + v0 := b.NewValue0(v.Pos, OpAMD64ANDQconst, t) + v0.AuxInt = int32ToAuxInt(63) + v0.AddArg(x) + v.AddArg3(ptr, v0, mem) + return true + } // match: (ANDQmodify [off1] {sym} (ADDQconst [off2] base) val mem) // cond: is32Bit(int64(off1)+int64(off2)) // result: (ANDQmodify [off1+off2] {sym} base val mem) @@ -12709,9 +12769,9 @@ func rewriteValueAMD64_OpAMD64MOVLstore(v *Value) bool { } break } - // match: (MOVLstore {sym} [off] ptr y:(BTCL l:(MOVLload [off] {sym} ptr mem) x) mem) + // match: (MOVLstore {sym} [off] ptr y:(BTCL l:(MOVLload [off] {sym} ptr mem) x) mem) // cond: y.Uses==1 && l.Uses==1 && clobber(y, l) - // result: (BTCLmodify [off] {sym} ptr x mem) + // result: (BTCLmodify [off] {sym} ptr (ANDLconst [31] x) mem) for { off := auxIntToInt32(v.AuxInt) sym := auxToSym(v.Aux) @@ -12720,6 +12780,7 @@ func rewriteValueAMD64_OpAMD64MOVLstore(v *Value) bool { if y.Op != OpAMD64BTCL { break } + t := y.Type x := y.Args[1] l := y.Args[0] if l.Op != OpAMD64MOVLload || auxIntToInt32(l.AuxInt) != off || auxToSym(l.Aux) != sym { @@ -12732,12 +12793,15 @@ func rewriteValueAMD64_OpAMD64MOVLstore(v *Value) bool { v.reset(OpAMD64BTCLmodify) v.AuxInt = int32ToAuxInt(off) v.Aux = symToAux(sym) - v.AddArg3(ptr, x, mem) + v0 := b.NewValue0(l.Pos, OpAMD64ANDLconst, t) + v0.AuxInt = int32ToAuxInt(31) + v0.AddArg(x) + v.AddArg3(ptr, v0, mem) return true } - // match: (MOVLstore {sym} [off] ptr y:(BTRL l:(MOVLload [off] {sym} ptr mem) x) mem) + // match: (MOVLstore {sym} [off] ptr y:(BTRL l:(MOVLload [off] {sym} ptr mem) x) mem) // cond: y.Uses==1 && l.Uses==1 && clobber(y, l) - // result: (BTRLmodify [off] {sym} ptr x mem) + // result: (BTRLmodify [off] {sym} ptr (ANDLconst [31] x) mem) for { off := auxIntToInt32(v.AuxInt) sym := auxToSym(v.Aux) @@ -12746,6 +12810,7 @@ func rewriteValueAMD64_OpAMD64MOVLstore(v *Value) bool { if y.Op != OpAMD64BTRL { break } + t := y.Type x := y.Args[1] l := y.Args[0] if l.Op != OpAMD64MOVLload || auxIntToInt32(l.AuxInt) != off || auxToSym(l.Aux) != sym { @@ -12758,12 +12823,15 @@ func rewriteValueAMD64_OpAMD64MOVLstore(v *Value) bool { v.reset(OpAMD64BTRLmodify) v.AuxInt = int32ToAuxInt(off) v.Aux = symToAux(sym) - v.AddArg3(ptr, x, mem) + v0 := b.NewValue0(l.Pos, OpAMD64ANDLconst, t) + v0.AuxInt = int32ToAuxInt(31) + v0.AddArg(x) + v.AddArg3(ptr, v0, mem) return true } - // match: (MOVLstore {sym} [off] ptr y:(BTSL l:(MOVLload [off] {sym} ptr mem) x) mem) + // match: (MOVLstore {sym} [off] ptr y:(BTSL l:(MOVLload [off] {sym} ptr mem) x) mem) // cond: y.Uses==1 && l.Uses==1 && clobber(y, l) - // result: (BTSLmodify [off] {sym} ptr x mem) + // result: (BTSLmodify [off] {sym} ptr (ANDLconst [31] x) mem) for { off := auxIntToInt32(v.AuxInt) sym := auxToSym(v.Aux) @@ -12772,6 +12840,7 @@ func rewriteValueAMD64_OpAMD64MOVLstore(v *Value) bool { if y.Op != OpAMD64BTSL { break } + t := y.Type x := y.Args[1] l := y.Args[0] if l.Op != OpAMD64MOVLload || auxIntToInt32(l.AuxInt) != off || auxToSym(l.Aux) != sym { @@ -12784,7 +12853,10 @@ func rewriteValueAMD64_OpAMD64MOVLstore(v *Value) bool { v.reset(OpAMD64BTSLmodify) v.AuxInt = int32ToAuxInt(off) v.Aux = symToAux(sym) - v.AddArg3(ptr, x, mem) + v0 := b.NewValue0(l.Pos, OpAMD64ANDLconst, t) + v0.AuxInt = int32ToAuxInt(31) + v0.AddArg(x) + v.AddArg3(ptr, v0, mem) return true } // match: (MOVLstore [off] {sym} ptr a:(ADDLconst [c] l:(MOVLload [off] {sym} ptr2 mem)) mem) @@ -13525,6 +13597,7 @@ func rewriteValueAMD64_OpAMD64MOVQstore(v *Value) bool { v_2 := v.Args[2] v_1 := v.Args[1] v_0 := v.Args[0] + b := v.Block // match: (MOVQstore [off1] {sym} (ADDQconst [off2] ptr) val mem) // cond: is32Bit(int64(off1)+int64(off2)) // result: (MOVQstore [off1+off2] {sym} ptr val mem) @@ -13890,9 +13963,9 @@ func rewriteValueAMD64_OpAMD64MOVQstore(v *Value) bool { } break } - // match: (MOVQstore {sym} [off] ptr y:(BTCQ l:(MOVQload [off] {sym} ptr mem) x) mem) + // match: (MOVQstore {sym} [off] ptr y:(BTCQ l:(MOVQload [off] {sym} ptr mem) x) mem) // cond: y.Uses==1 && l.Uses==1 && clobber(y, l) - // result: (BTCQmodify [off] {sym} ptr x mem) + // result: (BTCQmodify [off] {sym} ptr (ANDQconst [63] x) mem) for { off := auxIntToInt32(v.AuxInt) sym := auxToSym(v.Aux) @@ -13901,6 +13974,7 @@ func rewriteValueAMD64_OpAMD64MOVQstore(v *Value) bool { if y.Op != OpAMD64BTCQ { break } + t := y.Type x := y.Args[1] l := y.Args[0] if l.Op != OpAMD64MOVQload || auxIntToInt32(l.AuxInt) != off || auxToSym(l.Aux) != sym { @@ -13913,12 +13987,15 @@ func rewriteValueAMD64_OpAMD64MOVQstore(v *Value) bool { v.reset(OpAMD64BTCQmodify) v.AuxInt = int32ToAuxInt(off) v.Aux = symToAux(sym) - v.AddArg3(ptr, x, mem) + v0 := b.NewValue0(l.Pos, OpAMD64ANDQconst, t) + v0.AuxInt = int32ToAuxInt(63) + v0.AddArg(x) + v.AddArg3(ptr, v0, mem) return true } - // match: (MOVQstore {sym} [off] ptr y:(BTRQ l:(MOVQload [off] {sym} ptr mem) x) mem) + // match: (MOVQstore {sym} [off] ptr y:(BTRQ l:(MOVQload [off] {sym} ptr mem) x) mem) // cond: y.Uses==1 && l.Uses==1 && clobber(y, l) - // result: (BTRQmodify [off] {sym} ptr x mem) + // result: (BTRQmodify [off] {sym} ptr (ANDQconst [63] x) mem) for { off := auxIntToInt32(v.AuxInt) sym := auxToSym(v.Aux) @@ -13927,6 +14004,7 @@ func rewriteValueAMD64_OpAMD64MOVQstore(v *Value) bool { if y.Op != OpAMD64BTRQ { break } + t := y.Type x := y.Args[1] l := y.Args[0] if l.Op != OpAMD64MOVQload || auxIntToInt32(l.AuxInt) != off || auxToSym(l.Aux) != sym { @@ -13939,12 +14017,15 @@ func rewriteValueAMD64_OpAMD64MOVQstore(v *Value) bool { v.reset(OpAMD64BTRQmodify) v.AuxInt = int32ToAuxInt(off) v.Aux = symToAux(sym) - v.AddArg3(ptr, x, mem) + v0 := b.NewValue0(l.Pos, OpAMD64ANDQconst, t) + v0.AuxInt = int32ToAuxInt(63) + v0.AddArg(x) + v.AddArg3(ptr, v0, mem) return true } - // match: (MOVQstore {sym} [off] ptr y:(BTSQ l:(MOVQload [off] {sym} ptr mem) x) mem) + // match: (MOVQstore {sym} [off] ptr y:(BTSQ l:(MOVQload [off] {sym} ptr mem) x) mem) // cond: y.Uses==1 && l.Uses==1 && clobber(y, l) - // result: (BTSQmodify [off] {sym} ptr x mem) + // result: (BTSQmodify [off] {sym} ptr (ANDQconst [63] x) mem) for { off := auxIntToInt32(v.AuxInt) sym := auxToSym(v.Aux) @@ -13953,6 +14034,7 @@ func rewriteValueAMD64_OpAMD64MOVQstore(v *Value) bool { if y.Op != OpAMD64BTSQ { break } + t := y.Type x := y.Args[1] l := y.Args[0] if l.Op != OpAMD64MOVQload || auxIntToInt32(l.AuxInt) != off || auxToSym(l.Aux) != sym { @@ -13965,7 +14047,10 @@ func rewriteValueAMD64_OpAMD64MOVQstore(v *Value) bool { v.reset(OpAMD64BTSQmodify) v.AuxInt = int32ToAuxInt(off) v.Aux = symToAux(sym) - v.AddArg3(ptr, x, mem) + v0 := b.NewValue0(l.Pos, OpAMD64ANDQconst, t) + v0.AuxInt = int32ToAuxInt(63) + v0.AddArg(x) + v.AddArg3(ptr, v0, mem) return true } // match: (MOVQstore [off] {sym} ptr a:(ADDQconst [c] l:(MOVQload [off] {sym} ptr2 mem)) mem) @@ -18352,6 +18437,33 @@ func rewriteValueAMD64_OpAMD64ORLmodify(v *Value) bool { v_2 := v.Args[2] v_1 := v.Args[1] v_0 := v.Args[0] + b := v.Block + // match: (ORLmodify [off] {sym} ptr s:(SHLL (MOVLconst [1]) x) mem) + // result: (BTSLmodify [off] {sym} ptr (ANDLconst [31] x) mem) + for { + off := auxIntToInt32(v.AuxInt) + sym := auxToSym(v.Aux) + ptr := v_0 + s := v_1 + if s.Op != OpAMD64SHLL { + break + } + t := s.Type + x := s.Args[1] + s_0 := s.Args[0] + if s_0.Op != OpAMD64MOVLconst || auxIntToInt32(s_0.AuxInt) != 1 { + break + } + mem := v_2 + v.reset(OpAMD64BTSLmodify) + v.AuxInt = int32ToAuxInt(off) + v.Aux = symToAux(sym) + v0 := b.NewValue0(v.Pos, OpAMD64ANDLconst, t) + v0.AuxInt = int32ToAuxInt(31) + v0.AddArg(x) + v.AddArg3(ptr, v0, mem) + return true + } // match: (ORLmodify [off1] {sym} (ADDQconst [off2] base) val mem) // cond: is32Bit(int64(off1)+int64(off2)) // result: (ORLmodify [off1+off2] {sym} base val mem) @@ -19979,6 +20091,33 @@ func rewriteValueAMD64_OpAMD64ORQmodify(v *Value) bool { v_2 := v.Args[2] v_1 := v.Args[1] v_0 := v.Args[0] + b := v.Block + // match: (ORQmodify [off] {sym} ptr s:(SHLQ (MOVQconst [1]) x) mem) + // result: (BTSQmodify [off] {sym} ptr (ANDQconst [63] x) mem) + for { + off := auxIntToInt32(v.AuxInt) + sym := auxToSym(v.Aux) + ptr := v_0 + s := v_1 + if s.Op != OpAMD64SHLQ { + break + } + t := s.Type + x := s.Args[1] + s_0 := s.Args[0] + if s_0.Op != OpAMD64MOVQconst || auxIntToInt64(s_0.AuxInt) != 1 { + break + } + mem := v_2 + v.reset(OpAMD64BTSQmodify) + v.AuxInt = int32ToAuxInt(off) + v.Aux = symToAux(sym) + v0 := b.NewValue0(v.Pos, OpAMD64ANDQconst, t) + v0.AuxInt = int32ToAuxInt(63) + v0.AddArg(x) + v.AddArg3(ptr, v0, mem) + return true + } // match: (ORQmodify [off1] {sym} (ADDQconst [off2] base) val mem) // cond: is32Bit(int64(off1)+int64(off2)) // result: (ORQmodify [off1+off2] {sym} base val mem) @@ -28014,6 +28153,33 @@ func rewriteValueAMD64_OpAMD64XORLmodify(v *Value) bool { v_2 := v.Args[2] v_1 := v.Args[1] v_0 := v.Args[0] + b := v.Block + // match: (XORLmodify [off] {sym} ptr s:(SHLL (MOVLconst [1]) x) mem) + // result: (BTCLmodify [off] {sym} ptr (ANDLconst [31] x) mem) + for { + off := auxIntToInt32(v.AuxInt) + sym := auxToSym(v.Aux) + ptr := v_0 + s := v_1 + if s.Op != OpAMD64SHLL { + break + } + t := s.Type + x := s.Args[1] + s_0 := s.Args[0] + if s_0.Op != OpAMD64MOVLconst || auxIntToInt32(s_0.AuxInt) != 1 { + break + } + mem := v_2 + v.reset(OpAMD64BTCLmodify) + v.AuxInt = int32ToAuxInt(off) + v.Aux = symToAux(sym) + v0 := b.NewValue0(v.Pos, OpAMD64ANDLconst, t) + v0.AuxInt = int32ToAuxInt(31) + v0.AddArg(x) + v.AddArg3(ptr, v0, mem) + return true + } // match: (XORLmodify [off1] {sym} (ADDQconst [off2] base) val mem) // cond: is32Bit(int64(off1)+int64(off2)) // result: (XORLmodify [off1+off2] {sym} base val mem) @@ -28382,6 +28548,33 @@ func rewriteValueAMD64_OpAMD64XORQmodify(v *Value) bool { v_2 := v.Args[2] v_1 := v.Args[1] v_0 := v.Args[0] + b := v.Block + // match: (XORQmodify [off] {sym} ptr s:(SHLQ (MOVQconst [1]) x) mem) + // result: (BTCQmodify [off] {sym} ptr (ANDQconst [63] x) mem) + for { + off := auxIntToInt32(v.AuxInt) + sym := auxToSym(v.Aux) + ptr := v_0 + s := v_1 + if s.Op != OpAMD64SHLQ { + break + } + t := s.Type + x := s.Args[1] + s_0 := s.Args[0] + if s_0.Op != OpAMD64MOVQconst || auxIntToInt64(s_0.AuxInt) != 1 { + break + } + mem := v_2 + v.reset(OpAMD64BTCQmodify) + v.AuxInt = int32ToAuxInt(off) + v.Aux = symToAux(sym) + v0 := b.NewValue0(v.Pos, OpAMD64ANDQconst, t) + v0.AuxInt = int32ToAuxInt(63) + v0.AddArg(x) + v.AddArg3(ptr, v0, mem) + return true + } // match: (XORQmodify [off1] {sym} (ADDQconst [off2] base) val mem) // cond: is32Bit(int64(off1)+int64(off2)) // result: (XORQmodify [off1+off2] {sym} base val mem) diff --git a/test/codegen/bits.go b/test/codegen/bits.go index 4508eba487..6f7502292a 100644 --- a/test/codegen/bits.go +++ b/test/codegen/bits.go @@ -262,8 +262,8 @@ func bitcompl32(a, b uint32) (n uint32) { return n } -// check direct operation on memory with constant source -func bitOpOnMem(a []uint32) { +// check direct operation on memory with constant and shifted constant sources +func bitOpOnMem(a []uint32, b, c, d uint32) { // amd64:`ANDL\s[$]200,\s\([A-Z]+\)` a[0] &= 200 // amd64:`ORL\s[$]220,\s4\([A-Z]+\)` @@ -276,6 +276,12 @@ func bitOpOnMem(a []uint32) { a[4] |= 0x4000 // amd64:`BTCL\s[$]13,\s20\([A-Z]+\)`,-`XORL` a[5] ^= 0x2000 + // amd64:`BTRL\s[A-Z]+,\s24\([A-Z]+\)` + a[6] &^= 1 << (b & 31) + // amd64:`BTSL\s[A-Z]+,\s28\([A-Z]+\)` + a[7] |= 1 << (c & 31) + // amd64:`BTCL\s[A-Z]+,\s32\([A-Z]+\)` + a[8] ^= 1 << (d & 31) } func bitcheckMostNegative(b uint8) bool { From 9baddd3f21230c55f0ad2a10f5f20579dcf0a0bb Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Thu, 1 Apr 2021 11:28:29 -0400 Subject: [PATCH 12/12] [release-branch.go1.16] go1.16.3 Change-Id: Iace7cfec757a6d0bac25f729be5ecb2ae3b59d74 Reviewed-on: https://go-review.googlesource.com/c/go/+/306569 Run-TryBot: Dmitri Shuralyov Run-TryBot: Carlos Amedee Reviewed-by: Dmitri Shuralyov Reviewed-by: David Chase TryBot-Result: Go Bot --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index d47e8e7081..ea60ca03a0 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -go1.16.2 \ No newline at end of file +go1.16.3 \ No newline at end of file