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

Add function to compile prepared db with debug info #5157

Merged
merged 5 commits into from
Mar 19, 2024

Conversation

barabanovro
Copy link
Contributor

@barabanovro barabanovro commented Feb 28, 2024

This PR adds the compile_prepared_db_with_debug_info function to cairo-lang-compiler, allowing the compilation of programs and the retrieval of debug information.

This PR will enable cairovm.codes to display Cairo function names in a callstack.


This change is Reviewable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @barabanovro)


crates/cairo-lang-compiler/src/lib.rs line 104 at r1 (raw file):

        compile_prepared_db_with_debug_info(db, main_crate_ids, compiler_config)?;

    Ok(sierra_program)

.

Suggestion:

    Ok(compile_prepared_db_with_debug_info(db, main_crate_ids, compiler_config)?.sierra_program)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @barabanovro)

a discussion (no related file):
@yuvalsw for 2nd eye.


Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @barabanovro)


crates/cairo-lang-compiler/src/lib.rs line 115 at r2 (raw file):

/// * `Ok(SierraProgramWithDebug)` - The compiled program with debug info.
/// * `Err(anyhow::Error)` - Compilation failed.
pub fn compile_prepared_db_with_debug_info(

"with debug info" sounds like it gets it as an additional input.
I'd just call it compile_prepared_db_ex.

Also, please add in the beginning of the comment something like "same as compile_prepared_db, but returns all the raw debug info".

Copy link
Contributor

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yuvalsw)


crates/cairo-lang-compiler/src/lib.rs line 115 at r2 (raw file):

Previously, yuvalsw wrote…

"with debug info" sounds like it gets it as an additional input.
I'd just call it compile_prepared_db_ex.

Also, please add in the beginning of the comment something like "same as compile_prepared_db, but returns all the raw debug info".

I disagree with the _ex prefix. Sounds very WinAPI-style and it doesn't convey any meaning IMO.

I think what ultimately would be much better, is simply return debug info from the main function, and let downstream users pick what they need. I know this is a breaking change, but we allow ourselves to do them in our APIs, aren't we?

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkaput)


crates/cairo-lang-compiler/src/lib.rs line 115 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

I disagree with the _ex prefix. Sounds very WinAPI-style and it doesn't convey any meaning IMO.

I think what ultimately would be much better, is simply return debug info from the main function, and let downstream users pick what they need. I know this is a breaking change, but we allow ourselves to do them in our APIs, aren't we?

Calling this function with the original name is ok. I wouldn't worry about the breaking change but if there are many calls to the existing function it would be nice for them to have a wrapper/selector that takes only the program.
I am ok with other names, just "with_debug_info_ sounds like it's another input to the compilation rather than an output.

Copy link
Contributor

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yuvalsw)


crates/cairo-lang-compiler/src/lib.rs line 115 at r2 (raw file):

Previously, yuvalsw wrote…

Calling this function with the original name is ok. I wouldn't worry about the breaking change but if there are many calls to the existing function it would be nice for them to have a wrapper/selector that takes only the program.
I am ok with other names, just "with_debug_info_ sounds like it's another input to the compilation rather than an output.

Perhaps having a impl From<SierraProgramWithDebug> for SierraProgram could do the job?

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkaput)


crates/cairo-lang-compiler/src/lib.rs line 115 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

Perhaps having a impl From<SierraProgramWithDebug> for SierraProgram could do the job?

I like it less, but I'll leave you guys to choose the names, I am ok with anything that makes sense.
One suggestion could be calling the original one "compile_prepared_db_program" and this one "compile_prepared_db", if you like it.

@mazurroman
Copy link

Thank you @mkaput and @yuvalsw for your reviews and feedback. Please note @barabanovro is currently off and will get back to this after March 18th.

Copy link
Contributor

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @barabanovro)

Copy link
Contributor Author

@barabanovro barabanovro left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @mkaput, @orizi, and @yuvalsw)


crates/cairo-lang-compiler/src/lib.rs line 115 at r2 (raw file):

Previously, yuvalsw wrote…

I like it less, but I'll leave you guys to choose the names, I am ok with anything that makes sense.
One suggestion could be calling the original one "compile_prepared_db_program" and this one "compile_prepared_db", if you like it.

I have renamed functions to compile_prepared_db_program and compile_prepared_db as @yuvalsw suggested.

Copy link
Contributor

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mkaput)

Copy link
Contributor

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @barabanovro)

@orizi orizi enabled auto-merge March 19, 2024 09:58
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @barabanovro, @mkaput, and @yuvalsw)


crates/cairo-lang-compiler/src/lib.rs line 106 at r4 (raw file):

/// Runs Cairo compiler.
///
/// Similar to `compile_prepared_db_program`, but this function returns all the raw debug information.

fmt

Code quote:

/// Similar to `compile_prepared_db_program`, but this function returns all the raw debug information.

auto-merge was automatically disabled March 19, 2024 10:35

Head branch was pushed to by a user without write access

Copy link
Contributor Author

@barabanovro barabanovro left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @mkaput, @orizi, and @yuvalsw)


crates/cairo-lang-compiler/src/lib.rs line 106 at r4 (raw file):

Previously, orizi wrote…

fmt

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @barabanovro)

@orizi orizi enabled auto-merge March 19, 2024 10:37
@orizi orizi added this pull request to the merge queue Mar 19, 2024
Merged via the queue into starkware-libs:main with commit e6840d8 Mar 19, 2024
43 checks passed
shramee pushed a commit to shramee/cairo that referenced this pull request Sep 17, 2024
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.

5 participants