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 debug line number info for macro expansions. #35238

Merged
merged 1 commit into from
Aug 25, 2016

Conversation

vadimcn
Copy link
Contributor

@vadimcn vadimcn commented Aug 3, 2016

Macro expansions result in code tagged with completely different debug locations than the surrounding expressions. This wrecks havoc on debugger's ability the step over source lines.
This change fixes the problem by tagging expanded code as "inlined" at the macro expansion site, which allows the debugger to sort it out.
Note that only the outermost expansion is currently handled, stepping into a macro will still result in stepping craziness.

r? @eddyb

@sanxiyn
Copy link
Member

sanxiyn commented Aug 3, 2016

Failed tidy.

@eddyb
Copy link
Member

eddyb commented Aug 3, 2016

r? @michaelwoerister

return wrap(cast<MDNode>(unwrap<MetadataAsValue>(value)->getMetadata()));
else
return NULL;
}
Copy link
Member

Choose a reason for hiding this comment

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

Missing newline.

@eddyb
Copy link
Member

eddyb commented Aug 3, 2016

One thing that's interesting here and I didn't consider while looking into this myself, is having both scope and inlinedAt pointing to a scope of the same exact function.
I was worried that each macro invocation might needs its own DISubprogram, but that's not the case.

@bors
Copy link
Contributor

bors commented Aug 3, 2016

☔ The latest upstream changes (presumably #35174) made this pull request unmergeable. Please resolve the merge conflicts.

@michaelwoerister
Copy link
Member

Thanks for the PR, @vadimcn! I'll have a look at it shortly.

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 4, 2016

rebased

@sanxiyn
Copy link
Member

sanxiyn commented Aug 4, 2016

Travis says:

failures:
    [debuginfo-gdb] debuginfo-gdb/lexical-scope-with-macro.rs

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 4, 2016

Hmm, this appears to be legit. Looks like LLVM screws up variable debug info for "inlined" scopes.

@eddyb might be right about needing DISubprogram to correct this, at least for variables local to the macro. Although, if we do that, you'll for sure won't be able to see the vars from outer scopes, so testing variable shadowing will be pointless.
Another way to fix macro stepping, would be to do what C++ does, that is, use expansion site's line number for everything that comes out of the expansion. However, in this case we wouldn't be able to set a breakpoint inside the macro, which renders this test invalid as well.

@michaelwoerister
Copy link
Member

OK, I've looked this over and done some research. In principle this is a great idea. However, I'm a bit worried about doing something that is not really supported by LLVM.

Here's some background: In DWARF, inlined function instances are represented by DW_TAG_inlined_subroutine DIEs, which are similar to DW_TAG_lexical_block DIEs but hold some additional information. LLVM seems to construct these DW_TAG_inlined_subroutine DIEs automatically out of the dbg! source location attachments (there's no way to construct them explicitly in LLVM IR, as far as I can tell). So far so good.

However, each DW_TAG_inlined_subroutine DIE holds a reference to the DIE of the function that has been inlined. Yet, for an "inlined" macro, what we get there is the expanding function, leading to a situation where we more or less tell LLVM that some function X has been inlined into itself. From what I've seen in this PR, LLVM doesn't seem to complain but it sounds like trouble. We might be able to create a "dummy" function DIE for each macro. And I'm not entirely sure what the consequences for variable visibility would be with this approach.

@vadimcn

Another way to fix macro stepping, would be to do what C++ does, that is, use expansion site's line number for everything that comes out of the expansion.

That would also mean that we can't step into code within a macro at all, right? You would always just step over it. That doesn't sound like an acceptable solution either. And at least in MSVC I seem to remember that I was able to step through code within macros.

If we could somehow make it reliably work that macros look like inlined functions to the debugger, that would be great. I would consider it an acceptable trade-off then if one would not be able to see local variables defined outside a macro while stepping through code within it.

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 5, 2016

leading to a situation where we more or less tell LLVM that some function X has been inlined into itself.

I think this isn't necessarily a problem in itself. After all, recursive functions can be inlined.

We might be able to create a "dummy" function DIE for each macro. And I'm not entirely sure what the consequences for variable visibility would be with this approach.

I am pretty sure we wouldn't be able to see vars from outer scopes. But this is also the case in the current state of this PR.
The only sign of trouble I currently see is that when stopped inside the new_scope macro, the debugger doesn't show the inner a either.

That would also mean that we can't step into code within a macro at all, right? You would always just step over it. That doesn't sound like an acceptable solution either. And at least in MSVC I seem to remember that I was able to step through code within macros.

This may be a limitation of DWARF vs PDB. I would argue that this would still be an improvement over the current situation, where you are taken inside a macro whether you want it or not. So, IMO, worth considering.

@michaelwoerister
Copy link
Member

After all, recursive functions can be inlined.

You're right. Maybe it's no problem then that there is no separate subprogram DIE for the macro.

The only sign of trouble I currently see is that when stopped inside the new_scope macro, the debugger doesn't show the inner a either.

Yes, that's a problem that would need to be resolved before this PR can land.

I would argue that this would still be an improvement over the current situation, where you are taken inside a macro whether you want it or not.

I would argue that it's better that macros can be inconvenient than not being able to inspected them at all. I guess people's opinion on this will vary depending on how they are using macros.

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 8, 2016

Update: I have a fix for the inner variables not showing up, however that works only partially (only for frame variables, but not for print <var>). Basically I am running into the problem described in #35487. The worst thing is that this also breaks p <var> for any functions that contain macros, even when not stepping into them (because of "self-inlining").

I also tried emitting dummy functions for macros. This un-breaks print <var> in parent functions and overall looks nicer because you can see macro names in stack trace.
Unfortunately, I don't think we can use this, because it forces creation of an artificial scope for the function, that ends at the end of the macro expansion, however actual Rust visibility scopes may start in a macro and then extend past the end of it.

I would argue that it's better that macros can be inconvenient than not being able to inspected them at all. I guess people's opinion on this will vary depending on how they are using macros.

I disagree. Most of the users don't need to debug macro innards, but they need to step over them quite regularly (try!, println!, format!, fail!, etc). For those who need to step into macros we can provide a -Z... switch that reverts to the old behavior.

@michaelwoerister
Copy link
Member

however actual Rust visibility scopes may start in a macro and then extend past the end of it

That would only be the case when you have a "top level" let statement in the macro, right? That would seem like an acceptable limitation to me. Still much better than what we have now and also better than not generating debuginfo for macro interiors at all.

@michaelwoerister
Copy link
Member

So, it seems pretty clear to me now that we have to emulate, on the LLVM IR level, what the LLVM inlining pass would do when it's inlining a function. What that is exactly, I don't know yet :) But that should not be too hard to find out.

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 9, 2016

So, it seems pretty clear to me now that we have to emulate, on the LLVM IR level, what the LLVM inlining pass would do when it's inlining a function.

I'd say it's more like we are trying to reverse-engineer "what kind of a function would generate this code when inlined?"

To clarify, what I've been talking about on IRC, consider this code:

macro_rules! MACRO {
    () => {
        bbb();
        let ccc = 1; 
        ddd();
    }
}

fn function() {
    aaa();
    MACRO!();
    eee();
}

After expansion:

fn function() {
    aaa();
    bbb();
    let ccc = 1; 
    ddd();
    eee();
}

Right now we create two visibility scopes here: scope A for the whole function and scope B starting at ccc and extending to the end of function.
However, in order to represent MACRO as an inlined function, we'd need three: scope A' for function, scope B' for fn MACRO (bbb through ddd) and scope C' for ccc through ddd. Note that eee would now be in scope A'.

@michaelwoerister
Copy link
Member

Here's what a quick investigation turned up:

  1. There is no actual LLVM IR function emitted for an #[inline(always)] fn. LLVM's debuginfo code seems to be able to do with that.
  2. The is a DISubprogram for the inlined fn.
  3. The DISubprogram for the inlined fn also contains the complete DILexicalBlock structure.
  4. The variables originating from the inlined fn give the DILexicalBlock within the inlined fn as their scope.
  5. The !dbg attachment of an inlined variable's llvm.dbg.declare() also points to the DILexicalBlock within the inlined DISubprogram
  6. The inlinedAt info of the !dbg attachment is a DILocation that has the DILexicalBlock within the inlining fn as scope.

None of this is very surprising actually. To emulate this for macros, the pseudo DISubprograms we need to create for them also would need to contain the DILexicalBlock tree of the macro definition. In principle this shouldn't be a problem, but might need some additional machinery in the compiler.

@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 12, 2016

Per team discussion, implemented the C++ approach, i.e. stamp expanded code with the location of the expansion site.

r?

@bors
Copy link
Contributor

bors commented Aug 15, 2016

☔ The latest upstream changes (presumably #35340) made this pull request unmergeable. Please resolve the merge conflicts.

@michaelwoerister
Copy link
Member

I'll take a look shortly.

scopes[scope] = MirDebugScope {
scope_metadata: fn_metadata,
start_pos: loc.file.start_pos,
end_pos: loc.file.end_pos,
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be more correct to have the start and end pos of the function instead of the file containing it?

@bors
Copy link
Contributor

bors commented Aug 23, 2016

💔 Test failed - auto-linux-cross-opt

@eddyb
Copy link
Member

eddyb commented Aug 23, 2016

@bors retry

@bors
Copy link
Contributor

bors commented Aug 23, 2016

⌛ Testing commit 6a9db47 with merge 143ce0c...

@bors
Copy link
Contributor

bors commented Aug 23, 2016

💔 Test failed - auto-linux-64-cross-freebsd

@alexcrichton
Copy link
Member

@bors: retry

On Tue, Aug 23, 2016 at 11:40 AM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-cross-freebsd
https://buildbot.rust-lang.org/builders/auto-linux-64-cross-freebsd/builds/1377


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#35238 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95MvC9-p5NlSSqeHaUickIa1G09Woks5qiz6egaJpZM4JbWU2
.

@bors
Copy link
Contributor

bors commented Aug 24, 2016

⌛ Testing commit 6a9db47 with merge 47ebb6f...

@bors
Copy link
Contributor

bors commented Aug 24, 2016

💔 Test failed - auto-linux-64-x-android-t

@bors
Copy link
Contributor

bors commented Aug 25, 2016

☔ The latest upstream changes (presumably #35764) made this pull request unmergeable. Please resolve the merge conflicts.

Macro expansions produce code tagged with debug locations that are completely different from the surrounding expressions.  This wrecks havoc on debugger's ability the step over source lines.

In order to have a good line stepping behavior in debugger, we overwrite debug locations of macro expansions with that of the outermost expansion site.
@vadimcn
Copy link
Contributor Author

vadimcn commented Aug 25, 2016

@bors: r=michaelwoerister

@bors
Copy link
Contributor

bors commented Aug 25, 2016

📌 Commit cf64611 has been approved by michaelwoerister

Manishearth added a commit to Manishearth/rust that referenced this pull request Aug 25, 2016
…woerister

Fix debug line number info for macro expansions.

Macro expansions result in code tagged with completely different debug locations than the surrounding expressions.  This wrecks havoc on debugger's ability the step over source lines.
This change fixes the problem by tagging expanded code as "inlined" at the macro expansion site, which allows the debugger to sort it out.
Note that only the outermost expansion is currently handled, stepping into a macro will still result in stepping craziness.

r? @eddyb
bors added a commit that referenced this pull request Aug 25, 2016
Rollup of 6 pull requests

- Successful merges: #35238, #35867, #35885, #35916, #35947, #35955
- Failed merges:
@bors
Copy link
Contributor

bors commented Aug 25, 2016

⌛ Testing commit cf64611 with merge 540cdd6...

Manishearth added a commit to Manishearth/rust that referenced this pull request Aug 25, 2016
…woerister

Fix debug line number info for macro expansions.

Macro expansions result in code tagged with completely different debug locations than the surrounding expressions.  This wrecks havoc on debugger's ability the step over source lines.
This change fixes the problem by tagging expanded code as "inlined" at the macro expansion site, which allows the debugger to sort it out.
Note that only the outermost expansion is currently handled, stepping into a macro will still result in stepping craziness.

r? @eddyb
@bors
Copy link
Contributor

bors commented Aug 25, 2016

⛄ The build was interrupted to prioritize another pull request.

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.

None yet

6 participants