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

Revisit arm support #26

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Revisit arm support #26

wants to merge 2 commits into from

Conversation

henesy
Copy link

@henesy henesy commented Aug 9, 2020

I'm currently working on a file system using styx that can't be built for arm due to overflow errors: henesy/abfs#1

Notably, the old constant code yields a value that indicates a 64-bit integer: https://play.golang.org/p/AUpYTNF6hhs

However, I don't believe a 64-bit integer is necessary here for 9p parsing, I could be incorrect and am open to further input :)

The changes provided seem to pass go test and builds with no warnings/errors for GOARCH=arm.

I saw that the commit I based this off of was reverted due to tests failing, would it be possible to get details on what was failing if this patch doesn't solve the issues?

Hardware used was a Raspberry Pi 1 running 9front/arm.

Tested against jsonfs - my fs is still a port in progress to plan9.

@henesy
Copy link
Author

henesy commented Aug 9, 2020

I spoke too soon, i ran go test on linux/amd64, but not plan9/arm, there I get:

cpu% go test
PASS
panic: Log in goroutine after TestRootQid has completed

goroutine 224 [running]:
testing.(*common).logDepth(0x10886460, 0x10a11020, 0x1b, 0x3)
	/usr/glenda/src/go/src/testing/testing.go:721 +0x430
testing.(*common).log(...)
	/usr/glenda/src/go/src/testing/testing.go:703
testing.(*common).Logf(0x10886460, 0x199a30, 0x19, 0x10a20a58, 0x1, 0x1)
	/usr/glenda/src/go/src/testing/testing.go:749 +0x60
aqwari.net/net/styx.testLogger.Printf(0x10886460, 0x199a30, 0x19, 0x10a20a58, 0x1, 0x1)
	/usr/glenda/go/src/aqwari.net/net/styx/server_test.go:30 +0x48
aqwari.net/net/styx.(*Server).logf(...)
	/usr/glenda/go/src/aqwari.net/net/styx/server.go:200
aqwari.net/net/styx.(*conn).serve(0x109f9080)
	/usr/glenda/go/src/aqwari.net/net/styx/conn.go:193 +0x1dc
created by aqwari.net/net/styx.(*Server).Serve
	/usr/glenda/go/src/aqwari.net/net/styx/server.go:134 +0x31c
exit status: 'styx.test 5972: 2'
FAIL	aqwari.net/net/styx	0.964s
cpu% 

@henesy
Copy link
Author

henesy commented Aug 9, 2020

This seems to be an error in the logging? Doesn't look like an actual protocol error to me, but I'm not sure why these tests are failing

@henesy
Copy link
Author

henesy commented Aug 9, 2020

I don't think this is a styx bug, but looks like a Go bug which might be manifesting peculiarly on arm and/or plan9/arm specifically: golang/go#29388

Specifically, this is caused by a log message being triggered after the test in which the log message would have been sent during, has ended.

@henesy
Copy link
Author

henesy commented Aug 9, 2020

Tests pass, for example, if you disable the testing logger at aqwari.net/net/styx/server_test.go:30:

func (t testLogger) Printf(format string, args ...interface{}) {
	//t.Logf(format, args...)
}

then:

cpu% go version
go version go1.14.7 plan9/arm
cpu% 
cpu% go test
PASS
ok  	aqwari.net/net/styx	0.563s
cpu% 

So I think this doesn't cause an issues for styx, jsonfs works fine, etc., but plan9/arm as a target for Go has a bug in the logger for the "testing" package.

@droyo
Copy link
Owner

droyo commented Nov 1, 2020

Hey, sorry for taking so long to look at this, and thanks for working on it!

Indeed we don't need a 64-bit integer for parsing 9P messages. That limit you changed is just used for an internal message buffer, and messages cannot be larger than a uint32.

I don't have an ARM system handy right now, was this the only change needed to build the package? I'm happy to merge if so.

@henesy
Copy link
Author

henesy commented Nov 6, 2020

I think so, it’s been awhile since I looked at this

I don’t have a computer on me at the moment I can use, but I’ll get back to testing this in the near future I hope

iirc the ARM errors boiled down to a weird race condition that crops up in Go for something in the log/test package on ARMv5(?) specifically

I don’t remember if I dug up other errors during this time and it may be the case that some more work needs to be done

@henesy
Copy link
Author

henesy commented Nov 6, 2020

Thanks for checking in!

@droyo
Copy link
Owner

droyo commented Nov 7, 2020

Might the race condition be #13 ? I spent a couple hours looking at that the other day but didn't quite figure it out. If it's that, we have the same problem on x86.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants