Skip to content

Comments

test: add tests for overflowing arith#959

Merged
mooori merged 4 commits intonextfrom
mooori/test-arith-ops
Feb 20, 2026
Merged

test: add tests for overflowing arith#959
mooori merged 4 commits intonextfrom
mooori/test-arith-ops

Conversation

@mooori
Copy link
Contributor

@mooori mooori commented Feb 13, 2026

Addresses the overflowing part of #912.

Even though it doesn't fully resolve #912, I'd like to put this PR up for review to get feedback on the approach of using a generic function instead of macros.

Generated files in expected/

Should they be generated and committed for these tests? For now I did that to be consistent with existing tests.

For passing tests I did not review these files, assuming matching rust_out and vm_out is sufficient. Though the generated files can be very helpful to get an idea of generated code and inspect it in case of failures. So another option could be writing these files only if specifically requested or when an error occurs.

@greenhat
Copy link
Contributor

I bet the reason is rodata being different due to the file path difference that is in it - https://github.com/0xMiden/compiler/actions/runs/22096960736/job/63856846720?pr=959#step:7:816. We fix it with trim-path cargo option (see example at

trim-paths = ["diagnostics", "object"]
) but in this case you're using CompilerTest::rust_fn_body that calls rustc directly. So you can either find trim-path equivalent in rustc options or switch to CompilerTest::rust_fn_body_with_* that uses cargo.

@greenhat
Copy link
Contributor

The globals are placed after the 'rodata'.

@mooori
Copy link
Contributor Author

mooori commented Feb 18, 2026

Thanks for the hints! It was due to the path difference in rodata indeed.

With rustc's --remap-path-prefix path prefix I haven't been able to modify the path in the embedded panic message. Using rust_fn_body_with_stdlib_sys sets panic="abort", which solves the problem by removing the panic message. No panics are expected for these tests, so we can benefit from more compact wasm.

@mooori mooori marked this pull request as ready for review February 18, 2026 13:49
@mooori mooori requested review from bitwalker and greenhat February 18, 2026 13:49
Copy link
Contributor

@greenhat greenhat left a comment

Choose a reason for hiding this comment

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

Looking good! Nice job!

@mooori mooori merged commit c002d1a into next Feb 20, 2026
14 of 15 checks passed
@mooori mooori deleted the mooori/test-arith-ops branch February 20, 2026 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test integer checked and overflowing arith ops

2 participants