Far far pointer bug fix #71
Conversation
rawpointer.go
Outdated
| @@ -56,7 +56,14 @@ func rawDoubleFarPointer(segID SegmentID, off Address) rawPointer { | |||
| // a near pointer in the destination segment. Its offset will be | |||
| // relative to the beginning of the segment. | |||
| func landingPadNearPointer(far, tag rawPointer) rawPointer { | |||
| return tag | rawPointer(far.farAddress()-Address(wordSize))<<2 | |||
| farFarAddrress := far.farAddress() / Address(wordSize) | |||
| @@ -232,3 +232,39 @@ func TestRawPointerTotalListSize(t *testing.T) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func TestLandingPadNearPointer(t *testing.T) { | |||
There was a problem hiding this comment.
This test doesn't really help me understand what failed with the previous implementation. Instead, can you write this as a table test of:
tests := []struct{
far rawPointer
tag rawPointer
landing rawPointer
}{
// ...
}The test as written is halfway between a unit test and an integration test (usually a bad thing): it checks results by using other functions instead of testing just this function.
It would be nice to have a small integration test as well. I know far pointers don't have as great test coverage, so it would help to have more.
There was a problem hiding this comment.
This is much better, thank you!
|
fixed according to comments, thanks for reviewing! |
rawpointer_test.go
Outdated
| for _, test := range tests { | ||
| testNearPointer := landingPadNearPointer(test.far, test.tag) | ||
| if testNearPointer != test.landing { | ||
| t.Errorf("rawPointer(%#016x).pointerType() = %d; want rawPointer(%#016x).pointerType() = %d", testNearPointer, testNearPointer.pointerType(), test.landing, test.landing.pointerType()) |
There was a problem hiding this comment.
I've already spent a fair amount of work making rawPointers values easy to debug. Please rewrite this as:
t.Errorf("landingPadNearPointer(%v, %v) = %v; want %v", test.far, test.tag, landing, test.landing)
rawpointer_test.go
Outdated
| tag rawPointer | ||
| landing rawPointer | ||
| }{ | ||
| {0x08, 0x2000200000000, 0x2000200000000}, |
There was a problem hiding this comment.
These tests would also benefit in being rewritten in terms of rawPointer constructors. For example, this first one would be:
{rawFarPointer(0, 8), rawStructPointer(0, ObjectSize{16, 2}), rawStructPointer(0, ObjectSize{16, 2})},There was a problem hiding this comment.
Hey, trying to do as you suggest here but I have a problem understanding something, would love if you could clarify, in the function rawFarPointer you do:
return farPointer | rawPointer(off&^7) | (rawPointer(segID) << 32).
why the AND NOT in here? aren't you going against the spec here?
from the example you given if I pass an offset of 8 to the rawFarPointer what I'll get is 1 in the offset section according to capnp spec and not the expected 8.
shouldn't be return farPointer | rawPointer(off) << 3 | (rawPointer(segID) << 32)?
There was a problem hiding this comment.
I've defined Address to be a byte address, not a word address. rawFarPointer is doing the conversion and bit-shifting at the same time by just lopping off the bottom three bits.
| @@ -232,3 +232,39 @@ func TestRawPointerTotalListSize(t *testing.T) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func TestLandingPadNearPointer(t *testing.T) { | |||
There was a problem hiding this comment.
This is much better, thank you!
rawpointer.go
Outdated
| @@ -56,7 +56,14 @@ func rawDoubleFarPointer(segID SegmentID, off Address) rawPointer { | |||
| // a near pointer in the destination segment. Its offset will be | |||
| // relative to the beginning of the segment. | |||
| func landingPadNearPointer(far, tag rawPointer) rawPointer { | |||
| return tag | rawPointer(far.farAddress()-Address(wordSize))<<2 | |||
| farFarAddress := far.farAddress() / Address(wordSize) | |||
There was a problem hiding this comment.
Since this variable is not a byte address (it's not a word address), let's inline this into the next line.
|
updated :) |
This pull request covers issue #72.