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

Comments

Projects
None yet
3 participants
@dmitshur
Member

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

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Oct 5, 2018

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

gopherbot commented Oct 5, 2018

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

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 5, 2018

Member

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
Member

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

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 5, 2018

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Oct 5, 2018

Member

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

Member

bradfitz commented Oct 5, 2018

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

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 5, 2018

Member

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

Member

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 added a commit to gofunky/playground that referenced this issue Oct 8, 2018

playground: use Go 1.11.1
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment