Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/link: TestLargeText in linkbig_test.go does not look right #45406

Closed
perillo opened this issue Apr 6, 2021 · 6 comments
Closed

cmd/link: TestLargeText in linkbig_test.go does not look right #45406

perillo opened this issue Apr 6, 2021 · 6 comments

Comments

@perillo
Copy link
Contributor

@perillo perillo commented Apr 6, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16.3 linux/amd64

The TestLargeText seems to have some problems:

  1. It calls os.Chdir without checking the error
  2. Does not restore the original working directory
  3. Calls os.Chdir later, instead of calling it early (allowing a simplification of the code)
  4. Calls os.Chdir(tmpdir) twice

Here is a patch that will allow the use of the chtmpdir function (see #45182).
os.Chdir error handling and working directory restoration is not included.
It uses relative paths, since we have chdir early.

diff --git a/src/cmd/link/linkbig_test.go b/src/cmd/link/linkbig_test.go
index d5d77d6c72..e6f58a5843 100644
--- a/src/cmd/link/linkbig_test.go
+++ b/src/cmd/link/linkbig_test.go
@@ -28,6 +28,7 @@ func TestLargeText(t *testing.T) {
 	var w bytes.Buffer
 	const FN = 4
 	tmpdir := t.TempDir()
+	os.Chdir(tmpdir)
 
 	// Generate the scenario where the total amount of text exceeds the
 	// limit for the jmp/call instruction, on RISC architectures like ppc64le,
@@ -47,7 +48,7 @@ func TestLargeText(t *testing.T) {
 			fmt.Fprintf(&w, inst)
 		}
 		fmt.Fprintf(&w, "\tRET\n")
-		err := ioutil.WriteFile(tmpdir+"/"+testname+".s", w.Bytes(), 0666)
+		err := ioutil.WriteFile(testname+".s", w.Bytes(), 0666)
 		if err != nil {
 			t.Fatalf("can't write output: %v\n", err)
 		}
@@ -74,32 +75,30 @@ func TestLargeText(t *testing.T) {
 	fmt.Fprintf(&w, "\t}\n")
 	fmt.Fprintf(&w, "\tfmt.Printf(\"PASS\\n\")\n")
 	fmt.Fprintf(&w, "}")
-	err := ioutil.WriteFile(tmpdir+"/bigfn.go", w.Bytes(), 0666)
+	err := ioutil.WriteFile("bigfn.go", w.Bytes(), 0666)
 	if err != nil {
 		t.Fatalf("can't write output: %v\n", err)
 	}
 
 	// Build and run with internal linking.
-	os.Chdir(tmpdir)
 	cmd := exec.Command(testenv.GoToolPath(t), "build", "-o", "bigtext")
 	out, err := cmd.CombinedOutput()
 	if err != nil {
 		t.Fatalf("Build failed for big text program with internal linking: %v, output: %s", err, out)
 	}
-	cmd = exec.Command(tmpdir + "/bigtext")
+	cmd = exec.Command("./bigtext")
 	out, err = cmd.CombinedOutput()
 	if err != nil {
 		t.Fatalf("Program built with internal linking failed to run with err %v, output: %s", err, out)
 	}
 
 	// Build and run with external linking
-	os.Chdir(tmpdir)
 	cmd = exec.Command(testenv.GoToolPath(t), "build", "-o", "bigtext", "-ldflags", "'-linkmode=external'")
 	out, err = cmd.CombinedOutput()
 	if err != nil {
 		t.Fatalf("Build failed for big text program with external linking: %v, output: %s", err, out)
 	}
-	cmd = exec.Command(tmpdir + "/bigtext")
+	cmd = exec.Command("./bigtext")
 	out, err = cmd.CombinedOutput()
 	if err != nil {
 		t.Fatalf("Program built with external linking failed to run with err %v, output: %s", err, out)

Commenting the code that skip the test on my system (linux amd64), the test works, but I'm not sure if there was a good reason for the current implementation.

However I have found an additional issue: the test only works if GO111MODULE=off, otherwise the test fails with

Build failed for big text program with internal linking: exit status 1, output: go: go.mod file not found in current directory or any parent directory; see 'go help modules'

and all the tests that follow (when executing run.bash) fails with

cannot determine current directory: getwd: no such file or directory
@perillo perillo changed the title cmd: TestLargeText in linkbig_test.go does not look right cmd: TestLargeText in linkbig_test.go does not look right Apr 6, 2021
@dmitshur dmitshur changed the title cmd: TestLargeText in linkbig_test.go does not look right cmd/link: TestLargeText in linkbig_test.go does not look right Apr 6, 2021
@dmitshur dmitshur added this to the Backlog milestone Apr 6, 2021
@dmitshur dmitshur added the Testing label Apr 6, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 6, 2021

@laboger
Copy link
Contributor

@laboger laboger commented Apr 8, 2021

I can fix the chdir stuff that doesn't look right.
This test is intended to verify very large text sections where trampolines are generated, so that only applies to ppc64x and arm64 and don't think it should be enabled for amd64 (not sure if that was your suggestion.)

I can check the module issue.

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 8, 2021

@laboger, no, I was not suggesting to enable it for amd64. I just forced the test to run on Linux amd64 and I got that module issue, but it is strange that the build bots don't report it.

Thanks.

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 8, 2021

By the way, is Chdir necessary? Isn't it better to set the exec.Cmd.Dir field to the temporary directory?

@perillo
Copy link
Contributor Author

@perillo perillo commented Apr 8, 2021

Here a patch to remove the use of Chdir. I have also changed the name of the executable generated using external linking, just to be sure:

diff --git a/src/cmd/link/linkbig_test.go b/src/cmd/link/linkbig_test.go
index d5d77d6c72..577fe0daae 100644
--- a/src/cmd/link/linkbig_test.go
+++ b/src/cmd/link/linkbig_test.go
@@ -14,7 +14,6 @@ import (
 	"fmt"
 	"internal/testenv"
 	"io/ioutil"
-	"os"
 	"os/exec"
 	"testing"
 )
@@ -80,26 +79,28 @@ func TestLargeText(t *testing.T) {
 	}
 
 	// Build and run with internal linking.
-	os.Chdir(tmpdir)
 	cmd := exec.Command(testenv.GoToolPath(t), "build", "-o", "bigtext")
+	cmd.Dir = tmpdir
 	out, err := cmd.CombinedOutput()
 	if err != nil {
 		t.Fatalf("Build failed for big text program with internal linking: %v, output: %s", err, out)
 	}
-	cmd = exec.Command(tmpdir + "/bigtext")
+	cmd = exec.Command("./bigtext")
+	cmd.Dir = tmpdir
 	out, err = cmd.CombinedOutput()
 	if err != nil {
 		t.Fatalf("Program built with internal linking failed to run with err %v, output: %s", err, out)
 	}
 
 	// Build and run with external linking
-	os.Chdir(tmpdir)
-	cmd = exec.Command(testenv.GoToolPath(t), "build", "-o", "bigtext", "-ldflags", "'-linkmode=external'")
+	cmd = exec.Command(testenv.GoToolPath(t), "build", "-o", "bigtext-ext", "-ldflags", "'-linkmode=external'")
+	cmd.Dir = tmpdir
 	out, err = cmd.CombinedOutput()
 	if err != nil {
 		t.Fatalf("Build failed for big text program with external linking: %v, output: %s", err, out)
 	}
-	cmd = exec.Command(tmpdir + "/bigtext")
+	cmd = exec.Command("./bigtext-ext")
+	cmd.Dir = tmpdir
 	out, err = cmd.CombinedOutput()
 	if err != nil {
 		t.Fatalf("Program built with external linking failed to run with err %v, output: %s", err, out)

Setting Cmd.Dir when executing the generated binary is not really necessary.

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 9, 2021

Change https://golang.org/cl/308935 mentions this issue: cmd/link: fix TestLargeText

@gopherbot gopherbot closed this in 07b2fee Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants