Stack manipulation opcodes - uncover and cover#2541
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2541 +/- ##
==========================================
+ Coverage 46.97% 47.01% +0.04%
==========================================
Files 348 348
Lines 55716 55766 +50
==========================================
+ Hits 26172 26220 +48
+ Misses 26600 26598 -2
- Partials 2944 2948 +4
Continue to review full report at Codecov.
|
|
Hello All, the PR build is failing due to "scripts/travis/codegen_verification.sh". Will look into this tomorrow morning (PST) |
jannotti
left a comment
jannotti
left a comment
There was a problem hiding this comment.
I think the shifts can be done clearer and faster with copy()
| func opCover(cx *evalContext) { | ||
| depth := int(uint(cx.program[cx.pc+1])) | ||
| idx := len(cx.stack) - 1 - depth | ||
| // Need to check stack size explicitly here because checkArgs() doesn't understand dig |
| return | ||
| } | ||
| topIndex := len(cx.stack) - 1 | ||
| Idxsv := cx.stack[idx] |
There was a problem hiding this comment.
Use lower case for locals
|
updated to use copy |
|
I'm realizing there is a tricky issue with these opcodes - normally our assembler and the Check() call try to keep track of the types on the stack. If you push an int, then a bytes, then use +, you will get an error at assembly time. (In the presence of branches, we downgrade this error to a warning). This can only happen because each opcode (so far) only manipulates the top of the stack. The OpSpec describes that manipulation in terms of types. But these opcodes are different. They manipulate the stack deeply, and potentially change the types of many elements (the element doesn't change, but the type at a particular location does, as they all shift). We need to figure out how that can be made to work. It might just be that they need their own check function (like the branches have) or it might be that they can explicitly manipulate the type stack during assembly. They might need both, or even more. First step, let's get a unit test in that shows the problem. Something like:
I think this will not type check, but it is correct (if I haven't messed up). |
jdtzmn
left a comment
jdtzmn
left a comment
There was a problem hiding this comment.
Mostly good, nice work on this! I left a few comments.
| testPanics(t, "int 3; int 2; int 1; dig 11; int 2; ==; return", 3) | ||
| } | ||
|
|
||
| func TestCover(t *testing.T) { |
There was a problem hiding this comment.
It would be nice to see test cases here that check that other elements are shifted correctly
algochoi
left a comment
algochoi
left a comment
There was a problem hiding this comment.
Good work! Just very minor comments.
Based on the Travis error, maybe try running make sanity on the root folder or go fmt again?
| cx.err = fmt.Errorf("cover %d with stack size = %d", depth, len(cx.stack)) | ||
| return | ||
| } | ||
| topidx := len(cx.stack) - 1 |
| cx.err = fmt.Errorf("uncover %d with stack size = %d", depth, len(cx.stack)) | ||
| return | ||
| } | ||
| topidx := len(cx.stack) - 1 |
There was a problem hiding this comment.
nit: topIdx
I think Go convention is to camelCase local variables
| return | ||
| } | ||
| topidx := len(cx.stack) - 1 | ||
| idxsv := cx.stack[idx] |
There was a problem hiding this comment.
nit: I think this should be sv (based on the dig opcode)
| return | ||
| } | ||
| topidx := len(cx.stack) - 1 | ||
| topsv := cx.stack[topidx] |
There was a problem hiding this comment.
nit: maybe sv (based on the dig opcode)?
|
It looks to me like all we need to do to address the type checking is to manipulate the type stack in the assemble function for cover/uncover. Currently they use asmDefault. So they'll need custom assemblers, but it should pretty much just call the default assembler, and manipulate the stack. |
|
They are byte immediates.
Still, it seems as though this double cast is unneeded. I recall seeing it
before, messing with it a bit, and for some reason leaving it. I can't see
why, right now. In C, I guess something similar would be needed, as char
is not defined to be signed or unsigned. But byte is
definitely unsigned in Go, and the simple cast to int should preserve the
sign because there's room in int for the entire range of byte.
…On Mon, Jul 19, 2021 at 10:09 AM Jacob Daitzman ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In data/transactions/logic/eval.go
<#2541 (comment)>:
> @@ -1700,6 +1700,36 @@ func opDig(cx *evalContext) {
cx.stack = append(cx.stack, sv)
}
+func opCover(cx *evalContext) {
+ depth := int(uint(cx.program[cx.pc+1]))
Aren't uint64 immediates required to be non-negative?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#2541 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADL7TZG4725TJYM6TFF64DTYQW2DANCNFSM5AKLPDSQ>
.
|
| } | ||
| returns[len(returns)-1] = sv | ||
| } | ||
| fmt.Println(returns) |
| returns[i] = StackAny | ||
| } | ||
| idx := len(ops.typeStack) - depth | ||
| if idx >= 0 { |
There was a problem hiding this comment.
Will the idx here ever be less than 0? Won't that be caught in the opCover function?
There was a problem hiding this comment.
I believe this check occurs first, as jj said below opCover should now be sure that it has the correct size stack
There was a problem hiding this comment.
This is actually because the arrays will create out of bounds error if you don't check here. It doesn't panic though
| idx := len(ops.typeStack) - depth | ||
| if idx >= 0 { | ||
| sv := ops.typeStack[len(ops.typeStack)-1] | ||
| for i := idx; i < len(ops.typeStack)-1; i++ { |
There was a problem hiding this comment.
Could this be shifted more cleanly using copy()?
There was a problem hiding this comment.
It could, but I'm to blame for setting the precedent with dig. We need cleaner helper functions for building these arrays in the type* functions. Then we can also do select, dup, dup.
There was a problem hiding this comment.
Agreed. Does it make sense to include those changes in this PR or should it be done in a separate one?
There was a problem hiding this comment.
I think we should do those separately. I'm still mulling over the right interface, but I think I'm imagining two things, one I'm fairly sure about, and one less so.
Fairly sure: the typeCheck functions should check the appropriate depth first, and return early if the stack is short (with an appropriately sized slice to cause checkStack to complain about length). This means the rest of the typeCheck function can assume the stack is deep enough, and it'll be easier to write the code (using copy!) that returns slices that match parts of the existing stack.
Less sure: the interface should be reworked a little so that immediates are parsed early in one place, and supplied in a structured way to the typeCheck and assemble functions. That way it only happens once, and assemble functions are no longer expected to do that parsing or error checking.
There was a problem hiding this comment.
I agree with the first one for sure, the second point is good as well - would that still mean if a program failed in typeCheck it fails during assembly?
| cx.err = fmt.Errorf("cover %d with stack size = %d", depth, len(cx.stack)) | ||
| return | ||
| } | ||
| topIdx := len(cx.stack) - 1 |
There was a problem hiding this comment.
This may read better if you move topIdx above idx like so:
topIdx := len(cx.stack - 1)
idx := topIdx - depth
| // Need to check stack size explicitly here because checkArgs() doesn't understand cover | ||
| // so we can't expect our stack to be prechecked. |
There was a problem hiding this comment.
This may not be true any more with the changes to checkStack. I think that now opCover is sure to have a big enough stack to work with.
There was a problem hiding this comment.
good point, i'll re visit this
| testAccepts(t, "int 4; int 3; int 2; int 1; uncover 3; pop; int 1; ==; return", 5) | ||
| testAccepts(t, "int 4; int 3; int 2; int 1; uncover 3; pop; pop; int 2; ==; return", 5) | ||
| testAccepts(t, "int 1; int 3; int 2; int 1; uncover 3; pop; pop; int 2; ==; return", 5) | ||
| testPanics(t, obfuscate("int 4; int 3; int 2; int 1; uncover 11; int 3; ==; return"), 5) |
There was a problem hiding this comment.
Can you add one that panics, but just barely? In other words, confirm theres no off-by-one error by uncovering with an argument that's just one too high.
algorandskiy
left a comment
algorandskiy
left a comment
There was a problem hiding this comment.
Looks good, one more test request and a minor code style fix
| "Byteslice Logic": {"b|", "b&", "b^", "b~"}, | ||
| "Loading Values": {"intcblock", "intc", "intc_0", "intc_1", "intc_2", "intc_3", "pushint", "bytecblock", "bytec", "bytec_0", "bytec_1", "bytec_2", "bytec_3", "pushbytes", "bzero", "arg", "arg_0", "arg_1", "arg_2", "arg_3", "txn", "gtxn", "txna", "gtxna", "gtxns", "gtxnsa", "global", "load", "store", "gload", "gloads", "gaid", "gaids"}, | ||
| "Flow Control": {"err", "bnz", "bz", "b", "return", "pop", "dup", "dup2", "dig", "swap", "select", "assert", "callsub", "retsub"}, | ||
| "Flow Control": {"err", "bnz", "bz", "b", "return", "pop", "dup", "dup2", "dig", "cover", "uncover", "swap", "select", "assert", "callsub", "retsub"}, |
There was a problem hiding this comment.
just wondering how did "pop", "dup", "dup2", "dig", "cover", "uncover" end up to be flow control
There was a problem hiding this comment.
I think it's because it used to only be pop and dup so we didn't create a category for stack manipulation.
|
|
||
| func TestUncover(t *testing.T) { | ||
| t.Parallel() | ||
| testAccepts(t, "int 4; int 3; int 2; int 1; uncover 2; int 3; ==; return", 5) |
There was a problem hiding this comment.
please add unvcover 0 test as an edge case test
| depth := int(cx.program[cx.pc+1]) | ||
| idx := len(cx.stack) - 1 - depth | ||
| // Need to check stack size explicitly here because checkArgs() doesn't understand uncover | ||
| // so we can't expect our stack to be prechecked. | ||
| if idx < 0 { | ||
| cx.err = fmt.Errorf("uncover %d with stack size = %d", depth, len(cx.stack)) | ||
| return | ||
| } | ||
| topIdx := len(cx.stack) - 1 |
There was a problem hiding this comment.
make it the same as prev function
| depth := int(cx.program[cx.pc+1]) | |
| idx := len(cx.stack) - 1 - depth | |
| // Need to check stack size explicitly here because checkArgs() doesn't understand uncover | |
| // so we can't expect our stack to be prechecked. | |
| if idx < 0 { | |
| cx.err = fmt.Errorf("uncover %d with stack size = %d", depth, len(cx.stack)) | |
| return | |
| } | |
| topIdx := len(cx.stack) - 1 | |
| depth := int(cx.program[cx.pc+1]) | |
| topIdx := len(cx.stack) - 1 | |
| idx := topIdx - depth | |
| // Need to check stack size explicitly here because checkArgs() doesn't understand uncover | |
| // so we can't expect our stack to be prechecked. | |
| if idx < 0 { | |
| cx.err = fmt.Errorf("uncover %d with stack size = %d", depth, len(cx.stack)) | |
| return | |
| } |
7 participants
This PR adds two opcodes.
cover: "remove top of stack, and place it down the stack such that N elements are above it",
uncover: "remove the value at depth N in the stack and shift above items down so the Nth deep value is on top of the stack"