Skip to content

Generation of kernel procedure hashes in build.rs#887

Merged
bobbinth merged 10 commits intonextfrom
polydez-gen-kernel-procs
Sep 19, 2024
Merged

Generation of kernel procedure hashes in build.rs#887
bobbinth merged 10 commits intonextfrom
polydez-gen-kernel-procs

Conversation

@polydez
Copy link
Copy Markdown
Contributor

@polydez polydez commented Sep 18, 2024

I spent plenty of time understanding why changing constant in assembler leads to crashes of multiple tests. This is because we need to recalculate and store hashes of all changed functions in procedures.rs. Offsets of procedure hashes also stored in kernel_proc_offsets.masm file and there was no automated procedure to update them. This PR adds automatic generation of procedure hashes and offset files in build.rs.

@polydez polydez requested review from Fumuran and bobbinth September 18, 2024 11:55
@polydez polydez marked this pull request as ready for review September 18, 2024 11:55
@polydez polydez marked this pull request as draft September 18, 2024 11:59
@polydez polydez marked this pull request as ready for review September 18, 2024 12:52
Copy link
Copy Markdown
Contributor

@Fumuran Fumuran left a comment

Choose a reason for hiding this comment

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

Looks awesome!
For some reason I thought that we can get the kernel procedures information only during the compilation, completely forgetting that this can be done during the pre-processing. Well, I'm glad that we can automate this process.

Now I'm wondering how can we adapt this script in case of several kernel versions: at that point of time we don't know yet the kernel version which user will choose, which causes some difficulties.

Copy link
Copy Markdown
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Great job! Thank you! I left a few comments inline.

@polydez polydez requested a review from bobbinth September 19, 2024 04:44
@polydez polydez changed the title Generation of kernel procedure hashes and offsets in build.rs Generation of kernel procedure hashes in build.rs Sep 19, 2024
Copy link
Copy Markdown
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good! Not a full review yet, but I wanted to leave a couple of quick comments inline.

@polydez polydez force-pushed the polydez-gen-kernel-procs branch from 77768d6 to 119f0b0 Compare September 19, 2024 08:47
@polydez polydez requested a review from bobbinth September 19, 2024 08:48
Copy link
Copy Markdown
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

All looks good! Thank you!

@bobbinth bobbinth merged commit 2ea3185 into next Sep 19, 2024
@bobbinth bobbinth deleted the polydez-gen-kernel-procs branch September 19, 2024 20:48
spiral-ladder added a commit to spiral-ladder/miden-base that referenced this pull request Nov 28, 2024
0xMiden#887 added a build
script that builds `kernel_v0.rs`, but the script runs on changes to the
file itself. I suspect this was the reason for test times doubling
after that PR as pointed out in 0xMiden#903.

This PR simply removes this rerun check. I doubt this check is the correct
behaviour either way, since if it reruns based on changes to the
`kernel_v0.rs`, it means it reruns every time the file is generated - which means
the build script is triggered between each invocation of `cargo nextest`
as well.
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.

3 participants