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

Ensure disassemble/reassemble cycle works in testProg. #2745

Merged
merged 2 commits into from
Aug 23, 2021
Merged

Ensure disassemble/reassemble cycle works in testProg. #2745

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bddca4d
Ensure disassemble/reassemble cycle works in testProg.
jannotti Aug 14, 2021
001848b
Explain `rerun`
jannotti Aug 17, 2021
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
26 changes: 20 additions & 6 deletions data/transactions/logic/assembler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1938,6 +1938,7 @@ type disassembleState struct {
numericTargets bool
labelCount int
pendingLabels map[int]string
rerun bool
Copy link
Contributor

Choose a reason for hiding this comment

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

please add a comment for rerun, it is not obvious at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment added


nextpc int
err error
Expand All @@ -1951,6 +1952,9 @@ func (dis *disassembleState) putLabel(label string, target int) {
dis.pendingLabels = make(map[int]string)
}
dis.pendingLabels[target] = label
if target <= dis.pc {
dis.rerun = true
}
}

func (dis *disassembleState) outputLabelIfNeeded() (err error) {
Expand Down Expand Up @@ -2412,11 +2416,13 @@ type disInfo struct {
hasStatefulOps bool
}

// disassembleInstrumented is like Disassemble, but additionally returns where
// each program counter value maps in the disassembly
func disassembleInstrumented(program []byte) (text string, ds disInfo, err error) {
// disassembleInstrumented is like Disassemble, but additionally
// returns where each program counter value maps in the
// disassembly. If the labels names are known, they may be passed in.
// When doing so, labels for all jump targets must be provided.
func disassembleInstrumented(program []byte, labels map[int]string) (text string, ds disInfo, err error) {
out := strings.Builder{}
dis := disassembleState{program: program, out: &out}
dis := disassembleState{program: program, out: &out, pendingLabels: labels}
version, vlen := binary.Uvarint(program)
if vlen <= 0 {
fmt.Fprintf(dis.out, "// invalid version\n")
Expand Down Expand Up @@ -2468,18 +2474,26 @@ func disassembleInstrumented(program []byte) (text string, ds disInfo, err error
}

text = out.String()

if dis.rerun {
if labels != nil {
err = errors.New("rerun even though we had labels")
return
}
return disassembleInstrumented(program, dis.pendingLabels)
}
return
}

// Disassemble produces a text form of program bytes.
// AssembleString(Disassemble()) should result in the same program bytes.
func Disassemble(program []byte) (text string, err error) {
text, _, err = disassembleInstrumented(program)
text, _, err = disassembleInstrumented(program, nil)
return
}

// HasStatefulOps checks if the program has stateful opcodes
func HasStatefulOps(program []byte) (bool, error) {
_, ds, err := disassembleInstrumented(program)
_, ds, err := disassembleInstrumented(program, nil)
return ds.hasStatefulOps, err
}
37 changes: 20 additions & 17 deletions data/transactions/logic/assembler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,19 +384,11 @@ func TestAssembleAlias(t *testing.T) {
t.Parallel()
source1 := `txn Accounts 0 // alias to txna
pop
gtxn 0 ApplicationArgs 0 // alias to gtxn
gtxn 0 ApplicationArgs 0 // alias to gtxna
pop
`
ops1, err := AssembleStringWithVersion(source1, AssemblerMaxVersion)
require.NoError(t, err)

source2 := `txna Accounts 0
pop
gtxna 0 ApplicationArgs 0
pop
`
ops2, err := AssembleStringWithVersion(source2, AssemblerMaxVersion)
require.NoError(t, err)
ops1 := testProg(t, source1, AssemblerMaxVersion)
ops2 := testProg(t, strings.Replace(source1, "txn", "txna", -1), AssemblerMaxVersion)

require.Equal(t, ops1.Program, ops2.Program)
}
Expand Down Expand Up @@ -431,6 +423,20 @@ func testProg(t testing.TB, source string, ver uint64, expected ...expect) *OpSt
require.NoError(t, err)
require.NotNil(t, ops)
require.NotNil(t, ops.Program)
// It should always be possible to Disassemble
dis, err := Disassemble(ops.Program)
require.NoError(t, err)
// And, while the disassembly may not match input
// exactly, the assembly of the disassembly should
// give the same bytecode
ops2, err := AssembleStringWithVersion(dis, ver)
if len(ops2.Errors) > 0 || err != nil || ops2 == nil || ops2.Program == nil {
t.Log(program)
t.Log(dis)
}
require.Empty(t, ops2.Errors)
require.NoError(t, err)
require.Equal(t, ops.Program, ops2.Program)
} else {
require.Error(t, err)
errors := ops.Errors
Expand Down Expand Up @@ -592,8 +598,7 @@ func TestAssembleInt(t *testing.T) {
}

text := "int 0xcafebabe"
ops, err := AssembleStringWithVersion(text, v)
require.NoError(t, err)
ops := testProg(t, text, v)
s := hex.EncodeToString(ops.Program)
require.Equal(t, mutateProgVersion(v, expected), s)
})
Expand Down Expand Up @@ -643,8 +648,7 @@ func TestAssembleBytes(t *testing.T) {
}

for _, vi := range variations {
ops, err := AssembleStringWithVersion(vi, v)
require.NoError(t, err)
ops := testProg(t, vi, v)
s := hex.EncodeToString(ops.Program)
require.Equal(t, mutateProgVersion(v, expected), s)
}
Expand Down Expand Up @@ -1184,8 +1188,7 @@ intc 0
intc 0
bnz done
done:`
ops, err := AssembleStringWithVersion(source, AssemblerMaxVersion)
require.NoError(t, err)
ops := testProg(t, source, AssemblerMaxVersion)
require.Equal(t, 9, len(ops.Program))
expectedProgBytes := []byte("\x01\x20\x01\x01\x22\x22\x40\x00\x00")
expectedProgBytes[0] = byte(AssemblerMaxVersion)
Expand Down
2 changes: 1 addition & 1 deletion data/transactions/logic/debugger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func GetProgramID(program []byte) string {
}

func makeDebugState(cx *evalContext) DebugState {
disasm, dsInfo, err := disassembleInstrumented(cx.program)
disasm, dsInfo, err := disassembleInstrumented(cx.program, nil)
if err != nil {
// Report disassembly error as program text
disasm = err.Error()
Expand Down
43 changes: 15 additions & 28 deletions data/transactions/logic/eval_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1034,7 +1034,7 @@ func TestGlobal(t *testing.T) {
addr, err := basics.UnmarshalChecksumAddress(testAddr)
require.NoError(t, err)
ledger.creatorAddr = addr
for v := uint64(0); v <= AssemblerMaxVersion; v++ {
for v := uint64(1); v <= AssemblerMaxVersion; v++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it changed to 1? 0 is used to be a valid version for TEAL 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The trouble is that when AssembleStringWithVersion is given 0 as the version, the actual assembled version is 1, and so #pragma version 1 is generated in the disassembly. But then AssembleStringWithVersion(dissassembly, 0) fails to reassemble it, because the source specifies 1 when the assembler was explicitly called with 0. We could probably make all this work, but AssembleStringWithVersion is not actually used outside of testing, so it seems pointless to make it clever about accepting 1 when called with 0 (which might be reasonable!). The only public use invokes it with assembleNoVersion. None of the other versioning tests test against 0, they all start at 1.

Copy link
Contributor

@algorandskiy algorandskiy Aug 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm ok to have it set 1 here. The only problem this looks like the last place where assembling with version 0 happens. Probably a separate test checking assembling with version 0 at backwardCompat_test.go is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a good reason for AssembleStringWithVersion(s, 0) to work at all. There's no "user-level" way to cause the function to run that way. It always runs with the version assemblerNoVersion when invoked to assemble teal outside of tests. I don't think we need to be compatible with old test code. I don't think the WithVersion function should even be public. And it should probably fail if given 0.

_, ok := tests[v]
require.True(t, ok)
t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) {
Expand All @@ -1055,9 +1055,6 @@ func TestGlobal(t *testing.T) {
txgroup := make([]transactions.SignedTxn, 1)
txgroup[0] = txn
sb := strings.Builder{}
block := bookkeeping.Block{}
block.BlockHeader.Round = 999999
block.BlockHeader.TimeStamp = 2069
proto := config.ConsensusParams{
MinTxnFee: 123,
MinBalance: 1000000,
Expand Down Expand Up @@ -2684,10 +2681,9 @@ func TestStackUnderflow(t *testing.T) {
t.Parallel()
for v := uint64(1); v <= AssemblerMaxVersion; v++ {
t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) {
ops, err := AssembleStringWithVersion(`int 1`, v)
ops := testProg(t, `int 1`, v)
ops.Program = append(ops.Program, 0x08) // +
require.NoError(t, err)
err = Check(ops.Program, defaultEvalParams(nil, nil))
err := Check(ops.Program, defaultEvalParams(nil, nil))
require.NoError(t, err)
sb := strings.Builder{}
pass, err := Eval(ops.Program, defaultEvalParams(&sb, nil))
Expand All @@ -2707,10 +2703,9 @@ func TestWrongStackTypeRuntime(t *testing.T) {
t.Parallel()
for v := uint64(1); v <= AssemblerMaxVersion; v++ {
t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) {
ops, err := AssembleStringWithVersion(`int 1`, v)
require.NoError(t, err)
ops := testProg(t, `int 1`, v)
ops.Program = append(ops.Program, 0x01, 0x15) // sha256, len
err = Check(ops.Program, defaultEvalParams(nil, nil))
err := Check(ops.Program, defaultEvalParams(nil, nil))
require.NoError(t, err)
sb := strings.Builder{}
pass, err := Eval(ops.Program, defaultEvalParams(&sb, nil))
Expand All @@ -2730,11 +2725,9 @@ func TestEqMismatch(t *testing.T) {
t.Parallel()
for v := uint64(1); v <= AssemblerMaxVersion; v++ {
t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) {
ops, err := AssembleStringWithVersion(`byte 0x1234
int 1`, v)
require.NoError(t, err)
ops := testProg(t, `byte 0x1234; int 1`, v)
ops.Program = append(ops.Program, 0x12) // ==
err = Check(ops.Program, defaultEvalParams(nil, nil))
err := Check(ops.Program, defaultEvalParams(nil, nil))
require.NoError(t, err) // TODO: Check should know the type stack was wrong
sb := strings.Builder{}
pass, err := Eval(ops.Program, defaultEvalParams(&sb, nil))
Expand All @@ -2754,11 +2747,9 @@ func TestNeqMismatch(t *testing.T) {
t.Parallel()
for v := uint64(1); v <= AssemblerMaxVersion; v++ {
t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) {
ops, err := AssembleStringWithVersion(`byte 0x1234
int 1`, v)
require.NoError(t, err)
ops := testProg(t, `byte 0x1234; int 1`, v)
ops.Program = append(ops.Program, 0x13) // !=
err = Check(ops.Program, defaultEvalParams(nil, nil))
err := Check(ops.Program, defaultEvalParams(nil, nil))
require.NoError(t, err) // TODO: Check should know the type stack was wrong
sb := strings.Builder{}
pass, err := Eval(ops.Program, defaultEvalParams(&sb, nil))
Expand All @@ -2778,11 +2769,9 @@ func TestWrongStackTypeRuntime2(t *testing.T) {
t.Parallel()
for v := uint64(1); v <= AssemblerMaxVersion; v++ {
t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) {
ops, err := AssembleStringWithVersion(`byte 0x1234
int 1`, v)
require.NoError(t, err)
ops := testProg(t, `byte 0x1234; int 1`, v)
ops.Program = append(ops.Program, 0x08) // +
err = Check(ops.Program, defaultEvalParams(nil, nil))
err := Check(ops.Program, defaultEvalParams(nil, nil))
require.NoError(t, err)
sb := strings.Builder{}
pass, _ := Eval(ops.Program, defaultEvalParams(&sb, nil))
Expand All @@ -2802,15 +2791,14 @@ func TestIllegalOp(t *testing.T) {
t.Parallel()
for v := uint64(1); v <= AssemblerMaxVersion; v++ {
t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) {
ops, err := AssembleStringWithVersion(`int 1`, v)
require.NoError(t, err)
ops := testProg(t, `int 1`, v)
for opcode, spec := range opsByOpcode[v] {
if spec.op == nil {
ops.Program = append(ops.Program, byte(opcode))
break
}
}
err = Check(ops.Program, defaultEvalParams(nil, nil))
err := Check(ops.Program, defaultEvalParams(nil, nil))
require.Error(t, err)
sb := strings.Builder{}
pass, err := Eval(ops.Program, defaultEvalParams(&sb, nil))
Expand All @@ -2830,15 +2818,14 @@ func TestShortProgram(t *testing.T) {
t.Parallel()
for v := uint64(1); v <= AssemblerMaxVersion; v++ {
t.Run(fmt.Sprintf("v=%d", v), func(t *testing.T) {
ops, err := AssembleStringWithVersion(`int 1
ops := testProg(t, `int 1
bnz done
done:
int 1
`, v)
require.NoError(t, err)
// cut two last bytes - intc_1 and last byte of bnz
ops.Program = ops.Program[:len(ops.Program)-2]
err = Check(ops.Program, defaultEvalParams(nil, nil))
err := Check(ops.Program, defaultEvalParams(nil, nil))
require.Error(t, err)
sb := strings.Builder{}
pass, err := Eval(ops.Program, defaultEvalParams(&sb, nil))
Expand Down