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

fix: Generate debug metadata for methods #1384

Merged
merged 4 commits into from
Jan 10, 2025
Merged

fix: Generate debug metadata for methods #1384

merged 4 commits into from
Jan 10, 2025

Conversation

volsa
Copy link
Member

@volsa volsa commented Jan 9, 2025

...specifically in the context of function variables. Given

FUNCTION_BLOCK foo
    // ...
    METHOD bar
        // ...
    END_METHOD
END_FUNCTION_BLOCK

the compiler will generate two types for foo and bar and furthermore handle method calls by passing both these generated types to some function. That is we get the following

%foo = type {}
%foo.bar = type {}

// ...

define void @foo.bar(%foo* %0, %foo.bar* %1) !dbg !<...> {
    call void @llvm.dbg.declare(metadata %foo* %0, metadata !<...>, metadata !DIExpression()), !dbg !<...>
    call void @llvm.dbg.declare(metadata %foo.bar* %1, metadata !<...>, metadata !DIExpression()), !dbg !<...>
    //                                                          ^^^^^^
    //                                                          was previously ignored
}

Appparently when dealing with %foo and %foo.bar in the @foo.bar function context, we intentionally didn't create any debug information for foo.bar, resulting in the metadata !<...> field pointing to an empty member (e.g. !<num> = {}). As a result LLVM would segfault when dealing with this and therefore any rusty <file> --debug call.

This commit fixes this issue, such that the metadata field points to a correct DILocalVariable definition for %foo.bar.

volsa and others added 3 commits January 9, 2025 13:13
...specifically in the context of function variables. Given
```
FUNCTION_BLOCK foo
    // ...
    METHOD bar
        // ...
    END_METHOD
END_FUNCTION_BLOCK
```
the compiler will generate two types for `foo` and `bar` and furthermore handle method calls by passing both
these generated types to some function. That is we get the following
```
%foo = type {}
%foo.bar = type {}

// ...

define void @foo.bar(%foo* %0, %foo.bar* %1) !dbg !<...> {
    call void @llvm.dbg.declare(metadata %foo* %0, metadata !<...>, metadata !DIExpression()), !dbg !<...>
    call void @llvm.dbg.declare(metadata %foo.bar* %1, metadata !<...>, metadata !DIExpression()), !dbg !<...>
    //                                                          ^^^^^^
    //                                                          was previously ignored
}
```

Appparently when dealing with `%foo` and `%foo.bar` in the `@foo.bar` function context, we intentionally
didn't create any debug information for `foo.bar`, resulting in the `metadata !<...>` field pointing to an
empty member (e.g. `!<num> = {}`). As a result LLVM would segfault when dealing with this and therefore any
`rusty <file> --debug` call.

This commit fixes this issue, such that the metadata field points to a correct `DILocalVariable` definition
for `%foo.bar`.
@volsa volsa requested review from ghaith and mhasel January 10, 2025 08:23
@volsa volsa changed the title fix(debug): Generate debug metadata for methods fix: Generate debug metadata for methods Jan 10, 2025
END_METHOD
END_FUNCTION_BLOCK
",
4,
Copy link
Member

Choose a reason for hiding this comment

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

is there any particular reason to override the debug version here?

Copy link
Member Author

@volsa volsa Jan 10, 2025

Choose a reason for hiding this comment

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

None, I just copy-pasted the previous test and modified it. Good catch though, it probably should be a codegen call

assert!(codegen
.contains(r#"!17 = !DILocalVariable(name: "fb.foo", scope: !15, file: !2, line: 3, type: !18)"#));

// ...for reference, it would previously point to `!4` which is empty (`!4 = {}`). `!4` shouldn't exist in
Copy link
Member

Choose a reason for hiding this comment

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

I think !4 is not wrong here - it just points out that the struct fb does not have any members and therefore no local nodes need to be retained.
For comparison, this is an example by LLVM. Here, the function foo does not have any members, !DISubprogram's retainedNodes metadata points to !2 which is !2 = !{}.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you're right. I was under the impression that the !4 wasn't being used in the IR at all but that's not true.

Copy link
Member

@mhasel mhasel left a comment

Choose a reason for hiding this comment

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

nice!

@volsa volsa merged commit aecd676 into master Jan 10, 2025
19 checks passed
@volsa volsa deleted the volsa/dbgdecl branch January 10, 2025 12:32
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.

2 participants