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

Far far pointer bug fix #71

Merged
merged 12 commits into from
Mar 24, 2017
Merged

Far far pointer bug fix #71

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
58892e1
Fixed a bug related to double far pointers
levrado Mar 19, 2017
a6fa529
Added test
levrado Mar 19, 2017
bd87c29
change test to more readble
levrado Mar 19, 2017
3dfb252
Update rawpointer.go
levrado Mar 19, 2017
2e31f4f
changed test
levrado Mar 22, 2017
df49260
added myself to AUTHORS
levrado Mar 22, 2017
6c5ce07
Added myself to CONTRIBUTORS
levrado Mar 22, 2017
6a611fb
fixed typo
levrado Mar 22, 2017
3f327e2
sorting
levrado Mar 22, 2017
7c03026
sorting
levrado Mar 22, 2017
89a0941
change in test val
levrado Mar 22, 2017
5ab5f6c
changes according to comments
levrado Mar 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,6 @@ James McKaskill <[email protected]>
Jason E. Aten <[email protected]>
Johan Hernandez <[email protected]>
Joonsung Lee <[email protected]>
Lev Radomislensky <[email protected]>
Peter Waldschmidt <[email protected]>
William Laffin <[email protected]>
1 change: 1 addition & 0 deletions CONTRIBUTORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ James McKaskill <[email protected]>
Jason E. Aten <[email protected]>
Johan Hernandez <[email protected]>
Joonsung Lee <[email protected]>
Lev Radomislensky <[email protected]>
Peter Waldschmidt <[email protected]>
Ross Light <[email protected]> <[email protected]>
William Laffin <[email protected]>
9 changes: 8 additions & 1 deletion rawpointer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this variable is not a byte address (it's not a word address), let's inline this into the next line.


// remove 1 word and we'll get the offset to the start of the data section
rawFarFarPointer := rawPointer(farFarAddress - 1)

// finally to get an actual pointer do an OR with the tag after moving rawFarFarPointer
// by 2 bits which are the type indicator bits
return tag | rawFarFarPointer << 2
}

// Raw pointer types.
Expand Down
21 changes: 21 additions & 0 deletions rawpointer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,3 +232,24 @@ func TestRawPointerTotalListSize(t *testing.T) {
}
}
}

func TestLandingPadNearPointer(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better, thank you!

tests := []struct {
far rawPointer
tag rawPointer
landing rawPointer
}{
{0x08, 0x2000200000000, 0x2000200000000},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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})},

Copy link
Contributor Author

@levrado levrado Mar 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{0xa0, 0x2000200000000, 0x200020000004c},
{0xb12, 0x2000200000000, 0x2000200000584}, // struct pointer
{0xb12, 0x2000200000001, 0x2000200000585}, // list pointer
{0xb12, 0x2000200000003, 0x2000200000587}, // capability pointer
}

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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}
}
}