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.

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

Use assembler instead of source manager when building libraries #1445

Merged
merged 8 commits into from
Aug 13, 2024

Conversation

bobbinth
Copy link
Contributor

@bobbinth bobbinth commented Aug 11, 2024

This PR includes a number of small improvements to library-related components which I intend to release as miden-vm v0.10.3.

The main change is that Library::from_dir() function now takes an Assembler instead of a SourceMapManager as a parameter. This is needed because in miden-base we build libraries which rely on other libraries (e.g., miden-lib depends on miden-stdlib) and kernels (e.g., miden-lib needs to be compiled against the transaction kernel).

Another noteworthy change is that I've added with-debug-info feature to the miden-stdlib crate. When this feature is enabled, the assembler used to assembler the library is instantiated in the debug mode. Otherwise, the assembler is instantiated with the debug mode turned off.

Other changes include:

  • Removed the now-unused CompiledLibraryError.
  • Moved Library and KernelLibrary exports to the top level of miden-assembler.

Another notable change implemented in this PR is ability to build kernels from multiple modules (this is also needed for miden-base). The implemented solution is a temporary workaround until #1436 is properly addressed.


I also fixed a bug which prevented using kernel-specific instructions (e.g., caller) in internal procedures of a kernel module. For example, before the fix, the following would result in an error (assuming this code is for a kernel module):

proc.auth
    padw caller
end

export.foo
    push.1 push.2 add mul
    exec.auth
end

The way I fixed it was to add a separate is_kernel field to the ProcedureContext struct which is independent of the visibility field.


Lastly, I've implemented Assembler::add_modules_from_dir() method to simplify adding all .masm files to the assembler from a given directory.

Copy link
Contributor

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

Looks good, nothing catches my eye here, so good to go I think

Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

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

LGTM - only left a few nits and questions

stdlib/build.rs Outdated Show resolved Hide resolved
miden/src/cli/bundle.rs Show resolved Hide resolved
miden/src/cli/bundle.rs Outdated Show resolved Hide resolved
assembly/src/parser/mod.rs Outdated Show resolved Hide resolved
miden/src/cli/bundle.rs Show resolved Hide resolved
@bobbinth bobbinth merged commit c80dac7 into main Aug 13, 2024
9 checks passed
@bobbinth bobbinth deleted the bobbin-lib-fixes branch August 13, 2024 07:01
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