From 17de9e2d18a62e28bb8032b04d81835e7705d4c7 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Tue, 15 Nov 2022 12:34:31 -0500 Subject: [PATCH] cmd/fix: disallow cgo errors in tests The 'cgo' command invoked by 'go fix' was not valid when built with -trimpath, but the test was not failing because errors from the command were being logged and ignored instead of causing tests to fail. Changing the code and test not to ignore the errors revealed that a number of existing tests were always, unconditionally triggering cgo errors which were then ignored. This change updates those tests to no longer produce cgo errors, and to check their results when cgo is enabled. For #51473. Updates #51461. Change-Id: Ib9d1ea93f26d30daa824d75ed634eaf530af086d Reviewed-on: https://go-review.googlesource.com/c/go/+/450714 Reviewed-by: Ian Lance Taylor TryBot-Result: Gopher Robot Run-TryBot: Bryan Mills Auto-Submit: Bryan Mills --- src/cmd/fix/cftype_test.go | 22 ++++++++++++++++++++++ src/cmd/fix/egltype_test.go | 18 ++++++++++++++++++ src/cmd/fix/jnitype_test.go | 18 ++++++++++++++++++ src/cmd/fix/main_test.go | 24 +++++++++++++++++++++++- src/cmd/fix/typecheck.go | 21 +++++++++++++++++++-- 5 files changed, 100 insertions(+), 3 deletions(-) diff --git a/src/cmd/fix/cftype_test.go b/src/cmd/fix/cftype_test.go index a18eb25261..cde47f28a3 100644 --- a/src/cmd/fix/cftype_test.go +++ b/src/cmd/fix/cftype_test.go @@ -13,6 +13,7 @@ var cftypeTests = []testCase{ Name: "cftype.localVariable", In: `package main +// typedef const void *CFTypeRef; import "C" func f() { @@ -23,6 +24,7 @@ func f() { `, Out: `package main +// typedef const void *CFTypeRef; import "C" func f() { @@ -36,6 +38,7 @@ func f() { Name: "cftype.globalVariable", In: `package main +// typedef const void *CFTypeRef; import "C" var x C.CFTypeRef = nil @@ -46,6 +49,7 @@ func f() { `, Out: `package main +// typedef const void *CFTypeRef; import "C" var x C.CFTypeRef = 0 @@ -59,6 +63,7 @@ func f() { Name: "cftype.EqualArgument", In: `package main +// typedef const void *CFTypeRef; import "C" var x C.CFTypeRef @@ -67,6 +72,7 @@ var z = x != nil `, Out: `package main +// typedef const void *CFTypeRef; import "C" var x C.CFTypeRef @@ -78,6 +84,7 @@ var z = x != 0 Name: "cftype.StructField", In: `package main +// typedef const void *CFTypeRef; import "C" type T struct { @@ -88,6 +95,7 @@ var t = T{x: nil} `, Out: `package main +// typedef const void *CFTypeRef; import "C" type T struct { @@ -101,6 +109,7 @@ var t = T{x: 0} Name: "cftype.FunctionArgument", In: `package main +// typedef const void *CFTypeRef; import "C" func f(x C.CFTypeRef) { @@ -112,6 +121,7 @@ func g() { `, Out: `package main +// typedef const void *CFTypeRef; import "C" func f(x C.CFTypeRef) { @@ -126,12 +136,14 @@ func g() { Name: "cftype.ArrayElement", In: `package main +// typedef const void *CFTypeRef; import "C" var x = [3]C.CFTypeRef{nil, nil, nil} `, Out: `package main +// typedef const void *CFTypeRef; import "C" var x = [3]C.CFTypeRef{0, 0, 0} @@ -141,12 +153,14 @@ var x = [3]C.CFTypeRef{0, 0, 0} Name: "cftype.SliceElement", In: `package main +// typedef const void *CFTypeRef; import "C" var x = []C.CFTypeRef{nil, nil, nil} `, Out: `package main +// typedef const void *CFTypeRef; import "C" var x = []C.CFTypeRef{0, 0, 0} @@ -156,12 +170,14 @@ var x = []C.CFTypeRef{0, 0, 0} Name: "cftype.MapKey", In: `package main +// typedef const void *CFTypeRef; import "C" var x = map[C.CFTypeRef]int{nil: 0} `, Out: `package main +// typedef const void *CFTypeRef; import "C" var x = map[C.CFTypeRef]int{0: 0} @@ -171,12 +187,14 @@ var x = map[C.CFTypeRef]int{0: 0} Name: "cftype.MapValue", In: `package main +// typedef const void *CFTypeRef; import "C" var x = map[int]C.CFTypeRef{0: nil} `, Out: `package main +// typedef const void *CFTypeRef; import "C" var x = map[int]C.CFTypeRef{0: 0} @@ -186,6 +204,7 @@ var x = map[int]C.CFTypeRef{0: 0} Name: "cftype.Conversion1", In: `package main +// typedef const void *CFTypeRef; import "C" var x C.CFTypeRef @@ -193,6 +212,7 @@ var y = (*unsafe.Pointer)(&x) `, Out: `package main +// typedef const void *CFTypeRef; import "C" var x C.CFTypeRef @@ -203,6 +223,7 @@ var y = (*unsafe.Pointer)(unsafe.Pointer(&x)) Name: "cftype.Conversion2", In: `package main +// typedef const void *CFTypeRef; import "C" var x unsafe.Pointer @@ -210,6 +231,7 @@ var y = (*C.CFTypeRef)(&x) `, Out: `package main +// typedef const void *CFTypeRef; import "C" var x unsafe.Pointer diff --git a/src/cmd/fix/egltype_test.go b/src/cmd/fix/egltype_test.go index 9b64a7c20b..c44525c053 100644 --- a/src/cmd/fix/egltype_test.go +++ b/src/cmd/fix/egltype_test.go @@ -17,6 +17,7 @@ func eglTestsFor(tname string) []testCase { Name: "egl.localVariable", In: `package main +// typedef void *$EGLTYPE; import "C" func f() { @@ -27,6 +28,7 @@ func f() { `, Out: `package main +// typedef void *$EGLTYPE; import "C" func f() { @@ -40,6 +42,7 @@ func f() { Name: "egl.globalVariable", In: `package main +// typedef void *$EGLTYPE; import "C" var x C.$EGLTYPE = nil @@ -50,6 +53,7 @@ func f() { `, Out: `package main +// typedef void *$EGLTYPE; import "C" var x C.$EGLTYPE = 0 @@ -63,6 +67,7 @@ func f() { Name: "egl.EqualArgument", In: `package main +// typedef void *$EGLTYPE; import "C" var x C.$EGLTYPE @@ -71,6 +76,7 @@ var z = x != nil `, Out: `package main +// typedef void *$EGLTYPE; import "C" var x C.$EGLTYPE @@ -82,6 +88,7 @@ var z = x != 0 Name: "egl.StructField", In: `package main +// typedef void *$EGLTYPE; import "C" type T struct { @@ -92,6 +99,7 @@ var t = T{x: nil} `, Out: `package main +// typedef void *$EGLTYPE; import "C" type T struct { @@ -105,6 +113,7 @@ var t = T{x: 0} Name: "egl.FunctionArgument", In: `package main +// typedef void *$EGLTYPE; import "C" func f(x C.$EGLTYPE) { @@ -116,6 +125,7 @@ func g() { `, Out: `package main +// typedef void *$EGLTYPE; import "C" func f(x C.$EGLTYPE) { @@ -130,12 +140,14 @@ func g() { Name: "egl.ArrayElement", In: `package main +// typedef void *$EGLTYPE; import "C" var x = [3]C.$EGLTYPE{nil, nil, nil} `, Out: `package main +// typedef void *$EGLTYPE; import "C" var x = [3]C.$EGLTYPE{0, 0, 0} @@ -145,12 +157,14 @@ var x = [3]C.$EGLTYPE{0, 0, 0} Name: "egl.SliceElement", In: `package main +// typedef void *$EGLTYPE; import "C" var x = []C.$EGLTYPE{nil, nil, nil} `, Out: `package main +// typedef void *$EGLTYPE; import "C" var x = []C.$EGLTYPE{0, 0, 0} @@ -160,12 +174,14 @@ var x = []C.$EGLTYPE{0, 0, 0} Name: "egl.MapKey", In: `package main +// typedef void *$EGLTYPE; import "C" var x = map[C.$EGLTYPE]int{nil: 0} `, Out: `package main +// typedef void *$EGLTYPE; import "C" var x = map[C.$EGLTYPE]int{0: 0} @@ -175,12 +191,14 @@ var x = map[C.$EGLTYPE]int{0: 0} Name: "egl.MapValue", In: `package main +// typedef void *$EGLTYPE; import "C" var x = map[int]C.$EGLTYPE{0: nil} `, Out: `package main +// typedef void *$EGLTYPE; import "C" var x = map[int]C.$EGLTYPE{0: 0} diff --git a/src/cmd/fix/jnitype_test.go b/src/cmd/fix/jnitype_test.go index a6420f7b11..ecf01408c7 100644 --- a/src/cmd/fix/jnitype_test.go +++ b/src/cmd/fix/jnitype_test.go @@ -13,6 +13,7 @@ var jniTests = []testCase{ Name: "jni.localVariable", In: `package main +// typedef struct _jobject* jobject; import "C" func f() { @@ -23,6 +24,7 @@ func f() { `, Out: `package main +// typedef struct _jobject* jobject; import "C" func f() { @@ -36,6 +38,7 @@ func f() { Name: "jni.globalVariable", In: `package main +// typedef struct _jobject* jobject; import "C" var x C.jobject = nil @@ -46,6 +49,7 @@ func f() { `, Out: `package main +// typedef struct _jobject* jobject; import "C" var x C.jobject = 0 @@ -59,6 +63,7 @@ func f() { Name: "jni.EqualArgument", In: `package main +// typedef struct _jobject* jobject; import "C" var x C.jobject @@ -67,6 +72,7 @@ var z = x != nil `, Out: `package main +// typedef struct _jobject* jobject; import "C" var x C.jobject @@ -78,6 +84,7 @@ var z = x != 0 Name: "jni.StructField", In: `package main +// typedef struct _jobject* jobject; import "C" type T struct { @@ -88,6 +95,7 @@ var t = T{x: nil} `, Out: `package main +// typedef struct _jobject* jobject; import "C" type T struct { @@ -101,6 +109,7 @@ var t = T{x: 0} Name: "jni.FunctionArgument", In: `package main +// typedef struct _jobject* jobject; import "C" func f(x C.jobject) { @@ -112,6 +121,7 @@ func g() { `, Out: `package main +// typedef struct _jobject* jobject; import "C" func f(x C.jobject) { @@ -126,12 +136,14 @@ func g() { Name: "jni.ArrayElement", In: `package main +// typedef struct _jobject* jobject; import "C" var x = [3]C.jobject{nil, nil, nil} `, Out: `package main +// typedef struct _jobject* jobject; import "C" var x = [3]C.jobject{0, 0, 0} @@ -141,12 +153,14 @@ var x = [3]C.jobject{0, 0, 0} Name: "jni.SliceElement", In: `package main +// typedef struct _jobject* jobject; import "C" var x = []C.jobject{nil, nil, nil} `, Out: `package main +// typedef struct _jobject* jobject; import "C" var x = []C.jobject{0, 0, 0} @@ -156,12 +170,14 @@ var x = []C.jobject{0, 0, 0} Name: "jni.MapKey", In: `package main +// typedef struct _jobject* jobject; import "C" var x = map[C.jobject]int{nil: 0} `, Out: `package main +// typedef struct _jobject* jobject; import "C" var x = map[C.jobject]int{0: 0} @@ -171,12 +187,14 @@ var x = map[C.jobject]int{0: 0} Name: "jni.MapValue", In: `package main +// typedef struct _jobject* jobject; import "C" var x = map[int]C.jobject{0: nil} `, Out: `package main +// typedef struct _jobject* jobject; import "C" var x = map[int]C.jobject{0: 0} diff --git a/src/cmd/fix/main_test.go b/src/cmd/fix/main_test.go index 755007bc0d..837a5d72c7 100644 --- a/src/cmd/fix/main_test.go +++ b/src/cmd/fix/main_test.go @@ -5,13 +5,29 @@ package main import ( + "fmt" "go/ast" "go/parser" "internal/diff" + "internal/testenv" "strings" "testing" ) +func init() { + // If cgo is enabled, enforce that cgo commands invoked by cmd/fix + // do not fail during testing. + if testenv.HasCGO() { + // The reportCgoError hook is global, so we can't set it per-test + // if we want to be able to run those tests in parallel. + // Instead, simply set it to panic on error: the goroutine dump + // from the panic should help us determine which test failed. + reportCgoError = func(err error) { + panic(fmt.Sprintf("unexpected cgo error: %v", err)) + } + } +} + type testCase struct { Name string Fn func(*ast.File) bool @@ -79,7 +95,13 @@ func TestRewrite(t *testing.T) { tt := tt t.Run(tt.Name, func(t *testing.T) { if tt.Version == 0 { - t.Parallel() + if testing.Verbose() { + // Don't run in parallel: cmd/fix sometimes writes directly to stderr, + // and since -v prints which test is currently running we want that + // information to accurately correlate with the stderr output. + } else { + t.Parallel() + } } else { old := goVersion goVersion = tt.Version diff --git a/src/cmd/fix/typecheck.go b/src/cmd/fix/typecheck.go index 27042e05a5..b115987390 100644 --- a/src/cmd/fix/typecheck.go +++ b/src/cmd/fix/typecheck.go @@ -170,7 +170,16 @@ func typecheck(cfg *TypeConfig, f *ast.File) (typeof map[any]string, assign map[ if err != nil { return err } - cmd := exec.Command(filepath.Join(runtime.GOROOT(), "bin", "go"), "tool", "cgo", "-objdir", dir, "-srcdir", dir, "in.go") + goCmd := "go" + if goroot := runtime.GOROOT(); goroot != "" { + goCmd = filepath.Join(goroot, "bin", "go") + } + cmd := exec.Command(goCmd, "tool", "cgo", "-objdir", dir, "-srcdir", dir, "in.go") + if reportCgoError != nil { + // Since cgo command errors will be reported, also forward the error + // output from the command for debugging. + cmd.Stderr = os.Stderr + } err = cmd.Run() if err != nil { return err @@ -206,7 +215,11 @@ func typecheck(cfg *TypeConfig, f *ast.File) (typeof map[any]string, assign map[ return nil }() if err != nil { - fmt.Fprintf(os.Stderr, "go fix: warning: no cgo types: %s\n", err) + if reportCgoError == nil { + fmt.Fprintf(os.Stderr, "go fix: warning: no cgo types: %s\n", err) + } else { + reportCgoError(err) + } } } @@ -285,6 +298,10 @@ func typecheck(cfg *TypeConfig, f *ast.File) (typeof map[any]string, assign map[ return typeof, assign } +// reportCgoError, if non-nil, reports a non-nil error from running the "cgo" +// tool. (Set to a non-nil hook during testing if cgo is expected to work.) +var reportCgoError func(err error) + func makeExprList(a []*ast.Ident) []ast.Expr { var b []ast.Expr for _, x := range a { -- 2.50.0