From ad65e740dc861b30c139e1720716ba5991af1c5c Mon Sep 17 00:00:00 2001 From: Pinkle-pash Date: Tue, 6 Feb 2024 10:16:40 -0500 Subject: [PATCH] counter: support 386 systems Modify the way mmap is invoked on Windows systems to support our technique for extending the size of counter files. The existing code was overelaborate and failed on 386. Also, stop generating empty file names for uploading. Fixes golang/go#65447 Fixes golang/go#60692 Fixes golang/go#60615 Change-Id: I8349d250fb516c188850557d3d099378248fb17b Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/561995 Run-TryBot: Peter Weinberger Reviewed-by: Robert Findley TryBot-Result: Gopher Robot --- counter/counter.go | 2 +- counter/counter_disabled.go | 2 +- counter/countertest/countertest_test.go | 2 +- internal/mmap/mmap_windows.go | 14 ++-- internal/regtest/e2e_test.go | 3 + internal/upload/reports.go | 102 +++++++++++++----------- 6 files changed, 69 insertions(+), 56 deletions(-) diff --git a/counter/counter.go b/counter/counter.go index 9b80d46..6394287 100644 --- a/counter/counter.go +++ b/counter/counter.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 go1.19 && !openbsd && !js && !wasip1 && !solaris && !android && !plan9 && !386 +//go:build go1.19 && !openbsd && !js && !wasip1 && !solaris && !android package counter diff --git a/counter/counter_disabled.go b/counter/counter_disabled.go index c4b948d..1783581 100644 --- a/counter/counter_disabled.go +++ b/counter/counter_disabled.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 !go1.19 || openbsd || js || wasip1 || solaris || android || plan9 || 386 +//go:build !go1.19 || openbsd || js || wasip1 || solaris || android package counter diff --git a/counter/countertest/countertest_test.go b/counter/countertest/countertest_test.go index 24469e8..b62e2cb 100644 --- a/counter/countertest/countertest_test.go +++ b/counter/countertest/countertest_test.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 go1.21 +//go:build go1.21 && !openbsd && !js && !wasip1 && !solaris && !android package countertest diff --git a/internal/mmap/mmap_windows.go b/internal/mmap/mmap_windows.go index d1255fd..66997a8 100644 --- a/internal/mmap/mmap_windows.go +++ b/internal/mmap/mmap_windows.go @@ -25,22 +25,20 @@ func mmapFile(f *os.File, previous *Data) (Data, error) { if size == 0 { return Data{f, nil, nil}, nil } + // setting the min and max sizes to zero to map the whole file, as described in + // https://learn.microsoft.com/en-us/windows/win32/memory/creating-a-file-mapping-object#file-mapping-size h, err := windows.CreateFileMapping(windows.Handle(f.Fd()), nil, syscall.PAGE_READWRITE, 0, 0, nil) if err != nil { return Data{}, fmt.Errorf("CreateFileMapping %s: %w", f.Name(), err) } - + // the mapping extends from zero to the end of the file mapping + // https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-mapviewoffile addr, err := windows.MapViewOfFile(h, syscall.FILE_MAP_READ|syscall.FILE_MAP_WRITE, 0, 0, 0) if err != nil { return Data{}, fmt.Errorf("MapViewOfFile %s: %w", f.Name(), err) } - var info windows.MemoryBasicInformation - err = windows.VirtualQuery(addr, &info, unsafe.Sizeof(info)) - if err != nil { - return Data{}, fmt.Errorf("VirtualQuery %s: %w", f.Name(), err) - } - data := unsafe.Slice((*byte)(unsafe.Pointer(addr)), int(info.RegionSize)) - return Data{f, data, h}, nil + // need to remember addr and h for unmapping + return Data{f, unsafe.Slice((*byte)(unsafe.Pointer(addr)), size), h}, nil } func munmapFile(d Data) error { diff --git a/internal/regtest/e2e_test.go b/internal/regtest/e2e_test.go index 6db66a8..0d49241 100644 --- a/internal/regtest/e2e_test.go +++ b/internal/regtest/e2e_test.go @@ -24,6 +24,7 @@ import ( ) func TestRunProg(t *testing.T) { + testenv.SkipIfUnsupportedPlatform(t) testenv.MustHaveExec(t) prog1 := NewProgram(t, "prog1", func() int { fmt.Println("FuncB") @@ -55,6 +56,7 @@ func programIncCounters() int { } func TestE2E_off(t *testing.T) { + testenv.SkipIfUnsupportedPlatform(t) testenv.MustHaveExec(t) testenv.SkipIfUnsupportedPlatform(t) @@ -94,6 +96,7 @@ func TestE2E_off(t *testing.T) { } func TestE2E(t *testing.T) { + testenv.SkipIfUnsupportedPlatform(t) testenv.MustHaveExec(t) testenv.SkipIfUnsupportedPlatform(t) programIncCounters := NewProgram(t, "prog", programIncCounters) diff --git a/internal/upload/reports.go b/internal/upload/reports.go index ecba0f8..ef14c59 100644 --- a/internal/upload/reports.go +++ b/internal/upload/reports.go @@ -57,7 +57,9 @@ func (u *Uploader) reports(todo *work) ([]string, error) { if err != nil { return nil, err } - todo.readyfiles = append(todo.readyfiles, fname) + if fname != "" { + todo.readyfiles = append(todo.readyfiles, fname) + } } return todo.readyfiles, nil } @@ -176,81 +178,91 @@ func (u *Uploader) createReport(start time.Time, expiryDate string, files []stri if err := json.Unmarshal(localContents, &x); err != nil { return "", fmt.Errorf("failed to unmarshal local report (%v)", err) } - // 2. create the uploadable version - cfg := config.NewConfig(u.Config) - upload := &telemetry.Report{ - Week: report.Week, - LastWeek: report.LastWeek, - X: report.X, - Config: report.Config, - } - for _, p := range report.Programs { - // does the uploadConfig want this program? - // if so, copy over the Stacks and Counters - // that the uploadConfig mentions. - if !cfg.HasGoVersion(p.GoVersion) || !cfg.HasProgram(p.Program) || !cfg.HasVersion(p.Program, p.Version) { - continue - } - x := &telemetry.ProgramReport{ - Program: p.Program, - Version: p.Version, - GOOS: p.GOOS, - GOARCH: p.GOARCH, - GoVersion: p.GoVersion, - Counters: make(map[string]int64), - Stacks: make(map[string]int64), + + var uploadContents []byte + if uploadOK { + // 2. create the uploadable version + cfg := config.NewConfig(u.Config) + upload := &telemetry.Report{ + Week: report.Week, + LastWeek: report.LastWeek, + X: report.X, + Config: report.Config, } - upload.Programs = append(upload.Programs, x) - for k, v := range p.Counters { - if cfg.HasCounter(p.Program, k) && report.X <= cfg.Rate(p.Program, k) { - x.Counters[k] = v + for _, p := range report.Programs { + // does the uploadConfig want this program? + // if so, copy over the Stacks and Counters + // that the uploadConfig mentions. + if !cfg.HasGoVersion(p.GoVersion) || !cfg.HasProgram(p.Program) || !cfg.HasVersion(p.Program, p.Version) { + continue } - } - // and the same for Stacks - // this can be made more efficient, when it matters - for k, v := range p.Stacks { - before, _, _ := strings.Cut(k, "\n") - if cfg.HasStack(p.Program, before) && report.X <= cfg.Rate(p.Program, before) { - x.Stacks[k] = v + x := &telemetry.ProgramReport{ + Program: p.Program, + Version: p.Version, + GOOS: p.GOOS, + GOARCH: p.GOARCH, + GoVersion: p.GoVersion, + Counters: make(map[string]int64), + Stacks: make(map[string]int64), + } + upload.Programs = append(upload.Programs, x) + for k, v := range p.Counters { + if cfg.HasCounter(p.Program, k) && report.X <= cfg.Rate(p.Program, k) { + x.Counters[k] = v + } + } + // and the same for Stacks + // this can be made more efficient, when it matters + for k, v := range p.Stacks { + before, _, _ := strings.Cut(k, "\n") + if cfg.HasStack(p.Program, before) && report.X <= cfg.Rate(p.Program, before) { + x.Stacks[k] = v + } } } - } - uploadContents, err := json.MarshalIndent(upload, "", " ") - if err != nil { - return "", fmt.Errorf("failed to marshal upload report (%v)", err) + uploadContents, err = json.MarshalIndent(upload, "", " ") + if err != nil { + return "", fmt.Errorf("failed to marshal upload report (%v)", err) + } } localFileName := filepath.Join(u.LocalDir, "local."+expiryDate+".json") uploadFileName := filepath.Join(u.LocalDir, expiryDate+".json") + + /* Prepare to write files */ // if either file exists, someone has been here ahead of us // (there is still a race, but this check shortens the open window) if _, err := os.Stat(localFileName); err == nil { deleteFiles(files) - return "", fmt.Errorf("report %s already exists", localFileName) + return "", fmt.Errorf("local report %s already exists", localFileName) } if _, err := os.Stat(uploadFileName); err == nil { deleteFiles(files) - return "", fmt.Errorf("local report %s already exists", uploadFileName) + return "", fmt.Errorf("report %s already exists", uploadFileName) } // write the uploadable file var errUpload, errLocal error if uploadOK { errUpload = os.WriteFile(uploadFileName, uploadContents, 0644) } - // save all local reports. + // write the local file errLocal = os.WriteFile(localFileName, localContents, 0644) + /* Wrote the files */ // even though these errors won't occur, what should happen // if errUpload == nil and it is ok to upload, and errLocal != nil? if errLocal != nil { - return "", fmt.Errorf("failed to write local file %s (%v)", uploadFileName, errLocal) + return "", fmt.Errorf("failed to write local file %s (%v)", localFileName, errLocal) } if errUpload != nil { return "", fmt.Errorf("failed to write upload file %s (%v)", uploadFileName, errUpload) } - logger.Printf("created %s, deleting %v", uploadFileName, files) + logger.Printf("created %q, deleting %v", uploadFileName, files) deleteFiles(files) - return uploadFileName, nil + if uploadOK { + return uploadFileName, nil + } + return "", nil } // return an existing ProgremReport, or create anew