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 <[email protected]> Reviewed-by: Robert Findley <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
Pinkle-pash · Feb 11, 2024 · ad65e74 · ad65e74
1 parent 8aeedef
commit ad65e74
Show file tree

Hide file tree

Showing 6 changed files with 69 additions and 56 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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