Fix debug line number info for macro expansions. #35238

vadimcn · 2016-08-03T06:36:23Z

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 · 2016-08-03T08:15:33Z

Failed tidy.

eddyb · 2016-08-03T14:00:24Z

r? @michaelwoerister

eddyb · 2016-08-03T14:01:39Z

src/rustllvm/RustWrapper.cpp

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


Missing newline.

eddyb · 2016-08-03T14:06:59Z

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 · 2016-08-03T17:59:36Z

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

michaelwoerister · 2016-08-04T01:25:51Z

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

vadimcn · 2016-08-04T01:53:46Z

rebased

sanxiyn · 2016-08-04T11:53:56Z

Travis says:

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

vadimcn · 2016-08-04T19:22:38Z

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 · 2016-08-05T14:29:44Z

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 · 2016-08-05T18:04:52Z

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 · 2016-08-05T19:06:01Z

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 · 2016-08-08T21:39:46Z

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 · 2016-08-08T22:35:58Z

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 · 2016-08-08T23:38:50Z

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 · 2016-08-09T20:14:06Z

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 · 2016-08-09T20:17:27Z

Here's what a quick investigation turned up:

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.
The is a DISubprogram for the inlined fn.
The DISubprogram for the inlined fn also contains the complete DILexicalBlock structure.
The variables originating from the inlined fn give the DILexicalBlock within the inlined fn as their scope.
The !dbg attachment of an inlined variable's llvm.dbg.declare() also points to the DILexicalBlock within the inlined DISubprogram
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 · 2016-08-12T01:58:53Z

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

r?

bors · 2016-08-15T18:41:01Z

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

michaelwoerister · 2016-08-15T20:11:13Z

I'll take a look shortly.

michaelwoerister · 2016-08-16T13:22:39Z

src/librustc_trans/debuginfo/create_scope_map.rs

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


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

bors · 2016-08-23T18:30:38Z

💔 Test failed - auto-linux-cross-opt

eddyb · 2016-08-23T18:34:41Z

@bors retry

bors · 2016-08-23T18:37:27Z

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

bors · 2016-08-23T18:40:27Z

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

alexcrichton · 2016-08-23T19:36:37Z

@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 · 2016-08-24T02:47:23Z

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

bors · 2016-08-24T04:35:03Z

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

bors · 2016-08-25T01:48:41Z

☔ 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 · 2016-08-25T07:42:30Z

@bors: r=michaelwoerister

bors · 2016-08-25T07:42:31Z

📌 Commit cf64611 has been approved by michaelwoerister

@eddyb

…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

Rollup of 6 pull requests - Successful merges: #35238, #35867, #35885, #35916, #35947, #35955 - Failed merges:

bors · 2016-08-25T11:31:30Z

⌛ Testing commit cf64611 with merge 540cdd6...

@eddyb

…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 · 2016-08-25T11:35:51Z

⛄ The build was interrupted to prioritize another pull request.

Rollup of 6 pull requests - Successful merges: #35238, #35867, #35885, #35916, #35947, #35955 - Failed merges:

rust-highfive assigned eddyb Aug 3, 2016

rust-highfive assigned michaelwoerister and unassigned eddyb Aug 3, 2016

eddyb reviewed Aug 3, 2016
View reviewed changes

vadimcn force-pushed the macro-debug-locs branch from 433c59f to 5fe1a78 Compare August 3, 2016 16:43

vadimcn force-pushed the macro-debug-locs branch from 5fe1a78 to 659f420 Compare August 4, 2016 01:52

vadimcn force-pushed the macro-debug-locs branch from 659f420 to aa1f77d Compare August 12, 2016 01:08

vadimcn force-pushed the macro-debug-locs branch from aa1f77d to 702fbb7 Compare August 12, 2016 04:44

michaelwoerister reviewed Aug 16, 2016
View reviewed changes

bors mentioned this pull request Aug 23, 2016

Rollup of 13 pull requests #35945

Closed

vadimcn force-pushed the macro-debug-locs branch from 6a9db47 to f0164b1 Compare August 25, 2016 07:32

vadimcn force-pushed the macro-debug-locs branch from f0164b1 to cf64611 Compare August 25, 2016 07:41

Manishearth mentioned this pull request Aug 25, 2016

Rollup of 6 pull requests #35979

Merged

bors added a commit that referenced this pull request Aug 25, 2016

Auto merge of #35979 - Manishearth:rollup, r=Manishearth

05fb797

Rollup of 6 pull requests - Successful merges: #35238, #35867, #35885, #35916, #35947, #35955 - Failed merges:

bors added a commit that referenced this pull request Aug 25, 2016

Auto merge of #35979 - Manishearth:rollup, r=Manishearth

17a2be8

Rollup of 6 pull requests - Successful merges: #35238, #35867, #35885, #35916, #35947, #35955 - Failed merges:

bors merged commit cf64611 into rust-lang:master Aug 25, 2016

vadimcn mentioned this pull request Oct 7, 2016

!include'd source has all debug/error line numbers attributed to !include statement #36382

Closed

lucab mentioned this pull request Nov 26, 2016

rust build on ARMv8 fails in test macro-stepping.rs, other debuginfo-gdb tests. #37225

Open

michaelwoerister mentioned this pull request Jan 18, 2017

Code in block passed to macro has line number of macro invocation #39153

Closed

hsivonen mentioned this pull request Aug 18, 2017

RFC: Debuggable macro expansions rust-lang/rfcs#2117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix debug line number info for macro expansions. #35238

Fix debug line number info for macro expansions. #35238

vadimcn commented Aug 3, 2016

sanxiyn commented Aug 3, 2016

eddyb commented Aug 3, 2016

eddyb Aug 3, 2016

eddyb commented Aug 3, 2016

bors commented Aug 3, 2016

michaelwoerister commented Aug 4, 2016

vadimcn commented Aug 4, 2016

sanxiyn commented Aug 4, 2016

vadimcn commented Aug 4, 2016

michaelwoerister commented Aug 5, 2016

vadimcn commented Aug 5, 2016

michaelwoerister commented Aug 5, 2016

vadimcn commented Aug 8, 2016

michaelwoerister commented Aug 8, 2016

michaelwoerister commented Aug 8, 2016

vadimcn commented Aug 9, 2016

michaelwoerister commented Aug 9, 2016

vadimcn commented Aug 12, 2016

bors commented Aug 15, 2016

michaelwoerister commented Aug 15, 2016

michaelwoerister Aug 16, 2016

bors commented Aug 23, 2016

eddyb commented Aug 23, 2016

bors commented Aug 23, 2016

bors commented Aug 23, 2016

alexcrichton commented Aug 23, 2016

bors commented Aug 24, 2016

bors commented Aug 24, 2016

bors commented Aug 25, 2016

vadimcn commented Aug 25, 2016

bors commented Aug 25, 2016

bors commented Aug 25, 2016

bors commented Aug 25, 2016

Fix debug line number info for macro expansions. #35238

Fix debug line number info for macro expansions. #35238

Conversation

vadimcn commented Aug 3, 2016

sanxiyn commented Aug 3, 2016

eddyb commented Aug 3, 2016

eddyb Aug 3, 2016

Choose a reason for hiding this comment

eddyb commented Aug 3, 2016

bors commented Aug 3, 2016

michaelwoerister commented Aug 4, 2016

vadimcn commented Aug 4, 2016

sanxiyn commented Aug 4, 2016

vadimcn commented Aug 4, 2016

michaelwoerister commented Aug 5, 2016

vadimcn commented Aug 5, 2016

michaelwoerister commented Aug 5, 2016

vadimcn commented Aug 8, 2016

michaelwoerister commented Aug 8, 2016

michaelwoerister commented Aug 8, 2016

vadimcn commented Aug 9, 2016

michaelwoerister commented Aug 9, 2016

vadimcn commented Aug 12, 2016

bors commented Aug 15, 2016

michaelwoerister commented Aug 15, 2016

michaelwoerister Aug 16, 2016

Choose a reason for hiding this comment

bors commented Aug 23, 2016

eddyb commented Aug 23, 2016

bors commented Aug 23, 2016

bors commented Aug 23, 2016

alexcrichton commented Aug 23, 2016

bors commented Aug 24, 2016

bors commented Aug 24, 2016

bors commented Aug 25, 2016

vadimcn commented Aug 25, 2016

bors commented Aug 25, 2016

bors commented Aug 25, 2016

bors commented Aug 25, 2016