styx icon indicating copy to clipboard operation
styx copied to clipboard

Revisit arm support

Open henesy opened this issue 5 years ago • 8 comments

I'm currently working on a file system using styx that can't be built for arm due to overflow errors: https://github.com/henesy/abfs/issues/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 avatar Aug 09 '20 05:08 henesy

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 avatar Aug 09 '20 06:08 henesy

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 avatar Aug 09 '20 06:08 henesy

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: https://github.com/golang/go/issues/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 avatar Aug 09 '20 06:08 henesy

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.

henesy avatar Aug 09 '20 06:08 henesy

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.

droyo avatar Nov 01 '20 02:11 droyo

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 avatar Nov 06 '20 20:11 henesy

Thanks for checking in!

henesy avatar Nov 06 '20 20:11 henesy

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.

droyo avatar Nov 07 '20 03:11 droyo