From 499458f7ca04087958987a33c2703c3ef03e27e2 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Wed, 28 Jun 2023 13:20:08 -0700 Subject: net/http: validate Host header before sending Verify that the Host header we send is valid. Avoids surprising behavior such as a Host of "go.dev\r\nX-Evil:oops" adding an X-Evil header to HTTP/1 requests. Add a test, skip the test for HTTP/2. HTTP/2 is not vulnerable to header injection in the way HTTP/1 is, but x/net/http2 doesn't validate the header and will go into a retry loop when the server rejects it. CL 506995 adds the necessary validation to x/net/http2. For #60374 Change-Id: I05cb6866a9bead043101954dfded199258c6dd04 Reviewed-on: https://go-review.googlesource.com/c/go/+/506996 Reviewed-by: Tatiana Bradley TryBot-Result: Gopher Robot Run-TryBot: Damien Neil --- src/net/http/http_test.go | 29 -------------------------- src/net/http/request.go | 47 +++++++++--------------------------------- src/net/http/request_test.go | 11 ++-------- src/net/http/transport_test.go | 19 +++++++++++++++++ 4 files changed, 31 insertions(+), 75 deletions(-) diff --git a/src/net/http/http_test.go b/src/net/http/http_test.go index 1c9fb33b69..91bb1b2620 100644 --- a/src/net/http/http_test.go +++ b/src/net/http/http_test.go @@ -48,35 +48,6 @@ func TestForeachHeaderElement(t *testing.T) { } } -func TestCleanHost(t *testing.T) { - tests := []struct { - in, want string - }{ - {"www.google.com", "www.google.com"}, - {"www.google.com foo", "www.google.com"}, - {"www.google.com/foo", "www.google.com"}, - {" first character is a space", ""}, - {"[1::6]:8080", "[1::6]:8080"}, - - // Punycode: - {"гофер.рф/foo", "xn--c1ae0ajs.xn--p1ai"}, - {"bücher.de", "xn--bcher-kva.de"}, - {"bücher.de:8080", "xn--bcher-kva.de:8080"}, - // Verify we convert to lowercase before punycode: - {"BÜCHER.de", "xn--bcher-kva.de"}, - {"BÜCHER.de:8080", "xn--bcher-kva.de:8080"}, - // Verify we normalize to NFC before punycode: - {"gophér.nfc", "xn--gophr-esa.nfc"}, // NFC input; no work needed - {"goph\u0065\u0301r.nfd", "xn--gophr-esa.nfd"}, // NFD input - } - for _, tt := range tests { - got := cleanHost(tt.in) - if tt.want != got { - t.Errorf("cleanHost(%q) = %q, want %q", tt.in, got, tt.want) - } - } -} - // Test that cmd/go doesn't link in the HTTP server. // // This catches accidental dependencies between the HTTP transport and diff --git a/src/net/http/request.go b/src/net/http/request.go index 4e9190493c..bd868373c5 100644 --- a/src/net/http/request.go +++ b/src/net/http/request.go @@ -17,7 +17,6 @@ import ( "io" "mime" "mime/multipart" - "net" "net/http/httptrace" "net/http/internal/ascii" "net/textproto" @@ -27,6 +26,7 @@ import ( "strings" "sync" + "golang.org/x/net/http/httpguts" "golang.org/x/net/idna" ) @@ -580,12 +580,19 @@ func (r *Request) write(w io.Writer, usingProxy bool, extraHeaders Header, waitF // is not given, use the host from the request URL. // // Clean the host, in case it arrives with unexpected stuff in it. - host := cleanHost(r.Host) + host := r.Host if host == "" { if r.URL == nil { return errMissingHost } - host = cleanHost(r.URL.Host) + host = r.URL.Host + } + host, err = httpguts.PunycodeHostPort(host) + if err != nil { + return err + } + if !httpguts.ValidHostHeader(host) { + return errors.New("http: invalid Host header") } // According to RFC 6874, an HTTP client, proxy, or other @@ -742,40 +749,6 @@ func idnaASCII(v string) (string, error) { return idna.Lookup.ToASCII(v) } -// cleanHost cleans up the host sent in request's Host header. -// -// It both strips anything after '/' or ' ', and puts the value -// into Punycode form, if necessary. -// -// Ideally we'd clean the Host header according to the spec: -// -// https://tools.ietf.org/html/rfc7230#section-5.4 (Host = uri-host [ ":" port ]") -// https://tools.ietf.org/html/rfc7230#section-2.7 (uri-host -> rfc3986's host) -// https://tools.ietf.org/html/rfc3986#section-3.2.2 (definition of host) -// -// But practically, what we are trying to avoid is the situation in -// issue 11206, where a malformed Host header used in the proxy context -// would create a bad request. So it is enough to just truncate at the -// first offending character. -func cleanHost(in string) string { - if i := strings.IndexAny(in, " /"); i != -1 { - in = in[:i] - } - host, port, err := net.SplitHostPort(in) - if err != nil { // input was just a host - a, err := idnaASCII(in) - if err != nil { - return in // garbage in, garbage out - } - return a - } - a, err := idnaASCII(host) - if err != nil { - return in // garbage in, garbage out - } - return net.JoinHostPort(a, port) -} - // removeZone removes IPv6 zone identifier from host. // E.g., "[fe80::1%en0]:8080" to "[fe80::1]:8080" func removeZone(host string) string { diff --git a/src/net/http/request_test.go b/src/net/http/request_test.go index 78b968f23c..0892bc255f 100644 --- a/src/net/http/request_test.go +++ b/src/net/http/request_test.go @@ -775,15 +775,8 @@ func TestRequestBadHost(t *testing.T) { } req.Host = "foo.com with spaces" req.URL.Host = "foo.com with spaces" - req.Write(logWrites{t, &got}) - want := []string{ - "GET /after HTTP/1.1\r\n", - "Host: foo.com\r\n", - "User-Agent: " + DefaultUserAgent + "\r\n", - "\r\n", - } - if !reflect.DeepEqual(got, want) { - t.Errorf("Writes = %q\n Want = %q", got, want) + if err := req.Write(logWrites{t, &got}); err == nil { + t.Errorf("Writing request with invalid Host: succeded, want error") } } diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go index 172aba679b..028fecc961 100644 --- a/src/net/http/transport_test.go +++ b/src/net/http/transport_test.go @@ -6731,3 +6731,22 @@ func testHandlerAbortRacesBodyRead(t *testing.T, mode testMode) { } wg.Wait() } + +func TestRequestSanitization(t *testing.T) { run(t, testRequestSanitization) } +func testRequestSanitization(t *testing.T, mode testMode) { + if mode == http2Mode { + // Remove this after updating x/net. + t.Skip("https://go.dev/issue/60374 test fails when run with HTTP/2") + } + ts := newClientServerTest(t, mode, HandlerFunc(func(rw ResponseWriter, req *Request) { + if h, ok := req.Header["X-Evil"]; ok { + t.Errorf("request has X-Evil header: %q", h) + } + })).ts + req, _ := NewRequest("GET", ts.URL, nil) + req.Host = "go.dev\r\nX-Evil:evil" + resp, _ := ts.Client().Do(req) + if resp != nil { + resp.Body.Close() + } +} -- cgit v1.2.3-54-g00ecf From 2db31efdba131a411474608ebcf96cc964f8032b Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Thu, 29 Jun 2023 09:48:52 -0700 Subject: cmd/compile/internal/types2: make TestIssue43124 match the go/types version Replace the (flaky) types2.TestIssue43124 with the code of the (stable) go/types version of this test. While at it, replace a handful of syntax.Pos{} with the equivalent nopos, to further reduce differences between the two versions of the issues_test.go file. For #61064. Change-Id: I69f3e4627a48c9928e335d67736cb875ba3835fc Reviewed-on: https://go-review.googlesource.com/c/go/+/507215 TryBot-Result: Gopher Robot Reviewed-by: Bryan Mills Reviewed-by: Robert Griesemer Run-TryBot: Robert Griesemer --- src/cmd/compile/internal/types2/issues_test.go | 81 ++++++++++++++++++-------- 1 file changed, 56 insertions(+), 25 deletions(-) diff --git a/src/cmd/compile/internal/types2/issues_test.go b/src/cmd/compile/internal/types2/issues_test.go index 8bd42a5271..5e0ae213dc 100644 --- a/src/cmd/compile/internal/types2/issues_test.go +++ b/src/cmd/compile/internal/types2/issues_test.go @@ -497,14 +497,14 @@ func TestIssue43088(t *testing.T) { // _ T2 // } // } - n1 := NewTypeName(syntax.Pos{}, nil, "T1", nil) + n1 := NewTypeName(nopos, nil, "T1", nil) T1 := NewNamed(n1, nil, nil) - n2 := NewTypeName(syntax.Pos{}, nil, "T2", nil) + n2 := NewTypeName(nopos, nil, "T2", nil) T2 := NewNamed(n2, nil, nil) - s1 := NewStruct([]*Var{NewField(syntax.Pos{}, nil, "_", T2, false)}, nil) + s1 := NewStruct([]*Var{NewField(nopos, nil, "_", T2, false)}, nil) T1.SetUnderlying(s1) - s2 := NewStruct([]*Var{NewField(syntax.Pos{}, nil, "_", T2, false)}, nil) - s3 := NewStruct([]*Var{NewField(syntax.Pos{}, nil, "_", s2, false)}, nil) + s2 := NewStruct([]*Var{NewField(nopos, nil, "_", T2, false)}, nil) + s3 := NewStruct([]*Var{NewField(nopos, nil, "_", s2, false)}, nil) T2.SetUnderlying(s3) // These calls must terminate (no endless recursion). @@ -535,38 +535,69 @@ func TestIssue44515(t *testing.T) { } func TestIssue43124(t *testing.T) { - testenv.MustHaveGoBuild(t) + // TODO(rFindley) move this to testdata by enhancing support for importing. + + testenv.MustHaveGoBuild(t) // The go command is needed for the importer to determine the locations of stdlib .a files. // All involved packages have the same name (template). Error messages should // disambiguate between text/template and html/template by printing the full // path. const ( asrc = `package a; import "text/template"; func F(template.Template) {}; func G(int) {}` - bsrc = `package b; import ("a"; "html/template"); func _() { a.F(template.Template{}) }` - csrc = `package c; import ("a"; "html/template"); func _() { a.G(template.Template{}) }` - ) + bsrc = ` +package b - a := mustTypecheck(asrc, nil, nil) - conf := Config{Importer: importHelper{pkg: a, fallback: defaultImporter()}} +import ( + "a" + "html/template" +) +func _() { // Packages should be fully qualified when there is ambiguity within the // error string itself. - _, err := typecheck(bsrc, &conf, nil) - if err == nil { - t.Fatal("package b had no errors") - } - if !strings.Contains(err.Error(), "text/template") || !strings.Contains(err.Error(), "html/template") { - t.Errorf("type checking error for b does not disambiguate package template: %q", err) - } + a.F(template /* ERRORx "cannot use.*html/template.* as .*text/template" */ .Template{}) +} +` + csrc = ` +package c - // ...and also when there is any ambiguity in reachable packages. - _, err = typecheck(csrc, &conf, nil) - if err == nil { - t.Fatal("package c had no errors") - } - if !strings.Contains(err.Error(), "html/template") { - t.Errorf("type checking error for c does not disambiguate package template: %q", err) +import ( + "a" + "fmt" + "html/template" +) + +// go.dev/issue/46905: make sure template is not the first package qualified. +var _ fmt.Stringer = 1 // ERRORx "cannot use 1.*as fmt\\.Stringer" + +// Packages should be fully qualified when there is ambiguity in reachable +// packages. In this case both a (and for that matter html/template) import +// text/template. +func _() { a.G(template /* ERRORx "cannot use .*html/template.*Template" */ .Template{}) } +` + + tsrc = ` +package template + +import "text/template" + +type T int + +// Verify that the current package name also causes disambiguation. +var _ T = template /* ERRORx "cannot use.*text/template.* as T value" */.Template{} +` + ) + + a := mustTypecheck(asrc, nil, nil) + imp := importHelper{pkg: a, fallback: defaultImporter()} + + withImporter := func(cfg *Config) { + cfg.Importer = imp } + + testFiles(t, []string{"b.go"}, [][]byte{[]byte(bsrc)}, 0, false, withImporter) + testFiles(t, []string{"c.go"}, [][]byte{[]byte(csrc)}, 0, false, withImporter) + testFiles(t, []string{"t.go"}, [][]byte{[]byte(tsrc)}, 0, false, withImporter) } func TestIssue50646(t *testing.T) { -- cgit v1.2.3-54-g00ecf From 683f51d3071abe8dbe13ef877595825b469f30e3 Mon Sep 17 00:00:00 2001 From: Jes Cok Date: Thu, 29 Jun 2023 14:17:00 +0000 Subject: cmd/asm/internal/lex: fix comment, remove the first "has" Change-Id: I429f0fa6c99ef576fe83c7bd0d1c1e176ecbb179 GitHub-Last-Rev: fb581b7f271f026182de0737c4fe5c360d5dea96 GitHub-Pull-Request: golang/go#61066 Reviewed-on: https://go-review.googlesource.com/c/go/+/507097 TryBot-Result: Gopher Robot Reviewed-by: Dmitri Shuralyov Auto-Submit: Dmitri Shuralyov Reviewed-by: Cherry Mui Run-TryBot: Cherry Mui --- src/cmd/asm/internal/lex/slice.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cmd/asm/internal/lex/slice.go b/src/cmd/asm/internal/lex/slice.go index 8ee0c7035f..61b15dd963 100644 --- a/src/cmd/asm/internal/lex/slice.go +++ b/src/cmd/asm/internal/lex/slice.go @@ -65,7 +65,7 @@ func (s *Slice) Col() int { // #define A #define B(x) x // and // #define A #define B (x) x - // The first has definition of B has an argument, the second doesn't. Because we let + // The first definition of B has an argument, the second doesn't. Because we let // text/scanner strip the blanks for us, this is extremely rare, hard to fix, and not worth it. return s.pos } -- cgit v1.2.3-54-g00ecf From 1e97c515367103da5278fe7047df8d5ffd8a3267 Mon Sep 17 00:00:00 2001 From: Chris O'Hara Date: Sat, 3 Jun 2023 11:49:48 +1000 Subject: syscall: stub Getrlimit on wasip1 This is a prerequisite to enabling the pure Go resolver for wasip1. Change-Id: Iecd8a18ce4c9eb69a697d29930bedb7175b4f0ce Reviewed-on: https://go-review.googlesource.com/c/go/+/500577 Run-TryBot: Ian Lance Taylor Reviewed-by: Johan Brandhorst-Satzkorn Reviewed-by: Ian Lance Taylor Reviewed-by: Dmitri Shuralyov Auto-Submit: Johan Brandhorst-Satzkorn TryBot-Result: Gopher Robot --- src/syscall/syscall_wasip1.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/syscall/syscall_wasip1.go b/src/syscall/syscall_wasip1.go index 5d19c000ae..e66afee5e9 100644 --- a/src/syscall/syscall_wasip1.go +++ b/src/syscall/syscall_wasip1.go @@ -478,3 +478,16 @@ func SetNonblock(fd int, nonblocking bool) error { errno := fd_fdstat_set_flags(int32(fd), flags) return errnoErr(errno) } + +type Rlimit struct { + Cur uint64 + Max uint64 +} + +const ( + RLIMIT_NOFILE = iota +) + +func Getrlimit(which int, lim *Rlimit) error { + return ENOSYS +} -- cgit v1.2.3-54-g00ecf From 18e17e2cb12837ea2c8582ecdb0cc780f49a1aac Mon Sep 17 00:00:00 2001 From: Chris O'Hara Date: Sat, 3 Jun 2023 11:51:02 +1000 Subject: net: enable pure Go resolver for wasip1 Top-level functions in the net package that only read files, for example LookupPort(...), or LookupIP(host) where host resides in /etc/hosts, now work on wasip1. If the application has the ability to create sockets (for example, when using a sockets extension to WASI preview 1), it's now possible to do name resolution by passing a custom Dial function to a Resolver instance. Change-Id: I923886f67e336820bc89f09ea1855387c8dac61a Reviewed-on: https://go-review.googlesource.com/c/go/+/500579 Auto-Submit: Ian Lance Taylor Reviewed-by: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Gopher Robot Reviewed-by: Randy Reddig Reviewed-by: Johan Brandhorst-Satzkorn Reviewed-by: Dmitri Shuralyov --- src/net/cgo_stub.go | 5 +++-- src/net/conf.go | 2 +- src/net/dnsclient_unix.go | 2 +- src/net/dnsconfig_unix.go | 2 +- src/net/lookup_fake.go | 2 +- src/net/lookup_unix.go | 2 +- src/net/net_fake.go | 6 ------ src/net/net_fake_js.go | 11 ++++++++++- 8 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/net/cgo_stub.go b/src/net/cgo_stub.go index bd483110b5..a8514c19f8 100644 --- a/src/net/cgo_stub.go +++ b/src/net/cgo_stub.go @@ -4,11 +4,12 @@ // This file holds stub versions of the cgo functions called on Unix systems. // We build this file if using the netgo build tag, or if cgo is not -// enabled and we are using a Unix system other than Darwin. +// enabled and we are using a Unix system other than Darwin, or if it's +// wasip1 where cgo is never available. // Darwin is exempted because it always provides the cgo routines, // in cgo_unix_syscall.go. -//go:build netgo || (!cgo && unix && !darwin) +//go:build netgo || (!cgo && unix && !darwin) || wasip1 package net diff --git a/src/net/conf.go b/src/net/conf.go index 1db166c9e3..77cc635592 100644 --- a/src/net/conf.go +++ b/src/net/conf.go @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//go:build !js && !wasip1 +//go:build !js package net diff --git a/src/net/dnsclient_unix.go b/src/net/dnsclient_unix.go index f3c075c83f..dab5144e5d 100644 --- a/src/net/dnsclient_unix.go +++ b/src/net/dnsclient_unix.go @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//go:build !js && !wasip1 +//go:build !js // DNS client: see RFC 1035. // Has to be linked into package net for Dial. diff --git a/src/net/dnsconfig_unix.go b/src/net/dnsconfig_unix.go index d5f34e5300..69b300410a 100644 --- a/src/net/dnsconfig_unix.go +++ b/src/net/dnsconfig_unix.go @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//go:build !js && !wasip1 && !windows +//go:build !js && !windows // Read system DNS config from /etc/resolv.conf diff --git a/src/net/lookup_fake.go b/src/net/lookup_fake.go index 45146e1c95..c27eae4ba5 100644 --- a/src/net/lookup_fake.go +++ b/src/net/lookup_fake.go @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//go:build (js && wasm) || wasip1 +//go:build js && wasm package net diff --git a/src/net/lookup_unix.go b/src/net/lookup_unix.go index dc75e0a3b6..56ae11e961 100644 --- a/src/net/lookup_unix.go +++ b/src/net/lookup_unix.go @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -//go:build unix +//go:build unix || wasip1 package net diff --git a/src/net/net_fake.go b/src/net/net_fake.go index 68d36966ca..908767a1f6 100644 --- a/src/net/net_fake.go +++ b/src/net/net_fake.go @@ -15,8 +15,6 @@ import ( "sync" "syscall" "time" - - "golang.org/x/net/dns/dnsmessage" ) var listenersMu sync.Mutex @@ -406,7 +404,3 @@ func (fd *fakeNetFD) writeMsg(p []byte, oob []byte, sa syscall.Sockaddr) (n int, func (fd *fakeNetFD) dup() (f *os.File, err error) { return nil, syscall.ENOSYS } - -func (r *Resolver) lookup(ctx context.Context, name string, qtype dnsmessage.Type, conf *dnsConfig) (dnsmessage.Parser, string, error) { - panic("unreachable") -} diff --git a/src/net/net_fake_js.go b/src/net/net_fake_js.go index 1fc0b50b7d..7ba108b664 100644 --- a/src/net/net_fake_js.go +++ b/src/net/net_fake_js.go @@ -8,7 +8,12 @@ package net -import "internal/poll" +import ( + "context" + "internal/poll" + + "golang.org/x/net/dns/dnsmessage" +) // Network file descriptor. type netFD struct { @@ -25,3 +30,7 @@ type netFD struct { pfd poll.FD isConnected bool // handshake completed or use of association with peer } + +func (r *Resolver) lookup(ctx context.Context, name string, qtype dnsmessage.Type, conf *dnsConfig) (dnsmessage.Parser, string, error) { + panic("unreachable") +} -- cgit v1.2.3-54-g00ecf From 5b72f45dd17314af39627c2fcac0fbc099b67603 Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Fri, 30 Jun 2023 14:32:39 -0400 Subject: runtime: check GOFLAGS not GCFLAGS GCFLAGS doesn't have any defined meaning. cmd/dist enables mayMoreStackPreempt with GOFLAGS. For #55160. Change-Id: I7ac71e4a1a983a56bd228ab5d24294db5cc595f7 Reviewed-on: https://go-review.googlesource.com/c/go/+/507359 Run-TryBot: Michael Pratt TryBot-Result: Gopher Robot Reviewed-by: Michael Knyszek Auto-Submit: Michael Pratt --- src/runtime/crash_unix_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/crash_unix_test.go b/src/runtime/crash_unix_test.go index 07060b8fab..6bca2ac66e 100644 --- a/src/runtime/crash_unix_test.go +++ b/src/runtime/crash_unix_test.go @@ -76,7 +76,7 @@ func TestCrashDumpsAllThreads(t *testing.T) { testenv.MustHaveGoBuild(t) - if strings.Contains(os.Getenv("GCFLAGS"), "mayMoreStackPreempt") { + if strings.Contains(os.Getenv("GOFLAGS"), "mayMoreStackPreempt") { // This test occasionally times out in this debug mode. This is probably // revealing a real bug in the scheduler, but since it seems to only // affect this test and this is itself a test of a debug mode, it's not -- cgit v1.2.3-54-g00ecf From 5c1a15df4141f77ba7c42c2c7c06d6eb22bcda63 Mon Sep 17 00:00:00 2001 From: Meng Zhuo Date: Sun, 25 Jun 2023 11:20:35 +0800 Subject: test/codegen: enable Mul2 DivPow2 test for riscv64 Change-Id: Ice0bb7a665599b334e927a1b00d1a5b400c15e3d Reviewed-on: https://go-review.googlesource.com/c/go/+/506035 TryBot-Result: Gopher Robot Reviewed-by: David Chase Reviewed-by: Keith Randall Reviewed-by: Keith Randall Run-TryBot: Keith Randall --- test/codegen/floats.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/codegen/floats.go b/test/codegen/floats.go index 81471082d4..9cb62e031a 100644 --- a/test/codegen/floats.go +++ b/test/codegen/floats.go @@ -20,6 +20,7 @@ func Mul2(f float64) float64 { // arm/7:"ADDD",-"MULD" // arm64:"FADDD",-"FMULD" // ppc64x:"FADD",-"FMUL" + // riscv64:"FADDD",-"FMULD" return f * 2.0 } @@ -29,6 +30,7 @@ func DivPow2(f1, f2, f3 float64) (float64, float64, float64) { // arm/7:"MULD",-"DIVD" // arm64:"FMULD",-"FDIVD" // ppc64x:"FMUL",-"FDIV" + // riscv64:"FMULD",-"FDIVD" x := f1 / 16.0 // 386/sse2:"MULSD",-"DIVSD" @@ -36,6 +38,7 @@ func DivPow2(f1, f2, f3 float64) (float64, float64, float64) { // arm/7:"MULD",-"DIVD" // arm64:"FMULD",-"FDIVD" // ppc64x:"FMUL",-"FDIVD" + // riscv64:"FMULD",-"FDIVD" y := f2 / 0.125 // 386/sse2:"ADDSD",-"DIVSD",-"MULSD" @@ -43,6 +46,7 @@ func DivPow2(f1, f2, f3 float64) (float64, float64, float64) { // arm/7:"ADDD",-"MULD",-"DIVD" // arm64:"FADDD",-"FMULD",-"FDIVD" // ppc64x:"FADD",-"FMUL",-"FDIV" + // riscv64:"FADDD",-"FMULD",-"FDIVD" z := f3 / 0.5 return x, y, z -- cgit v1.2.3-54-g00ecf From e4ed92a355cebc399dc34d33a556f656fa5c7690 Mon Sep 17 00:00:00 2001 From: Oleksandr Redko Date: Thu, 29 Jun 2023 16:06:19 +0300 Subject: os, syscall: update unreachable link about =C: envs Change-Id: I185dec133599f9c69fda7563697bbc33e433fb78 Reviewed-on: https://go-review.googlesource.com/c/go/+/507135 TryBot-Result: Gopher Robot Run-TryBot: Ian Lance Taylor Reviewed-by: Dmitri Shuralyov Reviewed-by: Ian Lance Taylor Auto-Submit: Ian Lance Taylor --- src/os/env_test.go | 2 +- src/syscall/env_windows.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/os/env_test.go b/src/os/env_test.go index 1b9e26594c..5809f4b866 100644 --- a/src/os/env_test.go +++ b/src/os/env_test.go @@ -130,7 +130,7 @@ func TestClearenv(t *testing.T) { defer func(origEnv []string) { for _, pair := range origEnv { // Environment variables on Windows can begin with = - // https://blogs.msdn.com/b/oldnewthing/archive/2010/05/06/10008132.aspx + // https://devblogs.microsoft.com/oldnewthing/20100506-00/?p=14133 i := strings.Index(pair[1:], "=") + 1 if err := Setenv(pair[:i], pair[i+1:]); err != nil { t.Errorf("Setenv(%q, %q) failed during reset: %v", pair[:i], pair[i+1:], err) diff --git a/src/syscall/env_windows.go b/src/syscall/env_windows.go index 20d74b51e0..220a005e1a 100644 --- a/src/syscall/env_windows.go +++ b/src/syscall/env_windows.go @@ -62,7 +62,7 @@ func Clearenv() { for _, s := range Environ() { // Environment variables can begin with = // so start looking for the separator = at j=1. - // https://blogs.msdn.com/b/oldnewthing/archive/2010/05/06/10008132.aspx + // https://devblogs.microsoft.com/oldnewthing/20100506-00/?p=14133 for j := 1; j < len(s); j++ { if s[j] == '=' { Unsetenv(s[0:j]) -- cgit v1.2.3-54-g00ecf From e126572f8a91d42b86242012012d0cad4507dca8 Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Mon, 3 Jul 2023 13:16:59 -0700 Subject: runtime: have ReadMemStats do a nil check before switching stacks This gives the user a better stack trace experience. No need to expose them to runtime.systemstack and friends. Fixes #61158 Change-Id: I4f423f82e54b062773067c0ae64622e37cb3948b Reviewed-on: https://go-review.googlesource.com/c/go/+/507755 TryBot-Result: Gopher Robot Reviewed-by: Michael Knyszek Run-TryBot: Keith Randall Reviewed-by: Keith Randall --- src/runtime/mstats.go | 1 + src/runtime/traceback.go | 1 + 2 files changed, 2 insertions(+) diff --git a/src/runtime/mstats.go b/src/runtime/mstats.go index ab383dd8e3..9a247b87b5 100644 --- a/src/runtime/mstats.go +++ b/src/runtime/mstats.go @@ -347,6 +347,7 @@ func init() { // which is a snapshot as of the most recently completed garbage // collection cycle. func ReadMemStats(m *MemStats) { + _ = m.Alloc // nil check test before we switch stacks, see issue 61158 stopTheWorld(stwReadMemStats) systemstack(func() { diff --git a/src/runtime/traceback.go b/src/runtime/traceback.go index d6f89210a4..86df1155b5 100644 --- a/src/runtime/traceback.go +++ b/src/runtime/traceback.go @@ -1147,6 +1147,7 @@ func showfuncinfo(sf srcFunc, firstFrame bool, calleeID abi.FuncID) bool { // isExportedRuntime reports whether name is an exported runtime function. // It is only for runtime functions, so ASCII A-Z is fine. +// TODO: this handles exported functions but not exported methods. func isExportedRuntime(name string) bool { const n = len("runtime.") return len(name) > n && name[:n] == "runtime." && 'A' <= name[n] && name[n] <= 'Z' -- cgit v1.2.3-54-g00ecf From b2215e49c79ef5078f3c1b46adc0bef6109af388 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Fri, 9 Jun 2023 19:14:55 +0000 Subject: runtime,runtime/metrics: clarify OS stack metrics There are some subtle details here about measuring OS stacks in cgo programs. There's also an expectation about magnitude in the MemStats docs that isn't in the runtime/metrics docs. Fix both. Fixes #54396. Change-Id: I6b60a62a4a304e6688e7ab4d511d66193fc25321 Reviewed-on: https://go-review.googlesource.com/c/go/+/502156 Run-TryBot: Michael Knyszek Auto-Submit: Michael Knyszek TryBot-Result: Gopher Robot Reviewed-by: Michael Pratt --- src/runtime/metrics/description.go | 18 ++++++++++++------ src/runtime/metrics/doc.go | 11 ++++++++++- src/runtime/mstats.go | 12 +++++++++++- 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/runtime/metrics/description.go b/src/runtime/metrics/description.go index aea51c7f75..745691b24f 100644 --- a/src/runtime/metrics/description.go +++ b/src/runtime/metrics/description.go @@ -339,9 +339,11 @@ var allDesc = []Description{ Kind: KindUint64, }, { - Name: "/memory/classes/heap/stacks:bytes", - Description: "Memory allocated from the heap that is reserved for stack space, whether or not it is currently in-use.", - Kind: KindUint64, + Name: "/memory/classes/heap/stacks:bytes", + Description: "Memory allocated from the heap that is reserved for stack space, whether or not it is currently in-use. " + + "Currently, this represents all stack memory for goroutines. It also includes all OS thread stacks in non-cgo programs. " + + "Note that stacks may be allocated differently in the future, and this may change.", + Kind: KindUint64, }, { Name: "/memory/classes/heap/unused:bytes", @@ -374,9 +376,13 @@ var allDesc = []Description{ Kind: KindUint64, }, { - Name: "/memory/classes/os-stacks:bytes", - Description: "Stack memory allocated by the underlying operating system.", - Kind: KindUint64, + Name: "/memory/classes/os-stacks:bytes", + Description: "Stack memory allocated by the underlying operating system. " + + "In non-cgo programs this metric is currently zero. This may change in the future." + + "In cgo programs this metric includes OS thread stacks allocated directly from the OS. " + + "Currently, this only accounts for one stack in c-shared and c-archive build modes, " + + "and other sources of stacks from the OS are not measured. This too may change in the future.", + Kind: KindUint64, }, { Name: "/memory/classes/other:bytes", diff --git a/src/runtime/metrics/doc.go b/src/runtime/metrics/doc.go index 5238bcea8e..5c52f78477 100644 --- a/src/runtime/metrics/doc.go +++ b/src/runtime/metrics/doc.go @@ -318,7 +318,10 @@ Below is the full list of supported metrics, ordered lexicographically. /memory/classes/heap/stacks:bytes Memory allocated from the heap that is reserved for stack space, - whether or not it is currently in-use. + whether or not it is currently in-use. Currently, this + represents all stack memory for goroutines. It also includes all + OS thread stacks in non-cgo programs. Note that stacks may be + allocated differently in the future, and this may change. /memory/classes/heap/unused:bytes Memory that is reserved for heap objects but is not currently @@ -345,6 +348,12 @@ Below is the full list of supported metrics, ordered lexicographically. /memory/classes/os-stacks:bytes Stack memory allocated by the underlying operating system. + In non-cgo programs this metric is currently zero. This may + change in the future.In cgo programs this metric includes + OS thread stacks allocated directly from the OS. Currently, + this only accounts for one stack in c-shared and c-archive build + modes, and other sources of stacks from the OS are not measured. + This too may change in the future. /memory/classes/other:bytes Memory used by execution trace buffers, structures for debugging diff --git a/src/runtime/mstats.go b/src/runtime/mstats.go index 9a247b87b5..9cdc565137 100644 --- a/src/runtime/mstats.go +++ b/src/runtime/mstats.go @@ -199,7 +199,17 @@ type MemStats struct { // StackSys is bytes of stack memory obtained from the OS. // // StackSys is StackInuse, plus any memory obtained directly - // from the OS for OS thread stacks (which should be minimal). + // from the OS for OS thread stacks. + // + // In non-cgo programs this metric is currently equal to StackInuse + // (but this should not be relied upon, and the value may change in + // the future). + // + // In cgo programs this metric includes OS thread stacks allocated + // directly from the OS. Currently, this only accounts for one stack in + // c-shared and c-archive build modes and other sources of stacks from + // the OS (notably, any allocated by C code) are not currently measured. + // Note this too may change in the future. StackSys uint64 // Off-heap memory statistics. -- cgit v1.2.3-54-g00ecf From cd6676126b7e663e6202e98e2f235fff20d5e858 Mon Sep 17 00:00:00 2001 From: zikaeroh Date: Thu, 22 Jun 2023 17:09:21 -0700 Subject: database/sql: prevent internal context error from being returned from Rows.Err() CL 497675 modified Rows such that context errors are propagated through Rows.Err(). This caused an issue where calling Close meant that an internal cancellation error would (eventually) be returned from Err: 1. A caller makes a query using a cancellable context. 2. initContextClose sees that either the query context or the transaction context can be canceled, so will need to spawn a goroutine to capture their errors. 3. initContextClose derives a context from the query context via WithCancel and sets rs.cancel. 4. When a user calls Close, rs.cancel is called. awaitDone's ctx is cancelled, which is good, since we don't want it to hang forever. 5. This internal cancellation (after CL 497675) has its error saved on contextDone. 6. Later, calling Err will return the error in contextDone if present. This leads to a race condition depending on how quickly Err is called after Close. The docs for Close and Err state that calling Close should have no affect on the return result for Err. So, a potential fix is to ensure that awaitDone does not save the error when the cancellation comes from a Close via rs.cancel. This CL does that, using a new context not derived from the query context, whose error is ignored as the query context's error used to be before the original bugfix. The included test fails before the CL, and passes afterward. Fixes #60932 Change-Id: I2bf4c549efd83d62b86e298c9c45ebd06a3ad89a Reviewed-on: https://go-review.googlesource.com/c/go/+/505397 Auto-Submit: Russ Cox Run-TryBot: Russ Cox TryBot-Result: Gopher Robot Reviewed-by: Russ Cox Reviewed-by: Dmitri Shuralyov Reviewed-by: Brad Fitzpatrick --- src/database/sql/sql.go | 17 +++++++++++------ src/database/sql/sql_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go index a77d63dc5e..0764c7d17a 100644 --- a/src/database/sql/sql.go +++ b/src/database/sql/sql.go @@ -2944,15 +2944,17 @@ func (rs *Rows) initContextClose(ctx, txctx context.Context) { if bypassRowsAwaitDone { return } - ctx, rs.cancel = context.WithCancel(ctx) - go rs.awaitDone(ctx, txctx) + closectx, cancel := context.WithCancel(ctx) + rs.cancel = cancel + go rs.awaitDone(ctx, txctx, closectx) } -// awaitDone blocks until either ctx or txctx is canceled. The ctx is provided -// from the query context and is canceled when the query Rows is closed. +// awaitDone blocks until ctx, txctx, or closectx is canceled. +// The ctx is provided from the query context. // If the query was issued in a transaction, the transaction's context -// is also provided in txctx to ensure Rows is closed if the Tx is closed. -func (rs *Rows) awaitDone(ctx, txctx context.Context) { +// is also provided in txctx, to ensure Rows is closed if the Tx is closed. +// The closectx is closed by an explicit call to rs.Close. +func (rs *Rows) awaitDone(ctx, txctx, closectx context.Context) { var txctxDone <-chan struct{} if txctx != nil { txctxDone = txctx.Done() @@ -2964,6 +2966,9 @@ func (rs *Rows) awaitDone(ctx, txctx context.Context) { case <-txctxDone: err := txctx.Err() rs.contextDone.Store(&err) + case <-closectx.Done(): + // rs.cancel was called via Close(); don't store this into contextDone + // to ensure Err() is unaffected. } rs.close(ctx.Err()) } diff --git a/src/database/sql/sql_test.go b/src/database/sql/sql_test.go index 718056c351..e6a5cd912a 100644 --- a/src/database/sql/sql_test.go +++ b/src/database/sql/sql_test.go @@ -4493,6 +4493,31 @@ func TestContextCancelBetweenNextAndErr(t *testing.T) { } } +func TestNilErrorAfterClose(t *testing.T) { + db := newTestDB(t, "people") + defer closeDB(t, db) + + // This WithCancel is important; Rows contains an optimization to avoid + // spawning a goroutine when the query/transaction context cannot be + // canceled, but this test tests a bug which is caused by said goroutine. + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + r, err := db.QueryContext(ctx, "SELECT|people|name|") + if err != nil { + t.Fatal(err) + } + + if err := r.Close(); err != nil { + t.Fatal(err) + } + + time.Sleep(10 * time.Millisecond) // increase odds of seeing failure + if err := r.Err(); err != nil { + t.Fatal(err) + } +} + // badConn implements a bad driver.Conn, for TestBadDriver. // The Exec method panics. type badConn struct{} -- cgit v1.2.3-54-g00ecf From c8dad424bf01df69af729845acc151a66b87d594 Mon Sep 17 00:00:00 2001 From: Michael Munday Date: Sat, 1 Jul 2023 00:01:26 +0100 Subject: math: fix portable FMA when x*y < 0 and x*y == -z When x*y == -z the portable implementation of FMA copied the sign bit from x*y into the result. This meant that when x*y == -z and x*y < 0 the result was -0 which is incorrect. Fixes #61130. Change-Id: Ib93a568b7bdb9031e2aedfa1bdfa9bddde90851d Reviewed-on: https://go-review.googlesource.com/c/go/+/507376 Reviewed-by: Dmitri Shuralyov Reviewed-by: Keith Randall Run-TryBot: Michael Munday TryBot-Result: Gopher Robot Reviewed-by: Joedian Reid --- src/math/all_test.go | 7 +++++++ src/math/fma.go | 5 +++++ 2 files changed, 12 insertions(+) diff --git a/src/math/all_test.go b/src/math/all_test.go index 886267bc17..96a398e9c6 100644 --- a/src/math/all_test.go +++ b/src/math/all_test.go @@ -2059,6 +2059,9 @@ var fmaC = []struct{ x, y, z, want float64 }{ // Special {0, 0, 0, 0}, + {Copysign(0, -1), 0, 0, 0}, + {0, 0, Copysign(0, -1), 0}, + {Copysign(0, -1), 0, Copysign(0, -1), Copysign(0, -1)}, {-1.1754226043408471e-38, NaN(), Inf(0), NaN()}, {0, 0, 2.22507385643494e-308, 2.22507385643494e-308}, {-8.65697792e+09, NaN(), -7.516192799999999e+09, NaN()}, @@ -2077,6 +2080,10 @@ var fmaC = []struct{ x, y, z, want float64 }{ {4.612811918325842e+18, 1.4901161193847641e-08, 2.6077032311277997e-08, 6.873625395187494e+10}, {-9.094947033611148e-13, 4.450691014249257e-308, 2.086006742350485e-308, 2.086006742346437e-308}, {-7.751454006381804e-05, 5.588653777189071e-308, -2.2207280111272877e-308, -2.2211612130544025e-308}, + + // Issue #61130 + {-1, 1, 1, 0}, + {1, 1, -1, 0}, } var sqrt32 = []float32{ diff --git a/src/math/fma.go b/src/math/fma.go index ca0bf99f21..ba03fbe8a9 100644 --- a/src/math/fma.go +++ b/src/math/fma.go @@ -132,6 +132,11 @@ func FMA(x, y, z float64) float64 { ps, pe, pm1, pm2, zs, ze, zm1, zm2 = zs, ze, zm1, zm2, ps, pe, pm1, pm2 } + // Special case: if p == -z the result is always +0 since neither operand is zero. + if ps != zs && pe == ze && pm1 == zm1 && pm2 == zm2 { + return 0 + } + // Align significands zm1, zm2 = shrcompress(zm1, zm2, uint(pe-ze)) -- cgit v1.2.3-54-g00ecf From 3fce1115359c4ab7d67fbf4efef1341e52b354b7 Mon Sep 17 00:00:00 2001 From: Meng Zhuo Date: Tue, 27 Jun 2023 23:22:04 +0800 Subject: cmd/compile: fix FMA negative commutativity of riscv64 According to RISCV manual 11.6: FMADD x,y,z computes x*y+z and FNMADD x,y,z => -x*y-z FMSUB x,y,z => x*y-z FNMSUB x,y,z => -x*y+z respectively However our implement of SSA convert FMADD -x,y,z to FNMADD x,y,z which is wrong and should be convert to FNMSUB according to manual. Change-Id: Ib297bc83824e121fd7dda171ed56ea9694a4e575 Reviewed-on: https://go-review.googlesource.com/c/go/+/506575 Run-TryBot: M Zhuo Reviewed-by: Keith Randall Reviewed-by: David Chase Reviewed-by: Joedian Reid Reviewed-by: Michael Munday TryBot-Result: Gopher Robot --- src/cmd/compile/internal/ssa/_gen/RISCV64.rules | 10 +++++----- src/cmd/compile/internal/ssa/rewriteRISCV64.go | 16 ++++++++-------- test/codegen/math.go | 8 ++++---- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/cmd/compile/internal/ssa/_gen/RISCV64.rules b/src/cmd/compile/internal/ssa/_gen/RISCV64.rules index eb1f10de96..9a6fcebdc5 100644 --- a/src/cmd/compile/internal/ssa/_gen/RISCV64.rules +++ b/src/cmd/compile/internal/ssa/_gen/RISCV64.rules @@ -836,11 +836,11 @@ // // Key: // -// [+ -](x * y) [+ -] z. -// _ N A S -// D U -// D B +// [+ -](x * y [+ -] z). +// _ N A S +// D U +// D B // // Note: multiplication commutativity handled by rule generator. -(F(MADD|NMADD|MSUB|NMSUB)D neg:(FNEGD x) y z) && neg.Uses == 1 => (F(NMADD|MADD|NMSUB|MSUB)D x y z) +(F(MADD|NMADD|MSUB|NMSUB)D neg:(FNEGD x) y z) && neg.Uses == 1 => (F(NMSUB|MSUB|NMADD|MADD)D x y z) (F(MADD|NMADD|MSUB|NMSUB)D x y neg:(FNEGD z)) && neg.Uses == 1 => (F(MSUB|NMSUB|MADD|NMADD)D x y z) diff --git a/src/cmd/compile/internal/ssa/rewriteRISCV64.go b/src/cmd/compile/internal/ssa/rewriteRISCV64.go index f1debe0c21..ffbeb1df47 100644 --- a/src/cmd/compile/internal/ssa/rewriteRISCV64.go +++ b/src/cmd/compile/internal/ssa/rewriteRISCV64.go @@ -3322,7 +3322,7 @@ func rewriteValueRISCV64_OpRISCV64FMADDD(v *Value) bool { v_0 := v.Args[0] // match: (FMADDD neg:(FNEGD x) y z) // cond: neg.Uses == 1 - // result: (FNMADDD x y z) + // result: (FNMSUBD x y z) for { for _i0 := 0; _i0 <= 1; _i0, v_0, v_1 = _i0+1, v_1, v_0 { neg := v_0 @@ -3335,7 +3335,7 @@ func rewriteValueRISCV64_OpRISCV64FMADDD(v *Value) bool { if !(neg.Uses == 1) { continue } - v.reset(OpRISCV64FNMADDD) + v.reset(OpRISCV64FNMSUBD) v.AddArg3(x, y, z) return true } @@ -3367,7 +3367,7 @@ func rewriteValueRISCV64_OpRISCV64FMSUBD(v *Value) bool { v_0 := v.Args[0] // match: (FMSUBD neg:(FNEGD x) y z) // cond: neg.Uses == 1 - // result: (FNMSUBD x y z) + // result: (FNMADDD x y z) for { for _i0 := 0; _i0 <= 1; _i0, v_0, v_1 = _i0+1, v_1, v_0 { neg := v_0 @@ -3380,7 +3380,7 @@ func rewriteValueRISCV64_OpRISCV64FMSUBD(v *Value) bool { if !(neg.Uses == 1) { continue } - v.reset(OpRISCV64FNMSUBD) + v.reset(OpRISCV64FNMADDD) v.AddArg3(x, y, z) return true } @@ -3412,7 +3412,7 @@ func rewriteValueRISCV64_OpRISCV64FNMADDD(v *Value) bool { v_0 := v.Args[0] // match: (FNMADDD neg:(FNEGD x) y z) // cond: neg.Uses == 1 - // result: (FMADDD x y z) + // result: (FMSUBD x y z) for { for _i0 := 0; _i0 <= 1; _i0, v_0, v_1 = _i0+1, v_1, v_0 { neg := v_0 @@ -3425,7 +3425,7 @@ func rewriteValueRISCV64_OpRISCV64FNMADDD(v *Value) bool { if !(neg.Uses == 1) { continue } - v.reset(OpRISCV64FMADDD) + v.reset(OpRISCV64FMSUBD) v.AddArg3(x, y, z) return true } @@ -3457,7 +3457,7 @@ func rewriteValueRISCV64_OpRISCV64FNMSUBD(v *Value) bool { v_0 := v.Args[0] // match: (FNMSUBD neg:(FNEGD x) y z) // cond: neg.Uses == 1 - // result: (FMSUBD x y z) + // result: (FMADDD x y z) for { for _i0 := 0; _i0 <= 1; _i0, v_0, v_1 = _i0+1, v_1, v_0 { neg := v_0 @@ -3470,7 +3470,7 @@ func rewriteValueRISCV64_OpRISCV64FNMSUBD(v *Value) bool { if !(neg.Uses == 1) { continue } - v.reset(OpRISCV64FMSUBD) + v.reset(OpRISCV64FMADDD) v.AddArg3(x, y, z) return true } diff --git a/test/codegen/math.go b/test/codegen/math.go index 6a7d304afd..331ebbe609 100644 --- a/test/codegen/math.go +++ b/test/codegen/math.go @@ -143,13 +143,13 @@ func fms(x, y, z float64) float64 { return math.FMA(x, y, -z) } -func fnma(x, y, z float64) float64 { - // riscv64:"FNMADDD" +func fnms(x, y, z float64) float64 { + // riscv64:"FNMSUBD",-"FNMADDD" return math.FMA(-x, y, z) } -func fnms(x, y, z float64) float64 { - // riscv64:"FNMSUBD" +func fnma(x, y, z float64) float64 { + // riscv64:"FNMADDD",-"FNMSUBD" return math.FMA(x, -y, -z) } -- cgit v1.2.3-54-g00ecf From 36ea4f9680f8296f1c7d0cf7dbb1b3a9d572754a Mon Sep 17 00:00:00 2001 From: Merrick Clay Date: Wed, 5 Jul 2023 15:06:39 -0600 Subject: log/slog: fix faulty test Adds an optional close quote in the expected log message regex for TestConnections to prevent failing when the source filepath is surrounded in quotes due to it containing one or more spaces. Fixes #61161 Change-Id: I0dd71fb4389bff963bbfdc668ef4e4dfe787eafc Reviewed-on: https://go-review.googlesource.com/c/go/+/508055 Reviewed-by: Jonathan Amsterdam Reviewed-by: Bryan Mills Run-TryBot: Bryan Mills TryBot-Result: Gopher Robot --- src/log/slog/logger_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/log/slog/logger_test.go b/src/log/slog/logger_test.go index d151c0490c..130f2e6ac8 100644 --- a/src/log/slog/logger_test.go +++ b/src/log/slog/logger_test.go @@ -106,7 +106,7 @@ func TestConnections(t *testing.T) { // log.Logger's output goes through the handler. SetDefault(New(NewTextHandler(&slogbuf, &HandlerOptions{AddSource: true}))) log.Print("msg2") - checkLogOutput(t, slogbuf.String(), "time="+timeRE+` level=INFO source=.*logger_test.go:\d{3} msg=msg2`) + checkLogOutput(t, slogbuf.String(), "time="+timeRE+` level=INFO source=.*logger_test.go:\d{3}"? msg=msg2`) // The default log.Logger always outputs at Info level. slogbuf.Reset() -- cgit v1.2.3-54-g00ecf From b490bdc27d5576e5ccdac33755c0156d609e1bb9 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Wed, 5 Jul 2023 12:08:51 -0400 Subject: go/types: record Config.GoVersion for reporting in Package.GoVersion method Clients of go/types, such as analyzers, may need to know which specific Go version a package is written for. Record that information in the Package and expose it using the new GoVersion method. Update parseGoVersion to handle the new Go versions that may be passed around starting in Go 1.21.0: versions like "go1.21.0" and "go1.21rc2". This is not strictly necessary today, but it adds some valuable future-proofing. While we are here, change NewChecker from panicking on invalid version to saving an error for returning later from Files. Go versions are now likely to be coming from a variety of sources, not just hard-coded in calls to NewChecker, making a panic inappropriate. For #61174. Fixes #61175. Change-Id: Ibe41fe207c1b6e71064b1fe448ac55776089c541 Reviewed-on: https://go-review.googlesource.com/c/go/+/507975 Run-TryBot: Russ Cox TryBot-Result: Gopher Robot Reviewed-by: Bryan Mills Reviewed-by: Robert Findley --- api/go1.21.txt | 1 + src/cmd/compile/internal/types2/package.go | 21 ++++++++----- src/cmd/compile/internal/types2/sizeof_test.go | 2 +- src/cmd/compile/internal/types2/version.go | 23 ++++++++------- src/go/build/deps_test.go | 2 +- src/go/types/check.go | 41 ++++++++++++++++---------- src/go/types/package.go | 21 ++++++++----- src/go/types/sizeof_test.go | 2 +- src/go/types/version.go | 23 ++++++++------- src/go/types/version_test.go | 24 +++++++++++++++ 10 files changed, 105 insertions(+), 55 deletions(-) create mode 100644 src/go/types/version_test.go diff --git a/api/go1.21.txt b/api/go1.21.txt index 6435d10914..c8ca3df2e6 100644 --- a/api/go1.21.txt +++ b/api/go1.21.txt @@ -174,6 +174,7 @@ pkg go/build, type Package struct, Directives []Directive #56986 pkg go/build, type Package struct, TestDirectives []Directive #56986 pkg go/build, type Package struct, XTestDirectives []Directive #56986 pkg go/token, method (*File) Lines() []int #57708 +pkg go/types, method (*Package) GoVersion() string #61175 pkg html/template, const ErrJSTemplate = 12 #59584 pkg html/template, const ErrJSTemplate ErrorCode #59584 pkg io/fs, func FormatDirEntry(DirEntry) string #54451 diff --git a/src/cmd/compile/internal/types2/package.go b/src/cmd/compile/internal/types2/package.go index 61670f6718..e08099d81f 100644 --- a/src/cmd/compile/internal/types2/package.go +++ b/src/cmd/compile/internal/types2/package.go @@ -10,13 +10,14 @@ import ( // A Package describes a Go package. type Package struct { - path string - name string - scope *Scope - imports []*Package - complete bool - fake bool // scope lookup errors are silently dropped if package is fake (internal use only) - cgo bool // uses of this package will be rewritten into uses of declarations from _cgo_gotypes.go + path string + name string + scope *Scope + imports []*Package + complete bool + fake bool // scope lookup errors are silently dropped if package is fake (internal use only) + cgo bool // uses of this package will be rewritten into uses of declarations from _cgo_gotypes.go + goVersion string // minimum Go version required for package (by Config.GoVersion, typically from go.mod) } // NewPackage returns a new Package for the given package path and name. @@ -35,6 +36,12 @@ func (pkg *Package) Name() string { return pkg.name } // SetName sets the package name. func (pkg *Package) SetName(name string) { pkg.name = name } +// GoVersion returns the minimum Go version required by this package. +// If the minimum version is unknown, GoVersion returns the empty string. +// Individual source files may specify a different minimum Go version, +// as reported in the [go/ast.File.GoVersion] field. +func (pkg *Package) GoVersion() string { return pkg.goVersion } + // Scope returns the (complete or incomplete) package scope // holding the objects declared at package level (TypeNames, // Consts, Vars, and Funcs). diff --git a/src/cmd/compile/internal/types2/sizeof_test.go b/src/cmd/compile/internal/types2/sizeof_test.go index af82b3fa7a..740dbc9276 100644 --- a/src/cmd/compile/internal/types2/sizeof_test.go +++ b/src/cmd/compile/internal/types2/sizeof_test.go @@ -47,7 +47,7 @@ func TestSizeof(t *testing.T) { // Misc {Scope{}, 60, 104}, - {Package{}, 36, 72}, + {Package{}, 44, 88}, {_TypeSet{}, 28, 56}, } diff --git a/src/cmd/compile/internal/types2/version.go b/src/cmd/compile/internal/types2/version.go index 7d01b829a9..e525f16470 100644 --- a/src/cmd/compile/internal/types2/version.go +++ b/src/cmd/compile/internal/types2/version.go @@ -6,7 +6,6 @@ package types2 import ( "cmd/compile/internal/syntax" - "errors" "fmt" "strings" ) @@ -44,23 +43,24 @@ var ( go1_21 = version{1, 21} ) -var errVersionSyntax = errors.New("invalid Go version syntax") - // parseGoVersion parses a Go version string (such as "go1.12") // and returns the version, or an error. If s is the empty // string, the version is 0.0. func parseGoVersion(s string) (v version, err error) { + bad := func() (version, error) { + return version{}, fmt.Errorf("invalid Go version syntax %q", s) + } if s == "" { return } if !strings.HasPrefix(s, "go") { - return version{}, errVersionSyntax + return bad() } s = s[len("go"):] i := 0 for ; i < len(s) && '0' <= s[i] && s[i] <= '9'; i++ { if i >= 10 || i == 0 && s[i] == '0' { - return version{}, errVersionSyntax + return bad() } v.major = 10*v.major + int(s[i]) - '0' } @@ -68,7 +68,7 @@ func parseGoVersion(s string) (v version, err error) { return } if i == 0 || s[i] != '.' { - return version{}, errVersionSyntax + return bad() } s = s[i+1:] if s == "0" { @@ -81,14 +81,15 @@ func parseGoVersion(s string) (v version, err error) { i = 0 for ; i < len(s) && '0' <= s[i] && s[i] <= '9'; i++ { if i >= 10 || i == 0 && s[i] == '0' { - return version{}, errVersionSyntax + return bad() } v.minor = 10*v.minor + int(s[i]) - '0' } - if i > 0 && i == len(s) { - return - } - return version{}, errVersionSyntax + // Accept any suffix after the minor number. + // We are only looking for the language version (major.minor) + // but want to accept any valid Go version, like go1.21.0 + // and go1.21rc2. + return } // langCompat reports an error if the representation of a numeric diff --git a/src/go/build/deps_test.go b/src/go/build/deps_test.go index be8ac30f9d..2f335068b8 100644 --- a/src/go/build/deps_test.go +++ b/src/go/build/deps_test.go @@ -286,7 +286,7 @@ var depsRules = ` math/big, go/token < go/constant; - container/heap, go/constant, go/parser, internal/types/errors + container/heap, go/constant, go/parser, internal/goversion, internal/types/errors < go/types; # The vast majority of standard library packages should not be resorting to regexp. diff --git a/src/go/types/check.go b/src/go/types/check.go index 5381b5db68..591de5f329 100644 --- a/src/go/types/check.go +++ b/src/go/types/check.go @@ -12,6 +12,7 @@ import ( "go/ast" "go/constant" "go/token" + "internal/goversion" . "internal/types/errors" ) @@ -98,11 +99,12 @@ type Checker struct { fset *token.FileSet pkg *Package *Info - version version // accepted language version - nextID uint64 // unique Id for type parameters (first valid Id is 1) - objMap map[Object]*declInfo // maps package-level objects and (non-interface) methods to declaration info - impMap map[importKey]*Package // maps (import path, source directory) to (complete or fake) package - valids instanceLookup // valid *Named (incl. instantiated) types per the validType check + version version // accepted language version + versionErr error // version error, delayed from NewChecker + nextID uint64 // unique Id for type parameters (first valid Id is 1) + objMap map[Object]*declInfo // maps package-level objects and (non-interface) methods to declaration info + impMap map[importKey]*Package // maps (import path, source directory) to (complete or fake) package + valids instanceLookup // valid *Named (incl. instantiated) types per the validType check // pkgPathMap maps package names to the set of distinct import paths we've // seen for that name, anywhere in the import graph. It is used for @@ -233,20 +235,21 @@ func NewChecker(conf *Config, fset *token.FileSet, pkg *Package, info *Info) *Ch info = new(Info) } - version, err := parseGoVersion(conf.GoVersion) - if err != nil { - panic(fmt.Sprintf("invalid Go version %q (%v)", conf.GoVersion, err)) + version, versionErr := parseGoVersion(conf.GoVersion) + if pkg != nil { + pkg.goVersion = conf.GoVersion } return &Checker{ - conf: conf, - ctxt: conf.Context, - fset: fset, - pkg: pkg, - Info: info, - version: version, - objMap: make(map[Object]*declInfo), - impMap: make(map[importKey]*Package), + conf: conf, + ctxt: conf.Context, + fset: fset, + pkg: pkg, + Info: info, + version: version, + versionErr: versionErr, + objMap: make(map[Object]*declInfo), + impMap: make(map[importKey]*Package), } } @@ -342,6 +345,12 @@ func (check *Checker) Files(files []*ast.File) error { return check.checkFiles(f var errBadCgo = errors.New("cannot use FakeImportC and go115UsesCgo together") func (check *Checker) checkFiles(files []*ast.File) (err error) { + if check.versionErr != nil { + return check.versionErr + } + if check.version.after(version{1, goversion.Version}) { + return fmt.Errorf("package requires newer Go version %v", check.version) + } if check.conf.FakeImportC && check.conf.go115UsesCgo { return errBadCgo } diff --git a/src/go/types/package.go b/src/go/types/package.go index 7aa62fb7a3..0f52d5f489 100644 --- a/src/go/types/package.go +++ b/src/go/types/package.go @@ -12,13 +12,14 @@ import ( // A Package describes a Go package. type Package struct { - path string - name string - scope *Scope - imports []*Package - complete bool - fake bool // scope lookup errors are silently dropped if package is fake (internal use only) - cgo bool // uses of this package will be rewritten into uses of declarations from _cgo_gotypes.go + path string + name string + scope *Scope + imports []*Package + complete bool + fake bool // scope lookup errors are silently dropped if package is fake (internal use only) + cgo bool // uses of this package will be rewritten into uses of declarations from _cgo_gotypes.go + goVersion string // minimum Go version required for package (by Config.GoVersion, typically from go.mod) } // NewPackage returns a new Package for the given package path and name. @@ -37,6 +38,12 @@ func (pkg *Package) Name() string { return pkg.name } // SetName sets the package name. func (pkg *Package) SetName(name string) { pkg.name = name } +// GoVersion returns the minimum Go version required by this package. +// If the minimum version is unknown, GoVersion returns the empty string. +// Individual source files may specify a different minimum Go version, +// as reported in the [go/ast.File.GoVersion] field. +func (pkg *Package) GoVersion() string { return pkg.goVersion } + // Scope returns the (complete or incomplete) package scope // holding the objects declared at package level (TypeNames, // Consts, Vars, and Funcs). diff --git a/src/go/types/sizeof_test.go b/src/go/types/sizeof_test.go index f17a1781f5..9e5b5f8b20 100644 --- a/src/go/types/sizeof_test.go +++ b/src/go/types/sizeof_test.go @@ -46,7 +46,7 @@ func TestSizeof(t *testing.T) { // Misc {Scope{}, 44, 88}, - {Package{}, 36, 72}, + {Package{}, 44, 88}, {_TypeSet{}, 28, 56}, } for _, test := range tests { diff --git a/src/go/types/version.go b/src/go/types/version.go index 07a42a79ee..108d9b34a0 100644 --- a/src/go/types/version.go +++ b/src/go/types/version.go @@ -5,7 +5,6 @@ package types import ( - "errors" "fmt" "go/ast" "go/token" @@ -45,23 +44,24 @@ var ( go1_21 = version{1, 21} ) -var errVersionSyntax = errors.New("invalid Go version syntax") - // parseGoVersion parses a Go version string (such as "go1.12") // and returns the version, or an error. If s is the empty // string, the version is 0.0. func parseGoVersion(s string) (v version, err error) { + bad := func() (version, error) { + return version{}, fmt.Errorf("invalid Go version syntax %q", s) + } if s == "" { return } if !strings.HasPrefix(s, "go") { - return version{}, errVersionSyntax + return bad() } s = s[len("go"):] i := 0 for ; i < len(s) && '0' <= s[i] && s[i] <= '9'; i++ { if i >= 10 || i == 0 && s[i] == '0' { - return version{}, errVersionSyntax + return bad() } v.major = 10*v.major + int(s[i]) - '0' } @@ -69,7 +69,7 @@ func parseGoVersion(s string) (v version, err error) { return } if i == 0 || s[i] != '.' { - return version{}, errVersionSyntax + return bad() } s = s[i+1:] if s == "0" { @@ -82,14 +82,15 @@ func parseGoVersion(s string) (v version, err error) { i = 0 for ; i < len(s) && '0' <= s[i] && s[i] <= '9'; i++ { if i >= 10 || i == 0 && s[i] == '0' { - return version{}, errVersionSyntax + return bad() } v.minor = 10*v.minor + int(s[i]) - '0' } - if i > 0 && i == len(s) { - return - } - return version{}, errVersionSyntax + // Accept any suffix after the minor number. + // We are only looking for the language version (major.minor) + // but want to accept any valid Go version, like go1.21.0 + // and go1.21rc2. + return } // langCompat reports an error if the representation of a numeric diff --git a/src/go/types/version_test.go b/src/go/types/version_test.go new file mode 100644 index 0000000000..dc9becf9e1 --- /dev/null +++ b/src/go/types/version_test.go @@ -0,0 +1,24 @@ +// Copyright 2023 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 types + +import "testing" + +var parseGoVersionTests = []struct { + in string + out version +}{ + {"go1.21", version{1, 21}}, + {"go1.21.0", version{1, 21}}, + {"go1.21rc2", version{1, 21}}, +} + +func TestParseGoVersion(t *testing.T) { + for _, tt := range parseGoVersionTests { + if out, err := parseGoVersion(tt.in); out != tt.out || err != nil { + t.Errorf("parseGoVersion(%q) = %v, %v, want %v, nil", tt.in, out, err, tt.out) + } + } +} -- cgit v1.2.3-54-g00ecf From 6305d7fdd3a5c9d50010c04f4c418444517082ab Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Wed, 5 Jul 2023 12:12:35 -0400 Subject: cmd/go: pass GoVersion in vet config When invoking a vet tool with -vettool (or vet itself), we need to pass the package's GoVersion to use when analyzing the package. The test of this behavior is in the x/tools/go/analysis CL 507880. For #61176. Change-Id: I3b5a90fcd19a0adc7fb29366e106e18f722fc061 Reviewed-on: https://go-review.googlesource.com/c/go/+/507976 TryBot-Result: Gopher Robot Reviewed-by: Bryan Mills Run-TryBot: Russ Cox --- src/cmd/go/internal/work/exec.go | 8 ++++++++ src/cmd/go/internal/work/gc.go | 14 +------------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go index d38a051b2b..13d2a78a97 100644 --- a/src/cmd/go/internal/work/exec.go +++ b/src/cmd/go/internal/work/exec.go @@ -1115,6 +1115,7 @@ type vetConfig struct { PackageVetx map[string]string // map package path to vetx data from earlier vet run VetxOnly bool // only compute vetx data; don't report detected problems VetxOutput string // write vetx data to this output file + GoVersion string // Go version for package SucceedOnTypecheckFailure bool // awful hack; see #18395 and below } @@ -1149,6 +1150,13 @@ func buildVetConfig(a *Action, srcfiles []string) { PackageFile: make(map[string]string), Standard: make(map[string]bool), } + if a.Package.Module != nil { + v := a.Package.Module.GoVersion + if v == "" { + v = gover.DefaultGoModVersion + } + vcfg.GoVersion = "go" + v + } a.vetCfg = vcfg for i, raw := range a.Package.Internal.RawImports { final := a.Package.Imports[i] diff --git a/src/cmd/go/internal/work/gc.go b/src/cmd/go/internal/work/gc.go index 6043ad5353..26b4e0f490 100644 --- a/src/cmd/go/internal/work/gc.go +++ b/src/cmd/go/internal/work/gc.go @@ -85,19 +85,7 @@ func (gcToolchain) gc(b *Builder, a *Action, archive string, importcfg, embedcfg if p.Module != nil { v := p.Module.GoVersion if v == "" { - // We started adding a 'go' directive to the go.mod file unconditionally - // as of Go 1.12, so any module that still lacks such a directive must - // either have been authored before then, or have a hand-edited go.mod - // file that hasn't been updated by cmd/go since that edit. - // - // Unfortunately, through at least Go 1.16 we didn't add versions to - // vendor/modules.txt. So this could also be a vendored 1.16 dependency. - // - // Fortunately, there were no breaking changes to the language between Go - // 1.11 and 1.16, so if we assume Go 1.16 semantics we will not introduce - // any spurious errors — we will only mask errors, and not particularly - // important ones at that. - v = "1.16" + v = gover.DefaultGoModVersion } if allowedVersion(v) { defaultGcFlags = append(defaultGcFlags, "-lang=go"+gover.Lang(v)) -- cgit v1.2.3-54-g00ecf From 449ef3795d8054faf4a601d8d1aab1f624b822f5 Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Mon, 3 Jul 2023 05:05:55 -0700 Subject: net: only build cgo_stub.go on unix or wasip1 We were building it for Windows, although Windows code never calls any of these functions. When using -tags netgo that cause a multiple definition of cgoAvailable. Fixes #61153 Change-Id: Ib9e1de7720a8c0dacd6f12002917bf305dfa5405 Reviewed-on: https://go-review.googlesource.com/c/go/+/507655 Reviewed-by: Damien Neil Auto-Submit: Ian Lance Taylor TryBot-Result: Gopher Robot Run-TryBot: Ian Lance Taylor Reviewed-by: Dmitri Shuralyov Reviewed-by: Bryan Mills Reviewed-by: Dmitri Shuralyov --- src/net/cgo_stub.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/net/cgo_stub.go b/src/net/cgo_stub.go index a8514c19f8..b26b11af8b 100644 --- a/src/net/cgo_stub.go +++ b/src/net/cgo_stub.go @@ -3,13 +3,13 @@ // license that can be found in the LICENSE file. // This file holds stub versions of the cgo functions called on Unix systems. -// We build this file if using the netgo build tag, or if cgo is not -// enabled and we are using a Unix system other than Darwin, or if it's -// wasip1 where cgo is never available. -// Darwin is exempted because it always provides the cgo routines, -// in cgo_unix_syscall.go. +// We build this file: +// - if using the netgo build tag on a Unix system +// - on a Unix system without the cgo resolver functions +// (Darwin always provides the cgo functions, in cgo_unix_syscall.go) +// - on wasip1, where cgo is never available -//go:build netgo || (!cgo && unix && !darwin) || wasip1 +//go:build (netgo && unix) || (unix && !cgo && !darwin) || wasip1 package net -- cgit v1.2.3-54-g00ecf From e6ec2a34dc1e1c319588fb0cb449abf55291977f Mon Sep 17 00:00:00 2001 From: Cherry Mui Date: Wed, 5 Jul 2023 16:03:55 -0400 Subject: runtime: print output on failure in TestMemPprof If running testprog fails, print the output. For #60901. Change-Id: Iee80fb09412ce13bae6ac3589779a8cdf7761765 Reviewed-on: https://go-review.googlesource.com/c/go/+/507978 Reviewed-by: Michael Pratt Reviewed-by: Michael Knyszek TryBot-Result: Gopher Robot Run-TryBot: Cherry Mui --- src/runtime/crash_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/crash_test.go b/src/runtime/crash_test.go index cd978cc34b..8f11333b46 100644 --- a/src/runtime/crash_test.go +++ b/src/runtime/crash_test.go @@ -534,7 +534,7 @@ func TestMemPprof(t *testing.T) { got, err := testenv.CleanCmdEnv(exec.Command(exe, "MemProf")).CombinedOutput() if err != nil { - t.Fatal(err) + t.Fatalf("testprog failed: %s, output:\n%s", err, got) } fn := strings.TrimSpace(string(got)) defer os.Remove(fn) -- cgit v1.2.3-54-g00ecf From 39c507071251dbf5ea098077ad0e791679dae548 Mon Sep 17 00:00:00 2001 From: Achille Roussel Date: Tue, 4 Jul 2023 18:15:58 -0700 Subject: os: do not skip directory entries with zero inodes on wasip1 When building programs to GOOS=wasip1, the program does not have the guarantees that the underlying directories will come from a file system where a zero inode value indicates that the entry was deleted but not yet removed from the directory. The host runtime may be running on windows or may be exposing virtual user-space file systems that do not have the concept of inodes. In those setup, we assume that the host runtime is in charge of dealing with edge cases such as skipping directory entries with zero inodes when needed, and the guest application should trust the list of entries that it sees; therefore, we disable skipping over zero inodes on wasip1. Change-Id: I99aa562441cdb4182965f270af054cf3cf7f8f20 Reviewed-on: https://go-review.googlesource.com/c/go/+/507915 Reviewed-by: Ian Lance Taylor Run-TryBot: Ian Lance Taylor Auto-Submit: Ian Lance Taylor TryBot-Result: Gopher Robot Reviewed-by: Dmitri Shuralyov Reviewed-by: Johan Brandhorst-Satzkorn --- src/os/dir_darwin.go | 9 +++++++++ src/os/dir_unix.go | 6 +++++- src/syscall/dirent.go | 9 +++++++-- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/os/dir_darwin.go b/src/os/dir_darwin.go index deba3eb37f..e6d5bda24b 100644 --- a/src/os/dir_darwin.go +++ b/src/os/dir_darwin.go @@ -54,6 +54,15 @@ func (f *File) readdir(n int, mode readdirMode) (names []string, dirents []DirEn if entptr == nil { // EOF break } + // Darwin may return a zero inode when a directory entry has been + // deleted but not yet removed from the directory. The man page for + // getdirentries(2) states that programs are responsible for skipping + // those entries: + // + // Users of getdirentries() should skip entries with d_fileno = 0, + // as such entries represent files which have been deleted but not + // yet removed from the directory entry. + // if dirent.Ino == 0 { continue } diff --git a/src/os/dir_unix.go b/src/os/dir_unix.go index 004b9fbb2b..266a78acaf 100644 --- a/src/os/dir_unix.go +++ b/src/os/dir_unix.go @@ -89,7 +89,11 @@ func (f *File) readdir(n int, mode readdirMode) (names []string, dirents []DirEn if !ok { break } - if ino == 0 { + // When building to wasip1, the host runtime might be running on Windows + // or might expose a remote file system which does not have the concept + // of inodes. Therefore, we cannot make the assumption that it is safe + // to skip entries with zero inodes. + if ino == 0 && runtime.GOOS != "wasip1" { continue } const namoff = uint64(unsafe.Offsetof(syscall.Dirent{}.Name)) diff --git a/src/syscall/dirent.go b/src/syscall/dirent.go index eee94bf73c..1a0f1eec11 100644 --- a/src/syscall/dirent.go +++ b/src/syscall/dirent.go @@ -6,7 +6,10 @@ package syscall -import "unsafe" +import ( + "runtime" + "unsafe" +) // readInt returns the size-bytes unsigned integer in native byte order at offset off. func readInt(b []byte, off, size uintptr) (u uint64, ok bool) { @@ -75,7 +78,9 @@ func ParseDirent(buf []byte, max int, names []string) (consumed int, count int, if !ok { break } - if ino == 0 { // File absent in directory. + // See src/os/dir_unix.go for the reason why this condition is + // excluded on wasip1. + if ino == 0 && runtime.GOOS != "wasip1" { // File absent in directory. continue } const namoff = uint64(unsafe.Offsetof(Dirent{}.Name)) -- cgit v1.2.3-54-g00ecf From d3d78b4bcc7c4021c4a3a8a3ecdb85ec59bdd58b Mon Sep 17 00:00:00 2001 From: Jonathan Amsterdam Date: Thu, 6 Jul 2023 12:30:39 -0400 Subject: log/slog: handle recursively empty groups Handlers should not display empty groups. A group with no attributes is certainly empty. But we also want to consider a group to be empty if all its attributes are empty groups. The built-in handlers did not handle this second case properly. This CL fixes that. There are two places in the implementation that we need to consider. For Values of KindGroup, we change the GroupValue constructor to omit Attrs that are empty groups. A Group is then empty if and only if it has no Attrs. This avoids a recursive check for emptiness. It does require allocation, but that doesn't worry us because Group values should be relatively rare. For groups established by WithGroup, we avoid opening such groups unless the Record contains non-empty groups. As we did for values, we avoid adding empty groups to records in the first place, so we only need to check that the record has at least one Attr. We are doing extra work, so we need to make sure we aren't slowing things down unduly. Benchmarks before and after this change show minimal differences. Fixes #61067. Change-Id: I684c7ca834bbf69210516faecae04ee548846fb7 Reviewed-on: https://go-review.googlesource.com/c/go/+/508436 Run-TryBot: Jonathan Amsterdam Reviewed-by: Alan Donovan TryBot-Result: Gopher Robot --- src/log/slog/handler.go | 24 ++++++++++++++------- src/log/slog/handler_test.go | 50 ++++++++++++++++++++++++++++++++++++++++++++ src/log/slog/record.go | 24 ++++++++++++++++++--- src/log/slog/value.go | 34 ++++++++++++++++++++++++++++++ src/log/slog/value_test.go | 12 +++++++++++ 5 files changed, 134 insertions(+), 10 deletions(-) diff --git a/src/log/slog/handler.go b/src/log/slog/handler.go index dc4c2d92bd..e479ca8a4c 100644 --- a/src/log/slog/handler.go +++ b/src/log/slog/handler.go @@ -221,6 +221,11 @@ func (h *commonHandler) enabled(l Level) bool { } func (h *commonHandler) withAttrs(as []Attr) *commonHandler { + // We are going to ignore empty groups, so if the entire slice consists of + // them, there is nothing to do. + if countEmptyGroups(as) == len(as) { + return h + } h2 := h.clone() // Pre-format the attributes as an optimization. state := h2.newHandleState((*buffer.Buffer)(&h2.preformattedAttrs), false, "") @@ -308,15 +313,20 @@ func (s *handleState) appendNonBuiltIns(r Record) { } // Attrs in Record -- unlike the built-in ones, they are in groups started // from WithGroup. - s.prefix.WriteString(s.h.groupPrefix) - s.openGroups() - r.Attrs(func(a Attr) bool { - s.appendAttr(a) - return true - }) + // If the record has no Attrs, don't output any groups. + nOpenGroups := s.h.nOpenGroups + if r.NumAttrs() > 0 { + s.prefix.WriteString(s.h.groupPrefix) + s.openGroups() + nOpenGroups = len(s.h.groups) + r.Attrs(func(a Attr) bool { + s.appendAttr(a) + return true + }) + } if s.h.json { // Close all open groups. - for range s.h.groups { + for range s.h.groups[:nOpenGroups] { s.buf.WriteByte('}') } // Close the top-level object. diff --git a/src/log/slog/handler_test.go b/src/log/slog/handler_test.go index 741e86a826..f43d841483 100644 --- a/src/log/slog/handler_test.go +++ b/src/log/slog/handler_test.go @@ -214,6 +214,28 @@ func TestJSONAndTextHandlers(t *testing.T) { wantText: "msg=message h.a=1", wantJSON: `{"msg":"message","h":{"a":1}}`, }, + { + name: "nested empty group", + replace: removeKeys(TimeKey, LevelKey), + attrs: []Attr{ + Group("g", + Group("h", + Group("i"), Group("j"))), + }, + wantText: `msg=message`, + wantJSON: `{"msg":"message"}`, + }, + { + name: "nested non-empty group", + replace: removeKeys(TimeKey, LevelKey), + attrs: []Attr{ + Group("g", + Group("h", + Group("i"), Group("j", Int("a", 1)))), + }, + wantText: `msg=message g.h.j.a=1`, + wantJSON: `{"msg":"message","g":{"h":{"j":{"a":1}}}}`, + }, { name: "escapes", replace: removeKeys(TimeKey, LevelKey), @@ -281,6 +303,34 @@ func TestJSONAndTextHandlers(t *testing.T) { wantText: "msg=message p1=1 s1.s2.a=one s1.s2.b=2", wantJSON: `{"msg":"message","p1":1,"s1":{"s2":{"a":"one","b":2}}}`, }, + { + name: "empty with-groups", + replace: removeKeys(TimeKey, LevelKey), + with: func(h Handler) Handler { + return h.WithGroup("x").WithGroup("y") + }, + wantText: "msg=message", + wantJSON: `{"msg":"message"}`, + }, + { + name: "empty with-groups, no non-empty attrs", + replace: removeKeys(TimeKey, LevelKey), + with: func(h Handler) Handler { + return h.WithGroup("x").WithAttrs([]Attr{Group("g")}).WithGroup("y") + }, + wantText: "msg=message", + wantJSON: `{"msg":"message"}`, + }, + { + name: "one empty with-group", + replace: removeKeys(TimeKey, LevelKey), + with: func(h Handler) Handler { + return h.WithGroup("x").WithAttrs([]Attr{Int("a", 1)}).WithGroup("y") + }, + attrs: []Attr{Group("g", Group("h"))}, + wantText: "msg=message x.a=1", + wantJSON: `{"msg":"message","x":{"a":1}}`, + }, { name: "GroupValue as Attr value", replace: removeKeys(TimeKey, LevelKey), diff --git a/src/log/slog/record.go b/src/log/slog/record.go index 972552d519..67b76f34e1 100644 --- a/src/log/slog/record.go +++ b/src/log/slog/record.go @@ -93,9 +93,17 @@ func (r Record) Attrs(f func(Attr) bool) { } // AddAttrs appends the given Attrs to the Record's list of Attrs. +// It omits empty groups. func (r *Record) AddAttrs(attrs ...Attr) { - n := copy(r.front[r.nFront:], attrs) - r.nFront += n + var i int + for i = 0; i < len(attrs) && r.nFront < len(r.front); i++ { + a := attrs[i] + if a.Value.isEmptyGroup() { + continue + } + r.front[r.nFront] = a + r.nFront++ + } // Check if a copy was modified by slicing past the end // and seeing if the Attr there is non-zero. if cap(r.back) > len(r.back) { @@ -104,15 +112,25 @@ func (r *Record) AddAttrs(attrs ...Attr) { panic("copies of a slog.Record were both modified") } } - r.back = append(r.back, attrs[n:]...) + ne := countEmptyGroups(attrs[i:]) + r.back = slices.Grow(r.back, len(attrs[i:])-ne) + for _, a := range attrs[i:] { + if !a.Value.isEmptyGroup() { + r.back = append(r.back, a) + } + } } // Add converts the args to Attrs as described in [Logger.Log], // then appends the Attrs to the Record's list of Attrs. +// It omits empty groups. func (r *Record) Add(args ...any) { var a Attr for len(args) > 0 { a, args = argsToAttr(args) + if a.Value.isEmptyGroup() { + continue + } if r.nFront < len(r.front) { r.front[r.nFront] = a r.nFront++ diff --git a/src/log/slog/value.go b/src/log/slog/value.go index 71a59d2639..224848f695 100644 --- a/src/log/slog/value.go +++ b/src/log/slog/value.go @@ -164,9 +164,32 @@ func DurationValue(v time.Duration) Value { // GroupValue returns a new Value for a list of Attrs. // The caller must not subsequently mutate the argument slice. func GroupValue(as ...Attr) Value { + // Remove empty groups. + // It is simpler overall to do this at construction than + // to check each Group recursively for emptiness. + if n := countEmptyGroups(as); n > 0 { + as2 := make([]Attr, 0, len(as)-n) + for _, a := range as { + if !a.Value.isEmptyGroup() { + as2 = append(as2, a) + } + } + as = as2 + } return Value{num: uint64(len(as)), any: groupptr(unsafe.SliceData(as))} } +// countEmptyGroups returns the number of empty group values in its argument. +func countEmptyGroups(as []Attr) int { + n := 0 + for _, a := range as { + if a.Value.isEmptyGroup() { + n++ + } + } + return n +} + // AnyValue returns a Value for the supplied value. // // If the supplied value is of type Value, it is returned @@ -399,6 +422,17 @@ func (v Value) Equal(w Value) bool { } } +// isEmptyGroup reports whether v is a group that has no attributes. +func (v Value) isEmptyGroup() bool { + if v.Kind() != KindGroup { + return false + } + // We do not need to recursively examine the group's Attrs for emptiness, + // because GroupValue removed them when the group was constructed, and + // groups are immutable. + return len(v.group()) == 0 +} + // append appends a text representation of v to dst. // v is formatted as with fmt.Sprint. func (v Value) append(dst []byte) []byte { diff --git a/src/log/slog/value_test.go b/src/log/slog/value_test.go index 615bed79d9..923a4e0ccc 100644 --- a/src/log/slog/value_test.go +++ b/src/log/slog/value_test.go @@ -229,6 +229,18 @@ func TestZeroTime(t *testing.T) { } } +func TestEmptyGroup(t *testing.T) { + g := GroupValue( + Int("a", 1), + Group("g1", Group("g2")), + Group("g3", Group("g4", Int("b", 2)))) + got := g.Group() + want := []Attr{Int("a", 1), Group("g3", Group("g4", Int("b", 2)))} + if !attrsEqual(got, want) { + t.Errorf("\ngot %v\nwant %v", got, want) + } +} + type replace struct { v Value } -- cgit v1.2.3-54-g00ecf From 158d11196f732e4c80b03240548bdd373e6a9eff Mon Sep 17 00:00:00 2001 From: Michael Munday Date: Wed, 28 Jun 2023 23:12:33 +0100 Subject: math: add test that covers riscv64 fnm{add,sub} codegen Adds a test that triggers the RISC-V fused multiply-add code generation bug fixed by CL 506575. Change-Id: Ia3a55a68b48c5cc6beac4e5235975dea31f3faf2 Reviewed-on: https://go-review.googlesource.com/c/go/+/507035 Auto-Submit: M Zhuo Reviewed-by: M Zhuo Run-TryBot: Michael Munday TryBot-Result: Gopher Robot Reviewed-by: Dmitri Shuralyov Reviewed-by: Joedian Reid Reviewed-by: Keith Randall --- src/math/all_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/src/math/all_test.go b/src/math/all_test.go index 96a398e9c6..af3c38c2a6 100644 --- a/src/math/all_test.go +++ b/src/math/all_test.go @@ -3106,6 +3106,45 @@ func TestFMA(t *testing.T) { } } +//go:noinline +func fmsub(x, y, z float64) float64 { + return FMA(x, y, -z) +} + +//go:noinline +func fnmsub(x, y, z float64) float64 { + return FMA(-x, y, z) +} + +//go:noinline +func fnmadd(x, y, z float64) float64 { + return FMA(-x, y, -z) +} + +func TestFMANegativeArgs(t *testing.T) { + // Some architectures have instructions for fused multiply-subtract and + // also negated variants of fused multiply-add and subtract. This test + // aims to check that the optimizations that generate those instructions + // are applied correctly, if they exist. + for _, c := range fmaC { + want := PortableFMA(c.x, c.y, -c.z) + got := fmsub(c.x, c.y, c.z) + if !alike(got, want) { + t.Errorf("FMA(%g, %g, -(%g)) == %g, want %g", c.x, c.y, c.z, got, want) + } + want = PortableFMA(-c.x, c.y, c.z) + got = fnmsub(c.x, c.y, c.z) + if !alike(got, want) { + t.Errorf("FMA(-(%g), %g, %g) == %g, want %g", c.x, c.y, c.z, got, want) + } + want = PortableFMA(-c.x, c.y, -c.z) + got = fnmadd(c.x, c.y, c.z) + if !alike(got, want) { + t.Errorf("FMA(-(%g), %g, -(%g)) == %g, want %g", c.x, c.y, c.z, got, want) + } + } +} + // Check that math functions of high angle values // return accurate results. [Since (vf[i] + large) - large != vf[i], // testing for Trig(vf[i] + large) == Trig(vf[i]), where large is -- cgit v1.2.3-54-g00ecf From ac9137f8d312c3a6916ab71de61b05c192c455f0 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Fri, 7 Jul 2023 11:20:16 -0400 Subject: go/types, types2: do not mutate arguments in NewChecker CL 507975 resulted in new data races (as reported in #61212), because the pkg argument to NewChecker was mutated. Fix this by deferring the recording of the goVersion in pkg until type checking is actually initiated via a call to Checker.Files. Additionally, modify types2/check.go to bring it in sync with the changes in go/types/check.go, and generate the new version_test.go from the types2 file. Also move parsing the version into checkFiles, for simplicity. Fixes #61212 Change-Id: I15edb6c2cff3085622fe7c6a3b0dab531d27bd04 Reviewed-on: https://go-review.googlesource.com/c/go/+/508439 Run-TryBot: Robert Findley TryBot-Result: Gopher Robot Reviewed-by: Alan Donovan --- src/cmd/compile/internal/types2/check.go | 38 +++++++++++++------ src/cmd/compile/internal/types2/version_test.go | 24 ++++++++++++ src/go/types/check.go | 49 ++++++++++++++----------- src/go/types/generate_test.go | 1 + src/go/types/version_test.go | 2 + 5 files changed, 82 insertions(+), 32 deletions(-) create mode 100644 src/cmd/compile/internal/types2/version_test.go diff --git a/src/cmd/compile/internal/types2/check.go b/src/cmd/compile/internal/types2/check.go index b2a9eb0dbc..0a2a49062b 100644 --- a/src/cmd/compile/internal/types2/check.go +++ b/src/cmd/compile/internal/types2/check.go @@ -11,6 +11,7 @@ import ( "errors" "fmt" "go/constant" + "internal/goversion" . "internal/types/errors" ) @@ -231,19 +232,19 @@ func NewChecker(conf *Config, pkg *Package, info *Info) *Checker { info = new(Info) } - version, err := parseGoVersion(conf.GoVersion) - if err != nil { - panic(fmt.Sprintf("invalid Go version %q (%v)", conf.GoVersion, err)) - } + // Note: clients may call NewChecker with the Unsafe package, which is + // globally shared and must not be mutated. Therefore NewChecker must not + // mutate *pkg. + // + // (previously, pkg.goVersion was mutated here: go.dev/issue/61212) return &Checker{ - conf: conf, - ctxt: conf.Context, - pkg: pkg, - Info: info, - version: version, - objMap: make(map[Object]*declInfo), - impMap: make(map[importKey]*Package), + conf: conf, + ctxt: conf.Context, + pkg: pkg, + Info: info, + objMap: make(map[Object]*declInfo), + impMap: make(map[importKey]*Package), } } @@ -333,6 +334,20 @@ func (check *Checker) Files(files []*syntax.File) error { return check.checkFile var errBadCgo = errors.New("cannot use FakeImportC and go115UsesCgo together") func (check *Checker) checkFiles(files []*syntax.File) (err error) { + if check.pkg == Unsafe { + // Defensive handling for Unsafe, which cannot be type checked, and must + // not be mutated. See https://go.dev/issue/61212 for an example of where + // Unsafe is passed to NewChecker. + return nil + } + + check.version, err = parseGoVersion(check.conf.GoVersion) + if err != nil { + return err + } + if check.version.after(version{1, goversion.Version}) { + return fmt.Errorf("package requires newer Go version %v", check.version) + } if check.conf.FakeImportC && check.conf.go115UsesCgo { return errBadCgo } @@ -377,6 +392,7 @@ func (check *Checker) checkFiles(files []*syntax.File) (err error) { check.monomorph() } + check.pkg.goVersion = check.conf.GoVersion check.pkg.complete = true // no longer needed - release memory diff --git a/src/cmd/compile/internal/types2/version_test.go b/src/cmd/compile/internal/types2/version_test.go new file mode 100644 index 0000000000..651758e1b0 --- /dev/null +++ b/src/cmd/compile/internal/types2/version_test.go @@ -0,0 +1,24 @@ +// Copyright 2023 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 types2 + +import "testing" + +var parseGoVersionTests = []struct { + in string + out version +}{ + {"go1.21", version{1, 21}}, + {"go1.21.0", version{1, 21}}, + {"go1.21rc2", version{1, 21}}, +} + +func TestParseGoVersion(t *testing.T) { + for _, tt := range parseGoVersionTests { + if out, err := parseGoVersion(tt.in); out != tt.out || err != nil { + t.Errorf("parseGoVersion(%q) = %v, %v, want %v, nil", tt.in, out, err, tt.out) + } + } +} diff --git a/src/go/types/check.go b/src/go/types/check.go index 591de5f329..3b0f5e4fdf 100644 --- a/src/go/types/check.go +++ b/src/go/types/check.go @@ -99,12 +99,11 @@ type Checker struct { fset *token.FileSet pkg *Package *Info - version version // accepted language version - versionErr error // version error, delayed from NewChecker - nextID uint64 // unique Id for type parameters (first valid Id is 1) - objMap map[Object]*declInfo // maps package-level objects and (non-interface) methods to declaration info - impMap map[importKey]*Package // maps (import path, source directory) to (complete or fake) package - valids instanceLookup // valid *Named (incl. instantiated) types per the validType check + version version // accepted language version + nextID uint64 // unique Id for type parameters (first valid Id is 1) + objMap map[Object]*declInfo // maps package-level objects and (non-interface) methods to declaration info + impMap map[importKey]*Package // maps (import path, source directory) to (complete or fake) package + valids instanceLookup // valid *Named (incl. instantiated) types per the validType check // pkgPathMap maps package names to the set of distinct import paths we've // seen for that name, anywhere in the import graph. It is used for @@ -235,21 +234,20 @@ func NewChecker(conf *Config, fset *token.FileSet, pkg *Package, info *Info) *Ch info = new(Info) } - version, versionErr := parseGoVersion(conf.GoVersion) - if pkg != nil { - pkg.goVersion = conf.GoVersion - } + // Note: clients may call NewChecker with the Unsafe package, which is + // globally shared and must not be mutated. Therefore NewChecker must not + // mutate *pkg. + // + // (previously, pkg.goVersion was mutated here: go.dev/issue/61212) return &Checker{ - conf: conf, - ctxt: conf.Context, - fset: fset, - pkg: pkg, - Info: info, - version: version, - versionErr: versionErr, - objMap: make(map[Object]*declInfo), - impMap: make(map[importKey]*Package), + conf: conf, + ctxt: conf.Context, + fset: fset, + pkg: pkg, + Info: info, + objMap: make(map[Object]*declInfo), + impMap: make(map[importKey]*Package), } } @@ -345,8 +343,16 @@ func (check *Checker) Files(files []*ast.File) error { return check.checkFiles(f var errBadCgo = errors.New("cannot use FakeImportC and go115UsesCgo together") func (check *Checker) checkFiles(files []*ast.File) (err error) { - if check.versionErr != nil { - return check.versionErr + if check.pkg == Unsafe { + // Defensive handling for Unsafe, which cannot be type checked, and must + // not be mutated. See https://go.dev/issue/61212 for an example of where + // Unsafe is passed to NewChecker. + return nil + } + + check.version, err = parseGoVersion(check.conf.GoVersion) + if err != nil { + return err } if check.version.after(version{1, goversion.Version}) { return fmt.Errorf("package requires newer Go version %v", check.version) @@ -395,6 +401,7 @@ func (check *Checker) checkFiles(files []*ast.File) (err error) { check.monomorph() } + check.pkg.goVersion = check.conf.GoVersion check.pkg.complete = true // no longer needed - release memory diff --git a/src/go/types/generate_test.go b/src/go/types/generate_test.go index 05a848be31..75fda025ee 100644 --- a/src/go/types/generate_test.go +++ b/src/go/types/generate_test.go @@ -141,6 +141,7 @@ var filemap = map[string]action{ "universe.go": fixGlobalTypVarDecl, "util_test.go": fixTokenPos, "validtype.go": nil, + "version_test.go": nil, } // TODO(gri) We should be able to make these rewriters more configurable/composable. diff --git a/src/go/types/version_test.go b/src/go/types/version_test.go index dc9becf9e1..d25f7f5e67 100644 --- a/src/go/types/version_test.go +++ b/src/go/types/version_test.go @@ -1,3 +1,5 @@ +// Code generated by "go test -run=Generate -write=all"; DO NOT EDIT. + // Copyright 2023 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. -- cgit v1.2.3-54-g00ecf From 894d24d617bb72d6e1bed7b143f9f7a0ac16b844 Mon Sep 17 00:00:00 2001 From: Than McIntosh Date: Fri, 7 Jul 2023 16:33:39 -0400 Subject: src/database/sql: run gofmt Run gofmt on a source file. Change-Id: I180d5cc7425fc5d8e9cf63005ac692f361beb1ed Reviewed-on: https://go-review.googlesource.com/c/go/+/508497 Run-TryBot: Than McIntosh Reviewed-by: Eli Bendersky TryBot-Result: Gopher Robot --- src/database/sql/sql.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go index 0764c7d17a..836fe83e2e 100644 --- a/src/database/sql/sql.go +++ b/src/database/sql/sql.go @@ -2949,7 +2949,7 @@ func (rs *Rows) initContextClose(ctx, txctx context.Context) { go rs.awaitDone(ctx, txctx, closectx) } -// awaitDone blocks until ctx, txctx, or closectx is canceled. +// awaitDone blocks until ctx, txctx, or closectx is canceled. // The ctx is provided from the query context. // If the query was issued in a transaction, the transaction's context // is also provided in txctx, to ensure Rows is closed if the Tx is closed. -- cgit v1.2.3-54-g00ecf