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.

Sign up for GitHub

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

Jump to bottom

Fix macro call site spans #33749

Merged
merged 3 commits into from
Jun 14, 2016
Merged

Fix macro call site spans #33749

merged 3 commits into from
Jun 14, 2016

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented May 20, 2016 •
edited
Loading

Fix macro call site spans.
r? @nrc

@durka durka mentioned this pull request May 21, 2016
Closed
@nrc
Copy link
Member

nrc commented May 23, 2016

LGTM

@DanielJCampbell does this break any of your macro tracing stuff?

@DanielJCampbell
Copy link
Contributor

DanielJCampbell commented May 23, 2016 •
edited
Loading

Not that I can see. No failed tests means it passed the tests I had for my stuff, which is a good sign, and I didn't use the original_span method for anything.
Overall better spans is always good, even if I need to go back and tidy my stuff up if I find any issues later.

@nrc
Copy link
Member

nrc commented May 24, 2016

@bors: r+

@bors
Copy link
Contributor

bors commented May 24, 2016

📌 Commit 32b9494 has been approved by nrc

@bors
Copy link
Contributor

bors commented May 24, 2016

⌛ Testing commit 32b9494 with merge f3449b6...

@bors
Copy link
Contributor

bors commented May 24, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented May 26, 2016

⌛ Testing commit 32b9494 with merge c5bbba8...

@bors
Copy link
Contributor

bors commented May 26, 2016

💔 Test failed - auto-linux-64-opt-mir

@bors
Copy link
Contributor

bors commented May 26, 2016

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

@jseyfried
Copy link
Contributor Author

jseyfried commented May 27, 2016 •
edited
Loading

Rebased. @bors r=nrc

@bors
Copy link
Contributor

bors commented May 27, 2016

📌 Commit e77a219 has been approved by nrc

@bors
Copy link
Contributor

bors commented May 27, 2016

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented May 27, 2016

📌 Commit e77a219 has been approved by nrc

@bors
Copy link
Contributor

bors commented May 27, 2016

⌛ Testing commit e77a219 with merge 5b1bd32...

bors added a commit that referenced this pull request May 27, 2016
@bors
Auto merge of #33749 - jseyfried:fix_call_site_span, r=nrc
5b1bd32 
Fix macro call site spans

Fix macro call site spans.
r? @nrc
@bors
Copy link
Contributor

bors commented May 27, 2016

💔 Test failed - auto-linux-64-opt-mir

@jseyfried
Copy link
Contributor Author

@bors retry

@bors
Copy link
Contributor

bors commented May 27, 2016

⌛ Testing commit e77a219 with merge e8ab466...

@bors
Copy link
Contributor

bors commented May 27, 2016

💔 Test failed - auto-linux-64-opt-mir

@alexcrichton
Copy link
Member

(that error looks like it may be legit)

@jseyfried
Copy link
Contributor Author

jseyfried commented May 28, 2016 •
edited
Loading

I'm having trouble reproducing the error, but I think the above commit will fix it -- r? @nrc

@nrc
Copy link
Member

nrc commented May 30, 2016

r? @michaelwoerister for the last commit (r=me for the rest). If the #break comments are where the debugger should break, that looks wrong, but I have no idea what is going on here.

@jseyfried jseyfried mentioned this pull request Jun 10, 2016
Closed
@jseyfried
Copy link
Contributor Author

@michaelwoerister
The incorrect call site spans that this PR fixes cause very confusing diagnostics.

Since debuginfo for macros is already known to be buggy, perhaps we could just land this with the workaround in the last commit?

Another option would be to always use call site spans for debuginfo as described in this comment. While I believe this would fix the bugs I linked and allow this PR to land without the workaround, it would make it impossible to break on a line in a macro definition (in particular, we would have to remove this test).

@michaelwoerister
Copy link
Member

Oh, I forgot about this PR. I'll take a look at it tomorrow.

@michaelwoerister
Copy link
Member

OK, so I won't get into debuginfo vs macros here -- I'd need to put a lot more thought into that and assess the current situation. From what I can see here, things don't get considerably worse, right?

As far as the test case goes: There shouldn't even be a macro invocation here -- in fact, it had been a regular function call before someone changed it to account for the called function being moved, as far as I can tell. We just need something that allows us to reliably set a breakpoint at. I'd suggest just following the pattern of many other debuginfo test cases: Add an empty function fn zzz() { () } and call it wherever a breakpoint is needed. That should allow the #break directive to work reliably.

@jseyfried
Copy link
Contributor Author

From what I can see here, things don't get considerably worse, right?

Right. I believe things can only get worse when there is a macro-expanded procedural macro invocation (in this case, format!("")). If format!("") were invoked directly or if format were somehow defined with macro_rules!, the test would pass after this PR or would already be broken (respectively).

I'd suggest just following the pattern of many other debuginfo test cases: Add an empty function fn zzz() { () } and call it wherever a breakpoint is needed.

Makes sense, done.

@michaelwoerister
Copy link
Member

@bors r=nrc

Excellent :)

@bors
Copy link
Contributor

bors commented Jun 14, 2016

📌 Commit 2477341 has been approved by nrc

@bors
Copy link
Contributor

bors commented Jun 14, 2016

⌛ Testing commit 2477341 with merge bf84f4e...

bors added a commit that referenced this pull request Jun 14, 2016
@bors
Auto merge of #33749 - jseyfried:fix_call_site_span, r=nrc
bf84f4e 
Fix macro call site spans

Fix macro call site spans.
r? @nrc
@bors
Copy link
Contributor

bors commented Jun 14, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

@bors bors merged commit 2477341 into rust-lang:master Jun 14, 2016
@bors bors mentioned this pull request Jun 14, 2016
Merged
@jseyfried jseyfried mentioned this pull request Jun 24, 2016
Closed
This was referenced Jul 14, 2016
Merged
Closed
bors added a commit that referenced this pull request Jul 16, 2016
@bors
Auto merge of #34816 - jseyfried:fix_include_path, r=nrc
eb196dc 
Fix `include!()`s inside `asm!()` invocations

Fixes #34812, a regression caused by #33749 that was not fixed in #34450.
r? @nrc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@michaelwoerister michaelwoerister

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jseyfried @nrc @DanielJCampbell @bors @alexcrichton @michaelwoerister