Yul interpreter: Changes required before merging Redundant store eliminator #12276

bshastry · 2021-11-12T11:20:40Z

Required to fuzz #11352

bshastry · 2021-11-12T11:23:05Z

test/libyul/YulInterpreterTest.cpp

@@ -94,13 +94,13 @@ string YulInterpreterTest::interpret()
 	state.maxExprNesting = 64;
 	try
 	{
-		Interpreter::run(state, EVMDialect::strictAssemblyForEVMObjects(langutil::EVMVersion{}), *m_ast);
+		Interpreter::run(state, EVMDialect::strictAssemblyForEVMObjects(langutil::EVMVersion{}), *m_ast, false);


Memory tracing is enabled (last argument = false) for Yul interpreter tests. This may be changed to true if tests pass only without memory tracing.

bshastry · 2021-11-12T11:23:27Z

test/libyul/EwasmTranslationTest.cpp

@@ -108,13 +108,13 @@ string EwasmTranslationTest::interpret()
 	state.maxExprNesting = 64;
 	try
 	{
-		Interpreter::run(state, WasmDialect{}, *m_object->code);
+		Interpreter::run(state, WasmDialect{}, *m_object->code, false);


Memory tracing is enabled (last argument = false) for Ewasm->Yul tests. This may be changed to true if tests pass only without memory tracing.

bshastry · 2021-11-12T11:23:43Z

test/libyul/EwasmTranslationTest.cpp

 	}
 	catch (InterpreterTerminatedGeneric const&)
 	{
 	}

 	stringstream result;
-	state.dumpTraceAndState(result);
+	state.dumpTraceAndState(result, false);


Memory tracing enabled.

test/tools/ossfuzz/strictasm_diff_ossfuzz.cpp

bshastry · 2021-11-12T11:28:24Z

test/tools/yulInterpreter/EVMInstructionInterpreter.cpp

@@ -204,6 +205,7 @@ u256 EVMInstructionInterpreter::eval(
 	case Instruction::CALLDATASIZE:
 		return m_state.calldata.size();
 	case Instruction::CALLDATACOPY:
+		logTrace(_instruction, true, arg);


logTrace(instruction, true, arg) means interpret instruction I whose args are A... and tell the tracer that this instruction may write to memory.

If the interpreter is configured to disable memory tracing, this information (whether an instruction writes to memory or not) is useful in deciding whether to append the instruction to execution trace or not.

bshastry · 2021-11-12T11:29:15Z

test/tools/yulInterpreter/EVMInstructionInterpreter.cpp

@@ -99,6 +99,7 @@ u256 EVMInstructionInterpreter::eval(
 	switch (_instruction)
 	{
 	case Instruction::STOP:
+		logTrace(_instruction, false);


For instructions that do not write to memory such as stop(), the second argument is false

I'm wondering - shouldn't we use some utility to infer this fact from the opcode itself?

Sounds reasonable to me. More concretely, would it be a static look up table per opcode or an annotation to the opcode itself?

I think SemanticInformation::memory would be suitable. It is a slight problem since we cannot test this function here anymore, but I guess it's better to use constants here all the time.

Updated to make use of SemanticInformation::memory. If there are any mistaken assumptions, the fuzzer would report a bug which we could later investigate.

test/tools/yulInterpreter/EVMInstructionInterpreter.cpp

chriseth · 2021-11-15T13:42:58Z

Can you summarize the change? Do we at least compare the data in storage at the end?

bshastry · 2021-11-15T18:55:27Z

Can you summarize the change? Do we at least compare the data in storage at the end?

Storage is compared as before (no changes there).
Memory changes are not recorded if the flag added in this PR is set (disableMemoryTracing = true)
Memory dumps are not compared if the flag added in this PR is set (disableMemoryTracing = true)

hrkrshnn · 2022-01-03T18:11:26Z

Storage is compared as before (no changes there).

@bshastry How is that done? I have a feeling that might not be enough for something like

{
    let x := calldataload(0)
    let y := sload(x)
    // both of these can be removed
    sstore(x, y)
    sstore(x, y)

    let a := x
    let b := mload(a)
    // both of these can be removed
    mstore(a, b)
    mstore(a, b)
}
// ----
// step: equalStoreEliminator
//
// {
//     let x := calldataload(0)
//     let y := sload(x)
//     let a := x
//     let b := mload(a)
// }

bshastry · 2022-01-04T08:47:49Z

Storage is compared as before (no changes there).

@bshastry How is that done? I have a feeling that might not be enough for something like

{
    let x := calldataload(0)
    let y := sload(x)
    // both of these can be removed
    sstore(x, y)
    sstore(x, y)

    let a := x
    let b := mload(a)
    // both of these can be removed
    mstore(a, b)
    mstore(a, b)
}
// ----
// step: equalStoreEliminator
//
// {
//     let x := calldataload(0)
//     let y := sload(x)
//     let a := x
//     let b := mload(a)
// }

Essentially the following

sstore(a, b): an entry in map is created but not an instruction trace SSTORE(a, b)
mstore(a, b) is neither recorded in state nor trace

Since storage writes are always preserved in map, they may be compared pre/post optimisation. OTOH, memory may not be compared.

…ing. Model revert in yul interpreter. Add logTrace for a few more instructions and clear trace on revert.

chriseth · 2022-01-04T14:58:41Z

test/libyul/YulInterpreterTest.cpp

+			state,
+			EVMDialect::strictAssemblyForEVMObjects(langutil::EVMVersion{}),
+			*m_ast,
+			/*disableMemoryTracing=*/false


Maybe avoid the negation and call it enableMemoryTracing?

chriseth · 2022-01-04T15:02:26Z

test/tools/yulInterpreter/EVMInstructionInterpreter.cpp

@@ -339,12 +344,18 @@ u256 EVMInstructionInterpreter::eval(
 	case Instruction::REVERT:
 		accessMemory(arg[0], arg[1]);
 		logTrace(_instruction, arg);
+		m_state.storage.clear();


How "correct" is this interpreter? Shouldn't it only clear the changes to storage? Should we extract this change into its own PR?

chriseth · 2022-01-04T15:03:43Z

Could be merged like this.

bshastry commented Nov 12, 2021

View reviewed changes

bshastry force-pushed the fuzz-RSE branch 2 times, most recently from ecc80c6 to 9bf34de Compare November 14, 2021 17:24

bshastry mentioned this pull request Nov 14, 2021

Redundant store eliminator. #11352

Closed

bshastry commented Nov 14, 2021

View reviewed changes

test/tools/yulInterpreter/EVMInstructionInterpreter.cpp Outdated Show resolved Hide resolved

bshastry marked this pull request as ready for review November 22, 2021 11:13

cameel added optimizer testing 🔨 labels Dec 10, 2021

bshastry force-pushed the fuzz-RSE branch 4 times, most recently from bd97e62 to e203708 Compare January 3, 2022 17:14

bshastry mentioned this pull request Jan 3, 2022

Impose stricter upper bound on memory accesses in order to prevent wrap around #12484

Merged

bshastry force-pushed the fuzz-RSE branch from e203708 to 21a6f93 Compare January 4, 2022 10:52

Yul interpreter: Add flag to disable memory tracing and dump for fuzz…

b8ad2b2

…ing. Model revert in yul interpreter. Add logTrace for a few more instructions and clear trace on revert.

bshastry force-pushed the fuzz-RSE branch from 21a6f93 to b8ad2b2 Compare January 4, 2022 10:56

chriseth reviewed Jan 4, 2022

View reviewed changes

chriseth approved these changes Jan 5, 2022

View reviewed changes

chriseth merged commit 679f73c into develop Jan 5, 2022

chriseth deleted the fuzz-RSE branch January 5, 2022 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yul interpreter: Changes required before merging Redundant store eliminator #12276

Yul interpreter: Changes required before merging Redundant store eliminator #12276

bshastry commented Nov 12, 2021

bshastry Nov 12, 2021

bshastry Nov 12, 2021

bshastry Nov 12, 2021

bshastry Nov 12, 2021

bshastry Nov 12, 2021

chriseth Jan 3, 2022

bshastry Jan 3, 2022

chriseth Jan 3, 2022

chriseth Jan 3, 2022

bshastry Jan 3, 2022

chriseth commented Nov 15, 2021

bshastry commented Nov 15, 2021

hrkrshnn commented Jan 3, 2022

bshastry commented Jan 4, 2022

chriseth Jan 4, 2022

chriseth Jan 4, 2022

chriseth commented Jan 4, 2022

Yul interpreter: Changes required before merging Redundant store eliminator #12276

Yul interpreter: Changes required before merging Redundant store eliminator #12276

Conversation

bshastry commented Nov 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Nov 15, 2021

bshastry commented Nov 15, 2021

hrkrshnn commented Jan 3, 2022

bshastry commented Jan 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriseth commented Jan 4, 2022