Skip to content

Conversation

@ilyalesokhin-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

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, 2 unresolved discussions


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

                if std::env::var("CAIRO_DEBUG_SIERRA_GEN").is_ok() {
                    // If we are debugging sierra generation, we wan't to finish the compilation
                    // rather than panic.

the ap-overflow case isn't an issue for deciding on inlining?

Code quote:

                    // If we are debugging sierra generation, we wan't to finish the compilation
                    // rather than panic.

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

                if std::env::var("CAIRO_DEBUG_SIERRA_GEN").is_ok() {
                    // If we are debugging sierra generation, we wan't to finish the compilation
                    // rather than panic.

Suggestion:

                    // If we are debugging sierra generation, we want to finish the compilation
                    // rather than panic.

@ilyalesokhin-starkware
Copy link
Contributor Author

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

Previously, orizi wrote…

the ap-overflow case isn't an issue for deciding on inlining?

I think that if it happens here, it will happen in the final program. I just that that it was common enough that we should provide users the ability to debug it and get the code location from cairo-execute

Copy link
Contributor Author

@ilyalesokhin-starkware ilyalesokhin-starkware 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, 2 unresolved discussions (waiting on @orizi)


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

                if std::env::var("CAIRO_DEBUG_SIERRA_GEN").is_ok() {
                    // If we are debugging sierra generation, we wan't to finish the compilation
                    // rather than panic.

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.

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)


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

Previously, ilyalesokhin-starkware wrote…

I think that if it happens here, it will happen in the final program. I just that that it was common enough that we should provide users the ability to debug it and get the code location from cairo-execute

i'm actually not sure that is true for size estimation of const-folding:

if const_evaluated_as_true() {
   short()
} else {
   long()
}

@ilyalesokhin-starkware
Copy link
Contributor Author

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

Previously, orizi wrote…

i'm actually not sure that is true for size estimation of const-folding:

if const_evaluated_as_true() {
   short()
} else {
   long()
}

I don't follow.

@ilyalesokhin-starkware
Copy link
Contributor Author

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

Previously, ilyalesokhin-starkware wrote…

I don't follow.

You mean that the compilation of long would fail, but the compiled program will only include short?

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 @ilyalesokhin-starkware)


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

Previously, ilyalesokhin-starkware wrote…

You mean that the compilation of long would fail, but the compiled program will only include short?

yes

@ilyalesokhin-starkware
Copy link
Contributor Author

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

Previously, orizi wrote…

yes

I can restore it, but I guess this is true for any misscompilation that doesn't get into the final code.

Do we want people to report such cases or not?

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 @ilyalesokhin-starkware)


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

Previously, ilyalesokhin-starkware wrote…

I can restore it, but I guess this is true for any misscompilation that doesn't get into the final code.

Do we want people to report such cases or not?

ap-change specifically does sound like a special case for me.

@ilyalesokhin-starkware
Copy link
Contributor Author

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

Previously, orizi wrote…

ap-change specifically does sound like a special case for me.

Why?

is the case of huge code that becomes small because of const-folding an interesting one?

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 @ilyalesokhin-starkware)


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

Previously, ilyalesokhin-starkware wrote…

Why?

is the case of huge code that becomes small because of const-folding an interesting one?

i don't know - but it is a case that may happen now, and would start crashing after this change.

Copy link
Contributor Author

@ilyalesokhin-starkware ilyalesokhin-starkware 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 @orizi)


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

Previously, orizi wrote…

i don't know - but it is a case that may happen now, and would start crashing after this change.

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.

:lgtm:

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

@ilyalesokhin-starkware ilyalesokhin-starkware added this pull request to the merge queue Aug 3, 2025
Merged via the queue into main with commit 54ab922 Aug 3, 2025
49 checks passed
@orizi orizi deleted the ilya/debug_gen branch August 4, 2025 11:48
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.

4 participants