Extract serializable function debug info

[piotmag769]

Stack:

• Add option to extract functions debug info to configs #8969
• Extract serializable function debug info #8826 ⬅

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

[reviewable-StarkWare]

This change is 

[piotmag769]

@piotmag769 reviewed 3 of 6 files at r1.
Reviewable status: 3 of 6 files reviewed, all discussions resolved (waiting on @Arcticae and @orizi)

---

crates/cairo-lang-sierra-generator/src/debug_info/mod.rs line 23 at r1 (raw file):

}

/// A full path to a Cairo source file.

This is just moved from https://reviewable.io/reviews/starkware-libs/cairo/8826#file-OfUGeuh5gN5ta8Yj3V3

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 99 at r1 (raw file):

        function_file_path: &SourceFileFullPath,
        function_code_span: &SourceCodeSpan,
    ) -> HashMap<(CairoVariableName, SourceCodeSpan), Vec<VarId>> {

This return type may be subject to change depending on what is best for us (it may turn out that sierra_var -> cairo_var mapping is better), but we do not intend to stabilize it until we have debugger MVP

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 259 at r1 (raw file):

}

/// If the ast node is a lookup item, return corresponding ids.

Everything from this point to the end of the file is a slightly modified copy paste of LS logic used to find definition of local vars and params

[Arcticae]

@Arcticae reviewed 1 of 6 files at r1.
Reviewable status: 4 of 6 files reviewed, 12 unresolved discussions (waiting on @orizi and @piotmag769)

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 259 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Everything from this point to the end of the file is a slightly modified copy paste of LS logic used to find definition of local vars and params

Perhaps we should note this reference here in a comment?

---

crates/cairo-lang-sierra-generator/src/debug_info/mod.rs line 59 at r1 (raw file):

) -> Option<(SourceFileFullPath, SourceCodeSpan, bool)> {
    let is_macro =
        matches!(location.file_id(db).long(db), FileLongId::Virtual(_) | FileLongId::External(_));

This one is unused anyway

---

crates/cairo-lang-sierra-generator/src/debug_info/mod.rs line 63 at r1 (raw file):

    let file_full_path = location.file_id.full_path(db);
    let position = location.span.position_in_file(db, location.file_id)?;
    let source_location = SourceCodeSpan {

an into converson could be used here

---

crates/cairo-lang-sierra-generator/src/debug_info/mod.rs line 68 at r1 (raw file):

    };

    Some((SourceFileFullPath(file_full_path), source_location, is_macro))

Isn't that one a case for a nice new struct?

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info/serializable.rs line 24 at r1 (raw file):

}

/// The serializable debug info of a sierra function.

This comment is not needed really

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info/serializable.rs line 26 at r1 (raw file):

/// The serializable debug info of a sierra function.
#[derive(Serialize, Deserialize)]
pub struct SerializableFunctionDebugInfo {

Why name it in such a way?

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 76 at r1 (raw file):

impl<'db> FunctionDebugInfo<'db> {
    fn extract_serializable_debug_info(

I think i would like it more in the other way around - add construction methods to the newly created struct (and make a try_extract one for example). It's preferred to implement Froms rather than Intos usually.

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 92 at r1 (raw file):

    /// Extracts mapping from a cairo variable - its name and definition span - to a vector
    /// with sierra variables which values correspond to values of the cairo variable during

Let's explain a bit what this means (one more sentence will suffice).
I know what this means but the person reading it may not

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 100 at r1 (raw file):

        function_code_span: &SourceCodeSpan,
    ) -> HashMap<(CairoVariableName, SourceCodeSpan), Vec<VarId>> {
        let mut variables_locations: HashMap<_, Vec<_>> = Default::default();

It's confusing to use this name again in different context here (it also has different type)

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 101 at r1 (raw file):

    ) -> HashMap<(CairoVariableName, SourceCodeSpan), Vec<VarId>> {
        let mut variables_locations: HashMap<_, Vec<_>> = Default::default();
        for (sierra_var, code_location) in self.variables_locations.iter() {

[nit]: Wouldn't it be better to do a filter_map here with reduce? A matter of taste but we would avoid continues if we had a smaller, function mapping (sierra_var, code_location) to an Option<VarId>

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 104 at r1 (raw file):

            // Ignore inline locations for now as this function is supposed to be called only
            // on when sierra was compiled with inlining set to `avoid`.
            // TODO: handle them to make sure user function marked #[inline(always)] work.

I would link this TODO to an issue if you want to leave it here

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 195 at r1 (raw file):

    node: SyntaxNode<'db>,
) -> Option<TerminalIdentifier<'db>> {
    let identifier = TerminalIdentifier::cast_token(db, node)

What does the node need to be in order for this function to parse it into identifier properly? Did you happen to find cases where the LocationId and the node which is a variable doesn't fit this casting?

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 226 at r1 (raw file):

    if let Some(assignment_expr) = assignment_expr {
        let lhs = assignment_expr.lhs(db).as_syntax_node();
        let identifier_is_on_lhs = identifier_node.ancestors(db).any(|node| node == lhs);

Shouldn't this check if descendants of the lhs contains the identifier_node?

[piotmag769]

@piotmag769 reviewed 3 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Arcticae and @orizi)

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 76 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

I think i would like it more in the other way around - add construction methods to the newly created struct (and make a try_extract one for example). It's preferred to implement Froms rather than Intos usually.

Agree in principle, but this follows the convention in debug_info module (I created the convention a loooong time ago, but nvm that - check stuff like extract_statements_source_code_locations), I think it's better to be consistent. This code won't be modified much anyway

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 104 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

I would link this TODO to an issue if you want to leave it here

@orizi is it fine to link to an issue outside of compiler repo?

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 195 at r1 (raw file):

What does the node need to be in order for this function to parse it into identifier properly?

It is answered in the doc comment - not sure what else is there to add

and the node which is a variable

Well, it's not that easy. For example LocationId.stable_location may point to:

• 2 in x += 2
• whole x += 2
• x + 2 in y = x + 2
• mut x in let mut x = 5
• mut x : u8 in fn f(mut x: u8)

But I just realised I didn't cover

`a + b + c` in `x = a + b + c;

case, so thanks, I need to change the logic a bit and look for the identifier later on.

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 226 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Shouldn't this check if descendants of the lhs contains the identifier_node?

It is implied here though - if lhs is an ancestor of identifier_node then identifier_node is a descendant of lhs

---

crates/cairo-lang-sierra-generator/src/debug_info/mod.rs line 59 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

This one is unused anyway

It is by coverage mappings

---

crates/cairo-lang-sierra-generator/src/debug_info/mod.rs line 63 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

an into converson could be used here

No reason to implement it just to use it here + I don't want to make more changes in this PR

---

crates/cairo-lang-sierra-generator/src/debug_info/mod.rs line 68 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Isn't that one a case for a nice new struct?

1. It was already like that, don't want to change it, I only moved the code
2. It would just be a temporary struct that doesn't serve much purpose

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info/serializable.rs line 24 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

This comment is not needed really

I though @orizi may want a doc comment

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info/serializable.rs line 26 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Why name it in such a way?

To distinguish from FunctionDebugInfo<'db>

[piotmag769]

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Arcticae and @orizi)

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 101 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

[nit]: Wouldn't it be better to do a filter_map here with reduce? A matter of taste but we would avoid continues if we had a smaller, function mapping (sierra_var, code_location) to an Option<VarId>

It gets ugly really fast when you need 3 args in filter_map

[piotmag769]

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Arcticae and @orizi)

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 100 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

It's confusing to use this name again in different context here (it also has different type)

Done.

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 259 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Perhaps we should note this reference here in a comment?

Done.

[Arcticae]

Reviewable status: 5 of 6 files reviewed, 8 unresolved discussions (waiting on @orizi and @piotmag769)

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 92 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Let's explain a bit what this means (one more sentence will suffice).
I know what this means but the person reading it may not

I meant specifically why many sierra variables map to one cairo variable

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 101 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

It gets ugly really fast when you need 3 args in filter_map

Hmmm i don't really see this as ugli-er since it would be just the chain of methods and rest of code can be in a separate function with named args we'd just pass. But i'm leaving this up to you - it's not crucial here

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 195 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

What does the node need to be in order for this function to parse it into identifier properly?

It is answered in the doc comment - not sure what else is there to add

and the node which is a variable

Well, it's not that easy. For example LocationId.stable_location may point to:

• 2 in x += 2
• whole x += 2
• x + 2 in y = x + 2
• mut x in let mut x = 5
• mut x : u8 in fn f(mut x: u8)

But I just realised I didn't cover

`a + b + c` in `x = a + b + c;

case, so thanks, I need to change the logic a bit and look for the identifier later on.

So the node is a stable location of the cairo variable?
I think this fn would really use an example with in/out like you pasted above, it cleared it up for me

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 226 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

It is implied here though - if lhs is an ancestor of identifier_node then identifier_node is a descendant of lhs

Less intuitive when i read it - is it less expensive? (Just asking here)

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 259 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Done.

Please surround copied code with a "region" if it doesn't cover the whole file (one could think only one function is copied over)

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info/serializable.rs line 26 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

To distinguish from FunctionDebugInfo<'db>

This mapping is different - it's not just a serializable variant of the former. I think it's more of a debugger only input, so we should probably name it after that (i know that the mappings' type may change in the future but the point still stands).

[piotmag769]

@piotmag769 reviewed all commit messages.
Reviewable status: 5 of 6 files reviewed, 8 unresolved discussions (waiting on @Arcticae and @orizi)

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 92 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

I meant specifically why many sierra variables map to one cairo variable

I changed it, no longer relevant

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 195 at r1 (raw file):

So the node is a stable location of the cairo variable

A stable location the sierra var points to (as is described in the doc). It doesn't necessarily point to an identifier of a cairo variable though as I explained

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 226 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Less intuitive when i read it - is it less expensive? (Just asking here)

Ancestors are better performance wise (just one syntax tree branch instead of more)

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 259 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Please surround copied code with a "region" if it doesn't cover the whole file (one could think only one function is copied over)

It is done separately above every function already though

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info/serializable.rs line 26 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

This mapping is different - it's not just a serializable variant of the former. I think it's more of a debugger only input, so we should probably name it after that (i know that the mappings' type may change in the future but the point still stands).

I changed it, now it is basically a serialized version

[piotmag769]

Reviewable status: 4 of 6 files reviewed, 8 unresolved discussions (waiting on @Arcticae and @orizi)

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 101 at r1 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Hmmm i don't really see this as ugli-er since it would be just the chain of methods and rest of code can be in a separate function with named args we'd just pass. But i'm leaving this up to you - it's not crucial here

Done.

[Arcticae]

Reviewable status: 4 of 6 files reviewed, all discussions resolved (waiting on @orizi and @piotmag769)

[orizi]

@orizi reviewed 4 of 6 files at r1, 1 of 1 files at r6.
Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @piotmag769)

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 104 at r1 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

@orizi is it fine to link to an issue outside of compiler repo?

for some SM issues in repo? all good.

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 42 at r6 (raw file):

#[derive(Clone, Debug, Eq, PartialEq)]
pub struct AllFunctionsDebugInfo<'db>(OrderedHashMap<FunctionId, FunctionDebugInfo<'db>>);

doc

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 247 at r6 (raw file):

}

fn find_identifier_in_node<'db>(

doc

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info/serializable.rs line 10 at r6 (raw file):

use crate::debug_info::{SourceCodeSpan, SourceFileFullPath};

pub struct SerializableAllFunctionsDebugInfo(

doc

[orizi]

@orizi reviewed 1 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @piotmag769)

[piotmag769]

@piotmag769 reviewed 1 of 2 files at r3, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @orizi)

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 42 at r6 (raw file):

Previously, orizi wrote…

doc

Done.

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info.rs line 247 at r6 (raw file):

Previously, orizi wrote…

doc

Done.

---

crates/cairo-lang-sierra-generator/src/debug_info/function_debug_info/serializable.rs line 10 at r6 (raw file):

Previously, orizi wrote…

doc

Done.

[orizi]

@orizi reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @piotmag769)