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.

Sign up for GitHub

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

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

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

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

griesemer opened this issue Jun 17, 2015 · 11 comments
Labels
FrozenDueToAge help wanted OS-Plan9
Milestone
Go1.6

Comments

@griesemer
Copy link
Contributor

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

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
@griesemer
go/internal/gccgoimporter: enable tests on Plan9
3cc7b27 
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

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

@bradfitz
Copy link
Contributor

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

@griesemer
Copy link
Contributor Author

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 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

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

@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 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 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

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 commented Nov 5, 2015

This is a Plan 9 kernel bug.

@rsc rsc closed this as completed Nov 5, 2015
@0intro 0intro mentioned this issue Feb 26, 2016
Closed
@golang golang locked and limited conversation to collaborators Nov 4, 2016
@rsc rsc unassigned ality Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted OS-Plan9
Projects
None yet
Milestone
Go1.6
Development

No branches or pull requests
7 participants
@bradfitz @rsc @ality @ianlancetaylor @0intro @griesemer @gopherbot