trying reuse filetests #211

MCJOHN974 · 2024-02-08T09:57:39Z

Just small attempt to reuse some infra from filetests, for example .clif runtests parsing. Not ready to use

MCJOHN974 · 2024-02-12T12:53:20Z

This PR aims only integrate some code to filetest infrastructure, reuse .clif files parsing, and just to compile some clif to zkasm and print it to stdout. No new checks performed.

It would be followed up by next set of PRs:

adding assert as host function to zkasm, such that assert fail don't halt the execution.
assembling generated pieces of zkasm to one program
final assembling of all previous work together & migrating CI from old version to new version.

MCJOHN974 · 2024-02-12T13:28:22Z

@Akashin, @mooori it's ready for review now.

cranelift/filetests/filetests/runtests/arithmetic.clif

aborg-dev · 2024-02-12T14:06:12Z

cranelift/filetests/filetests/runtests/simple.clif

Can we use existing arithmetic.clif or is there some reason it wouldn't work?

arithmetic.clif panics for now, and changes to fix it worth separate PR

cranelift/filetests/src/test_run_zkasm.rs

aborg-dev · 2024-02-12T14:15:42Z

cranelift/filetests/src/test_run_zkasm.rs

+
+                // just wrap to make CI green until executor is not ready
+                // TODO: replace by actual execution and results checking
+                let _ = panic::catch_unwind(AssertUnwindSafe(|| {


I think we should use panic::catch_unwind rather sparingly as it increases the chance of catching and ignoring the wrong types of errors. Instead, we should rely on std::result and check that we get the appropriate error code.

In this case, though, I think we can simplify things by removing the todo! statement from the end of execute and then we don't need the catch_unwind at all.

From once side yes, it will simplify, from the other side, this annoying catch_unwind and todo! explicitly marks that this parts are not ready and should be rewritten

aborg-dev · 2024-02-12T14:21:20Z

cranelift/filetests/src/test_run_zkasm.rs

+    Ok(())
+}
+
+fn generate_zkasm(_func_store: &FunctionStore, func: &Function) -> Vec<String> {


It's not great that we duplicate the code from https://github.com/near/wasmtime/blob/main/cranelift/filetests/src/test_zkasm.rs. We need to move the shared bits into a separate module.
For example, we can start by creating a module in cranelift/filetests/src/zkasm_codegen.rs and moving the necessary functions from https://github.com/near/wasmtime/blob/main/cranelift/filetests/src/test_zkasm.rs for this PR. Later, we can move all relevant functions.

I think we will remove test_zkasm.rs once test_run_zkasm.rs will be ready. And than do some refactoring, such as creating zkasm_codegen.rs if it will be needed.

I agree that code duplication like this should be avoided. This code might be touched for upcoming refactorings or zkASM instrumentation. Keeping duplicate code in sync is tedious and if it gets out of sync that might lead to issues which are hard to debug.

I'm definitely agree that we don't want have a code duplication. But, one more problem, it's not pure duplication, this functions are rewrited a bit for test_run_zkasm.rs. And, until filetest infra is not ready we want to keep both versions -- one to keep existing test infra alive, the second one -- to make iterative progress to the new test infra.

As alternative, I can suggest to create some filtest-dev branch. Code still will be reviewed before go to that branch, but we can make some exceptions for such problems

Hopefully, #216 can be a good start for the refactoring to avoid duplication.

But, one more problem, it's not pure duplication, this functions are rewrited a bit for test_run_zkasm.rs

Can you please document the necessary changes so that it is easier to reconcile these two functions in the future?

MCJOHN974 · 2024-02-13T14:19:07Z

Btw does anybody knows what this failed windows tests telling? I see cranelift\filetests\src\zkasm_runner.rs:117:39: but we even don't have such file... Is this file generated during compilation?..

aborg-dev · 2024-02-13T14:35:22Z

Btw does anybody knows what this failed windows tests telling? I see cranelift\filetests\src\zkasm_runner.rs:117:39: but we even don't have such file... Is this file generated during compilation?..

This file was added in #213 - have you rebased on the latest main branch?

MCJOHN974 · 2024-02-14T00:31:11Z

Last commit changed structure of test_run_zkasm.rs a lot, and now it builds some .zkasm program from whole test file. It is still WIP, and I left a lot of TODOs, but I think it can be merged as iteration step

aborg-dev · 2024-02-14T08:26:49Z

cranelift/filetests/src/test_run_zkasm.rs

+        _: &'a Flags,
+        _: Option<&'a dyn TargetIsa>,
+    ) -> anyhow::Result<()> {
+        let mut zkasm_functions: Vec<Vec<String>> = Default::default();


Nit: Given that we have a simple Vec type on the left, we could use a more specific Vec::new() instead of Default::default():

This will avoid bringing in additional dependency on Default

The resulting code will be simpler (shorter; don't have to know what is the Default::default() for Vec)

aborg-dev · 2024-02-14T08:28:58Z

cranelift/filetests/src/test_run_zkasm.rs

+        let mut zkasm_functions: Vec<Vec<String>> = Default::default();
+        let mut invocations: Vec<Vec<String>> = Default::default();
+        for (func, details) in &testfile.functions {
+            let zkasm_function = zkasm_codegen::compile_clif_function(func);


Nit: I don't think zkasm_function temporary variable adds any more clarity to the code, consider inlining it:

zkasm_functions.push(zkasm_codegen::compile_clif_function(func));

aborg-dev · 2024-02-14T08:37:46Z

cranelift/filetests/src/test_run_zkasm.rs

+                        RunCommand::Print(_) => {
+                            unreachable!()
+                        }
+                        RunCommand::Run(invoce, compare, expected) => {


Counterintuitive, but a verb from "invocation" is "invoke".

Hehe, I was thinking for a moment that "invoke" looks much better, thanks!

aborg-dev · 2024-02-14T08:38:21Z

cranelift/filetests/src/test_run_zkasm.rs

+                            unreachable!()
+                        }
+                        RunCommand::Run(invoce, compare, expected) => {
+                            let invocation =


Ditto: this temporary is not adding much value

aborg-dev · 2024-02-14T08:40:13Z

cranelift/filetests/src/zkasm_codegen.rs

+    let isa = isa_builder
+        .finish(settings::Flags::new(flag_builder))
+        .unwrap();
+    let mut comp_ctx = cranelift_codegen::Context::for_function(func.clone());


Maybe "comp_ctx" -> "context" to follow the same convention as generate_zkasm above?

aborg-dev · 2024-02-14T08:57:13Z

cranelift/filetests/src/zkasm_codegen.rs

+// now it works for one very basic case
+pub fn build_test_zkasm(functions: Vec<Vec<String>>, invocations: Vec<Vec<String>>) -> String {
+    // TODO: use generate_preambule to get preambule
+    let preambule = "start:\n  zkPC + 2 => RR\n  :JMP(main)\n  :JMP(finalizeExecution)";


Nits:

"preambule" -> "preamble"

Let's make it more readable by splitting across multiple lines:

let preamble = "\ start: zkPC + 2 => RR :JMP(main) :JMP(finalizeExecution)";

aborg-dev · 2024-02-14T09:04:10Z

cranelift/filetests/src/zkasm_codegen.rs

+    ];
+    for invoce in invocations {
+        // TODO: remove this .clone()
+        main.append(&mut invoce.clone());


You should be able to use Vec::extend for this with something like

main.extend(invocations.iter().cloned())

main.extend() expects Vec<String> while invocations is Vec<Vec<String>>. And I'm not sure but I guess this .cloned() performs same amount of copying as .clone(), isn't it?

But

for invocation in invocations { main.extend(invocation); }

works.

aborg-dev · 2024-02-14T09:04:56Z

cranelift/filetests/src/zkasm_codegen.rs

+    }
+    main.push("  SP - 1 => SP".to_string());
+    main.push("  :JMP(RR)".to_string());
+    let mut postabmule = generate_postamble();


Nit: "postabmule" -> "postamble"

aborg-dev · 2024-02-14T09:07:25Z

cranelift/filetests/src/test_run_zkasm.rs

+                if let Some(command) = parse_run_command(comment.text, &func.signature)? {
+                    match command {
+                        RunCommand::Print(_) => {
+                            unreachable!()


This should be a todo!(), right?

Hmm, I don't know really now. I didn't see any Print command yet. And if we will somehow adopt cranelift tests to zkasm (for example remove test interpret from the header) removing print commands is normal strategy imo. But maybe todo!() is really better here

MCJOHN974 · 2024-02-14T12:13:21Z

Last commit fixes review notes (thanks @Akashin). Btw, I see that main fails windows tests at zkasm_runner.rs as well, so we just ignore them, am I right?

aborg-dev · 2024-02-15T09:17:46Z

Last commit fixes review notes (thanks @Akashin). Btw, I see that main fails windows tests at zkasm_runner.rs as well, so we just ignore them, am I right?

I'm working on fixing windows tests, but this should not block submitting this PR.

mooori · 2024-02-15T09:42:57Z

cranelift/filetests/filetests/runtests/simple.clif

+; run: %add_i64(0, 0) == 0
+; run: %add_i64(0, 1) == -1


Shouldn't these call %sub_i64?

Yes, thanks (= But this file is not supposed to really test something, it is more for me to debug the filetest

mooori · 2024-02-15T09:48:17Z

cranelift/filetests/src/zkasm_codegen.rs

+// TODO: this function should be much rewrited,
+// now it works for one very basic case


Could describe the one case it works for and outline how it needs to be rewritten.

It must be rewritten before we will migrate to this infra (optimistically before end of Feb), and I think nobody except me will do this, so this comment is just a signal for reviewers to not ask replace this assembling to something better, because I will do it in next PRs

mooori · 2024-02-15T09:48:55Z

cranelift/filetests/src/zkasm_codegen.rs

+// TODO: this function should be much rewrited,
+// now it works for one very basic case
+pub fn build_test_zkasm(functions: Vec<Vec<String>>, invocations: Vec<Vec<String>>) -> String {
+    // TODO: use generate_preambule to get preambule


Suggested change

// TODO: use generate_preambule to get preambule

// TODO: use generate_preamble to get preamble

MCJOHN974 · 2024-02-15T12:53:18Z

Thanks, @mooori, this commit fixes notes you find

MCJOHN974 · 2024-02-15T12:55:12Z

I'm working on fixing windows tests, but this should not block submitting this PR.

Thanks! I bet merging this PR is not blocking me and we can wait windows tests fix

MCJOHN974 · 2024-02-22T09:52:16Z

@Akashin, now CI is green and all review notes marked as outdated. Should we merge this?

aborg-dev

Yes, let's ship this!

MCJOHN974 added 2 commits February 8, 2024 12:55

trying reuse filetests

6718bf0

stage-1: cleanup

bfa4c6f

MCJOHN974 marked this pull request as ready for review February 12, 2024 13:27

MCJOHN974 requested review from aborg-dev and mooori February 12, 2024 13:35

aborg-dev suggested changes Feb 12, 2024

View reviewed changes

MCJOHN974 added 3 commits February 12, 2024 19:25

restore arithmetic.clif

fc8d077

removing outdated comments

7076d72

newline in the end of arithmetic.clif

28e2159

aborg-dev mentioned this pull request Feb 13, 2024

Move zkasm generation into zkasm_codegen.rs #216

Merged

MCJOHN974 added 3 commits February 14, 2024 00:44

Merge branch 'main' into viktar/filetests

de489ea

dedup

73ae77c

filetests: zkasm-building

1f09832

MCJOHN974 requested a review from aborg-dev February 14, 2024 00:31

aborg-dev reviewed Feb 14, 2024

View reviewed changes

review fixes

8edd7c5

mooori reviewed Feb 15, 2024

View reviewed changes

review fixes v2

c747cd4

Merge branch 'main' into viktar/filetests

1f8eaea

aborg-dev approved these changes Feb 22, 2024

View reviewed changes

MCJOHN974 added this pull request to the merge queue Feb 22, 2024

Merged via the queue into main with commit d174ee5 Feb 22, 2024
31 checks passed

MCJOHN974 deleted the viktar/filetests branch February 22, 2024 12:54

aborg-dev mentioned this pull request Feb 26, 2024

[ProjectTracking]: ZK WASM Stage 2, WASM Interpreter near/near-one-project-tracking#6

Closed

		// TODO: this function should be much rewrited,
		// now it works for one very basic case

	// TODO: use generate_preambule to get preambule
	// TODO: use generate_preamble to get preamble

trying reuse filetests #211

trying reuse filetests #211

Conversation

MCJOHN974 commented Feb 8, 2024

MCJOHN974 commented Feb 12, 2024 • edited Loading

MCJOHN974 commented Feb 12, 2024

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

MCJOHN974 commented Feb 13, 2024

aborg-dev commented Feb 13, 2024

MCJOHN974 commented Feb 14, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MCJOHN974 commented Feb 14, 2024

aborg-dev commented Feb 15, 2024

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

MCJOHN974 commented Feb 15, 2024

MCJOHN974 commented Feb 15, 2024

MCJOHN974 commented Feb 22, 2024

aborg-dev left a comment

Choose a reason for hiding this comment

MCJOHN974 commented Feb 12, 2024 •

edited

Loading