Skip to content

make asm diagnostic instruction optional #55193

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

make asm diagnostic instruction optional #55193

wants to merge 1 commit into from

Conversation

euclio
Copy link
Contributor

@euclio euclio commented Oct 19, 2018 •
edited
Loading

DiagnosticInfoInlineAsm::getInstruction may return a null pointer, so
the instruction shouldn't be blindly unwrapped.

Fixes #23458.
Fixes #55216.

@euclio
make asm diagnostic instruction optional
8e50140 
`DiagnosticInfoInlineAsm::getInstruction` may return a null pointer, so
the instruction shouldn't be blindly unwrapped.

Fixes rust-lang#23458.
@rust-highfive
Copy link
Contributor

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2018
@nagisa
Copy link
Member

nagisa commented Oct 20, 2018

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 20, 2018

📌 Commit 8e50140 has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 20, 2018
@Manishearth Manishearth mentioned this pull request Oct 20, 2018
Merged
@Manishearth
Copy link
Member

https://ci.appveyor.com/project/rust-lang/rust/builds/19663782/job/s4v19q20t4uttnsr

ui\issues\issue-23458.rs fails on windows

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 20, 2018
bors added a commit that referenced this pull request Oct 20, 2018
@bors
Auto merge of #55230 - Manishearth:rollup, r=Manishearth
d541876 
Rollup of 5 pull requests

Successful merges:

 - #55156 (Fixed: Multiple errors on single typo in match pattern)
 - #55189 (update books for the next release)
 - #55193 (make asm diagnostic instruction optional)
 - #55203 (Write an initial version of the `program_clauses` callback)
 - #55213 (ignore target folders)

Failed merges:

r? @ghost
@nikomatsakis
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Oct 22, 2018

📌 Commit 8e50140 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 22, 2018
@nikomatsakis
Copy link
Contributor

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 23, 2018
@nikomatsakis
Copy link
Contributor

I see @Manishearth's comment now...

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 23, 2018 •
edited
Loading

Any idea what's up with that failure @euclio ?

@euclio
Copy link
Contributor Author

euclio commented Oct 23, 2018

Not yet, I've been having trouble building Rust on my Windows machine. Hoping to get some more info this week.

@TimNN TimNN added A-allocators Area: Custom and system allocators and removed A-allocators Area: Custom and system allocators labels Oct 30, 2018
@euclio
Copy link
Contributor Author

euclio commented Nov 6, 2018

Ok, looks like it's hitting the same assertion as the linked issue.

capture10

I'm going to close this for now.

@euclio euclio closed this Nov 6, 2018
@nikic
Copy link
Contributor

nikic commented Nov 6, 2018

Would it be possible for you to produce a backtrace for the assertion?

Assembly printing goes through different code-paths for GCC-style and MS-style assembly, so this might be a platform difference. However, as LLVM assertions are default disabled nowadays, it might just be that the assertion occurs on both Linux and Windows before the diagnostic is emitted.

@nikic nikic mentioned this pull request Nov 26, 2018
Closed
@euclio euclio deleted the asm-ice branch March 13, 2019 17:56
@euclio euclio restored the asm-ice branch March 13, 2019 17:56
@euclio euclio mentioned this pull request Mar 16, 2019
Merged
bors added a commit that referenced this pull request Mar 25, 2019
@bors
Auto merge of #59242 - euclio:asm-ice, r=nagisa
3f36ac4 
make asm diagnostic instruction optional

`DiagnosticInfoInlineAsm::getInstruction` may return a null pointer, so
the instruction shouldn't be blindly unwrapped.

Reopening from #55193. I was unable to trigger the assertion on Windows after rebasing.

Fixes #23458.
Fixes #55216.
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@nikomatsakis nikomatsakis nikomatsakis approved these changes

Assignees

@nikomatsakis nikomatsakis

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@euclio @rust-highfive @nagisa @bors @Manishearth @nikomatsakis @nikic @TimNN