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

x/playground: strict-time.patch does not apply conflict-free on Go 1.11.x #28036

Closed
dmitshur opened this issue Oct 5, 2018 · 5 comments
Closed

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Oct 5, 2018

Running make test with Dockerfile updated to use Go 1.11.x fails because strict-time.patch no longer applies conflict-free.

With Go 1.10.3 (the current version in Dockerfile), we had:

Step 19/63 : RUN patch /usr/local/go/src/runtime/rt0_nacl_amd64p32.s /usr/local/playground/enable-fake-time.patch
 ---> Running in 07d052f922fd
patching file /usr/local/go/src/runtime/rt0_nacl_amd64p32.s
Removing intermediate container 07d052f922fd
 ---> 0ffdf4601200
Step 20/63 : RUN patch -p1 -d /usr/local/go </usr/local/playground/strict-time.patch
 ---> Running in 0a1d34894b7b
patching file src/runtime/os_nacl.go
Hunk #1 succeeded at 293 (offset 4 lines).
patching file src/runtime/sys_nacl_amd64p32.s

With Go 1.11.1, it currently is:

Step 19/63 : RUN patch /usr/local/go/src/runtime/rt0_nacl_amd64p32.s /usr/local/playground/enable-fake-time.patch
 ---> Running in 7fc26dbe59d4
patching file /usr/local/go/src/runtime/rt0_nacl_amd64p32.s
Removing intermediate container 7fc26dbe59d4
 ---> cd5ceaf9e81a
Step 20/63 : RUN patch -p1 -d /usr/local/go </usr/local/playground/strict-time.patch
 ---> Running in d56a00405c2d
patching file src/runtime/os_nacl.go
Reversed (or previously applied) patch detected!  Assume -R? [n] 
Apply anyway? [n] 
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file src/runtime/os_nacl.go.rej
patching file src/runtime/sys_nacl_amd64p32.s
Reversed (or previously applied) patch detected!  Assume -R? [n] 
Apply anyway? [n] 
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file src/runtime/sys_nacl_amd64p32.s.rej
The command '/bin/sh -c patch -p1 -d /usr/local/go </usr/local/playground/strict-time.patch' returned a non-zero code: 1
Makefile:8: recipe for target 'docker' failed
make: *** [docker] Error 1

Resolving this is a prerequisite for updating the Go playground to Go 1.11.x.

CL 131435 and CL 140097 are related to this (and can't be merged without resolving this issue).

This issue is for tracking purposes. /cc @katiehockman @andybons @bradfitz

@gopherbot gopherbot added this to the Unreleased milestone Oct 5, 2018
@gopherbot
Copy link

Change https://golang.org/cl/140097 mentions this issue: playground: use Go 1.11.1

@dmitshur
Copy link
Contributor Author

dmitshur commented Oct 5, 2018

Diff of applying the two .patches to Go 1.10.3 (mostly for my reference):

diff --git a/src/runtime/os_nacl.go b/src/runtime/os_nacl.go
index 6830da4c4f..1754bd875d 100644
--- a/src/runtime/os_nacl.go
+++ b/src/runtime/os_nacl.go
@@ -293,6 +293,15 @@ type gsignalStack struct{}
 
 var writelock uint32 // test-and-set spin lock for write
 
+// lastfaketime stores the last faketime value written to fd 1 or 2.
+var lastfaketime int64
+
+// lastfaketimefd stores the fd to which lastfaketime was written.
+//
+// Subsequent writes to the same fd may use the same timestamp,
+// but the timestamp must increase if the fd changes.
+var lastfaketimefd int32
+
 /*
 An attempt at IRT. Doesn't work. See end of sys_nacl_amd64.s.
 
diff --git a/src/runtime/rt0_nacl_amd64p32.s b/src/runtime/rt0_nacl_amd64p32.s
index 54e4b1de89..6ad8bea6c7 100644
--- a/src/runtime/rt0_nacl_amd64p32.s
+++ b/src/runtime/rt0_nacl_amd64p32.s
@@ -25,6 +25,6 @@ TEXT _rt0_amd64p32_nacl(SB),NOSPLIT,$16
 
 TEXT main(SB),NOSPLIT,$0
 	// Uncomment for fake time like on Go Playground.
-	//MOVQ	$1257894000000000000, AX
-	//MOVQ	AX, runtime·faketime(SB)
+	MOVQ	$1257894000000000000, AX
+	MOVQ	AX, runtime·faketime(SB)
 	JMP	runtime·rt0_go(SB)
diff --git a/src/runtime/sys_nacl_amd64p32.s b/src/runtime/sys_nacl_amd64p32.s
index ff4c2e7bb5..4c4d509576 100644
--- a/src/runtime/sys_nacl_amd64p32.s
+++ b/src/runtime/sys_nacl_amd64p32.s
@@ -89,6 +89,22 @@ playback:
 	CMPL BX, $0
 	JNE playback
 
+	MOVQ runtime·lastfaketime(SB), CX
+	MOVL runtime·lastfaketimefd(SB), BX
+	CMPL DI, BX
+	JE samefd
+
+	// If the current fd doesn't match the fd of the previous write,
+	// ensure that the timestamp is strictly greater. That way, we can
+	// recover the original order even if we read the fds separately.
+	INCQ CX
+	MOVL DI, runtime·lastfaketimefd(SB)
+
+samefd:
+	CMPQ AX, CX
+	CMOVQLT CX, AX
+	MOVQ AX, runtime·lastfaketime(SB)
+
 	// Playback header: 0 0 P B <8-byte time> <4-byte data length>
 	MOVL $(('B'<<24) | ('P'<<16)), 0(SP)
 	BSWAPQ AX

@dmitshur
Copy link
Contributor Author

dmitshur commented Oct 5, 2018

It looks like strict-time.patch isn't applying because it has already been applied to the Go tree in Go 1.11 in CL 105235.

If there's nothing unexpected, I expect the fix is as simple as removing strict-time.patch, it's no longer needed to be cherry-picked as of Go 1.11.

@bradfitz
Copy link
Contributor

bradfitz commented Oct 5, 2018

Yup, looks like it. Only enable-fake-time.patch is needed.

@dmitshur
Copy link
Contributor Author

dmitshur commented Oct 5, 2018

This issue has been resolved via CL 140097, and play.golang.org is running with Go 1.11.1 now:

https://play.golang.org/p/Ztyu2FJaajl

matfax pushed a commit to gofunky/playground that referenced this issue Oct 8, 2018
Go 1.11.1 has been released and should be used.

Revert CL 106216 (other than the added test case), because the
strict-time.patch has already been applied to the Go repository
via CL 105235 in Go 1.11.1.

Reference: https://groups.google.com/d/msg/golang-announce/pFXKAfoVJqw/eyDgSLVYAgAJ.

Fixes golang/go#28036.

Change-Id: Iacf9900a21c4b2f7bf5ac756be2cdbd8ac0be815
Reviewed-on: https://go-review.googlesource.com/c/140097
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants