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

go/internal/gccgoimporter: File.ReadAt moves underlying seek position on Plan9? #11265

Closed
griesemer opened this issue Jun 17, 2015 · 11 comments
Closed

Comments

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jun 17, 2015

https://go-review.googlesource.com/#/c/11152/ fails on Plan 9 without line 7 in https://go-review.googlesource.com/#/c/11152/7/src/go/internal/gccgoimporter/importer_test.go (and the corresponding line 7 in https://go-review.googlesource.com/#/c/11152/7/src/go/internal/gccgoimporter/gccgoinstallation_test.go).

The change was submitted with the tests disabled for Plan 9. When actually running the tests on Plan 9 (i.e., without the build tag), the failure is:

--- FAIL: TestGoxImporter (0.00s)
    importer_test.go:26: unrecognized magic string: "pack"
    importer_test.go:26: unrecognized magic string: "pack"
    importer_test.go:26: unrecognized magic string: "pack"
    importer_test.go:26: unrecognized magic string: "pack"
    importer_test.go:26: unrecognized magic string: "pack"
FAIL
FAIL    go/internal/gccgoimporter   0.031s

( https://storage.googleapis.com/go-build-log/e6448c25/plan9-386_c70f45b0.log )

Analysis:

In this specific test, the go/internal/gccgoimporter/testdata/*.gox files are read. In a first step, openExportFile (importer.go:74ff) reads the first 4 "magic" bytes using File.ReadAt (line 87). They are "v1;\n" (see e.g. testdata/pointer.gox). In a 2nd step, the closure returned by GetImporter (importer.go:142ff) reads the file's magic bytes again, this time via io.ReadSeeker.Read. It appears that the next 4 bytes "pack" are read this time, leading to the above error.

On non-Plan9 systems, this error doesn't appear. It seems that the Plan9 File.ReadAt moves the seek position, while other OSes don't.

From the documentation:

It would appear the File.ReadAt should follow this specification as well (and perhaps it should be documented).

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jun 18, 2015

I don't think we need to repeat the io.ReaderAt docs on every implementation thereof.

This is just a Plan 9 bug.

griesemer added a commit that referenced this issue Jun 18, 2015
Work-around issue #11265 and re-enable tests for Plan9.

Change-Id: I3aabb674a149b8eb936f948dd4cda5fd81454646
Reviewed-on: https://go-review.googlesource.com/11194
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Jun 18, 2015

It appears that https://go-review.googlesource.com/11194 makes the Plan9 test pass, confirming the hypothesis above.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jun 18, 2015

Also revert https://go-review.googlesource.com/11210

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Jun 18, 2015

It's fine to revert once the bug is fixed. This is not critical and the
work-around is documented.

On Wed, Jun 17, 2015 at 5:34 PM, Brad Fitzpatrick notifications@github.com
wrote:

Also revert https://go-review.googlesource.com/11210


Reply to this email directly or view it on GitHub
#11265 (comment).

@0intro
Copy link
Member

@0intro 0intro commented Jun 18, 2015

Thanks for investigating this issue.

In my understanding, the Plan 9 read documentation says that:

pread(fd, buf, nbytes, offset)

Is equivalent to:

seek(fd, offset, 0)
read(fd, buf, nbytes)

So, it does indeed change the seek offset, which is not the behavior expected by Go.

I think the solution is to call f.seek(0, SEEK_SET) just after syscall.Pread in Go's implementation of pread on Plan 9.

What do you think?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jun 18, 2015

That's not how I read those docs. Look at the last sentence:

Pread and Pwrite are equivalent to a seek(2) to offset followed by a read or write. By combining the operations in a single atomic call, they more closely match the 9P protocol (see intro(5)) and, more important, permit multiprocess programs to execute multiple concurrent read and write operations on the same file descriptor without interference.

So it seems like the normal pread or io.ReaderAt semantics.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jun 18, 2015

@0intro, even if it does atomically do the seek + read but leaves the seek offset modified for other threads, that still violates Go's semantics.

In Go, one goroutine must be allowed to use an io.Reader+io.ReaderAt as a Reader while the other uses the same value as an io.ReaderAt, and neither one of them is allowed to interfere with the other goroutine.

Where is the Plan 9 source for the implementation of pread?

@0intro
Copy link
Member

@0intro 0intro commented Jun 18, 2015

Where is the Plan 9 source for the implementation of pread?

Here:

https://github.com/0intro/plan9/blob/bd0c9332d5/sys/src/9/port/sysfile.c#L701

@ality ality assigned ality and unassigned 0intro Jun 21, 2015
@ality
Copy link
Member

@ality ality commented Jun 21, 2015

I'm working on this in https://go-review.googlesource.com/11280

However, I now think it's a kernel bug. Awaiting feedback on 9fans.
https://marc.info/?l=9fans&m=143488356201304&w=2

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 24, 2015

CL https://golang.org/cl/11280 mentions this issue.

@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Jul 11, 2015
@rsc
Copy link
Contributor

@rsc rsc commented Nov 5, 2015

This is a Plan 9 kernel bug.

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

7 participants