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

Jump to bottom

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
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Backlog

Comments

@perillo
Copy link
Contributor

perillo commented Apr 6, 2021 •
edited

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 6, 2021
@dmitshur dmitshur added this to the Backlog milestone Apr 6, 2021
@dmitshur dmitshur added the Testing An issue that has been verified to require only test changes, not just a test failure. label Apr 6, 2021
@ianlancetaylor
Copy link
Contributor

CC @laboger

@laboger
Copy link
Contributor

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 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 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 commented Apr 8, 2021 •
edited

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

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

@golang golang locked and limited conversation to collaborators Apr 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@dmitshur @ianlancetaylor @perillo @gopherbot @laboger