-
Notifications
You must be signed in to change notification settings - Fork 169
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bitwalker
reviewed
Aug 12, 2024
bitwalker
approved these changes
Aug 12, 2024
There was a problem hiding this 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
plafer
approved these changes
Aug 12, 2024
There was a problem hiding this 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 anAssembler
instead of aSourceMapManager
as a parameter. This is needed because inmiden-base
we build libraries which rely on other libraries (e.g.,miden-lib
depends onmiden-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 themiden-stdlib
crate. When this feature is enabled, the assembler used to assembler the library is instantiated in thedebug
mode. Otherwise, the assembler is instantiated with thedebug
mode turned off.Other changes include:
CompiledLibraryError
.Library
andKernelLibrary
exports to the top level ofmiden-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):The way I fixed it was to add a separate
is_kernel
field to theProcedureContext
struct which is independent of thevisibility
field.Lastly, I've implemented
Assembler::add_modules_from_dir()
method to simplify adding all.masm
files to the assembler from a given directory.