Skip to content
This repository has been archived by the owner on Oct 25, 2021. It is now read-only.

Avoiding Rust Deadlocks via Lifetime Visualization #352

Closed
wants to merge 22 commits into from

Conversation

songlh
Copy link

@songlh songlh commented Oct 15, 2020

Grant Application

This application is (select one):

  • Speculative (use this by default)
  • an RFP response

This application is (select one):

  • Public (fully)
  • Public with private finances

Abstract

We propose to build an IDE tool for visualizing the lifetime scope of a user-selected Rust variable. We believe our tool can help Rust programmers avoid deadlocks at the development stage. After writing a piece of code involving a mutex, a programmer can select the return value of a locking operation or the locking operation itself (when the return is not saved to a variable). Our tool will visualize the lifetime scope of the return value (i.e., the critical section). The programmer can then inspect whether the end of the critical section is expected. In addition, our tool will conduct deadlock detection for the selected critical section and provide detailed debugging information for identified bugs, such as highlighting blocking operations or function calls leading to blocking operations.

Checklist

  • The grants document has been read and understood.
  • The Google Form will be completed accurately. Note that the Google Form requires the pull request URL.
  • Abstract (above) is succinct and complete.
  • The application is being included into the correct directory: either 'targeted' or 'speculative'.
  • The application includes a project description.
  • The application includes all names of team members.
  • The application includes a description of the team's experience.
  • The application includes all necessary links (e.g. GitHub and LinkedIn)
  • The "Development Roadmap" section in the application has a timeline of development ("milestones").
  • The "Development Roadmap" section in the application has an estimate of funds required.
  • The "Development Roadmap" section gives an indication of the team's long term plans.
  • The "Development Roadmap" section includes documentation as a deliverable for at least one milestone.

@burdges
Copy link
Contributor

burdges commented Oct 15, 2020

Rust borrows shall never outlive their lexical scope. You can always shorten a borrow by surrounding in { .. }.

We're presumably talking post-NLL since pre-NLL this tool amounts to syntax highlighting the { .. } surrounding the borrow, no? I'm unsure if NLL is fully stabilized yet, so some borrows live shorter than their scope. If a borrow is confusing then you should simply add a scope { .. } that requires the compiler shorten the borrow.

Any tool like this must be part of RLS so the question is: What happened to rust-lang/rls#387 and rust-lang/rust#42733 ?

Aside from lifetimes, one could discuss durations for allocations but that's an unrelated problem with minimal connection to Rust ifetimes.

@songlh
Copy link
Author

songlh commented Oct 15, 2020

lifetime is much more tricky than lexical scope, such as figure 8 in our PLDI paper.

We have two pieces of evidence to show that similar mistakes are common:

First, in our previous bug study (i.e., the PLDI paper), we found 30 previously fixed double-locking bugs in famous Rust programs, such as Servo and Parity Ethereum. All of them are due to misunderstanding of Rust's lifetime and can be avoided by highlighting lifetime scope or critical section.

Second, we applied the double-lock detection component of our prototype to Substrate, Polkadot, and ink!. We found four previously unknown deadlocks. One is in Substrate. The other three are in the dependent libraries of Substrate or Polkadot. We reported all the detected bugs. All of them were fixed based on our reporting. The information of the detected bugs is listed as follows:

paritytech/parity-db#8

paritytech/substrate#6277

paritytech/parity-common#396

Although I agree that fixing lifetime-related bugs is to tune lexical scope, our tool is about avoiding similar mistakes during development, not educating developers how to fix the problems.

@burdges
Copy link
Contributor

burdges commented Oct 15, 2020

I see, fair enough. 👍 I'm kinda surprised that examples does not require let result = { connect(..) }; actually. Sounds good! :)

I suppose the remaining question is: Would this make sense inside RLS? Or does some CI tool make more sense? Or should it be run only infrequently? I.e. how fast does it run?

@songlh
Copy link
Author

songlh commented Oct 20, 2020

We have a prototype based on analyzing MIR, but I don’t see any difficulty to leverage the RLS platform.

The Rust compiler dumps the MIR file for each individual crate. If all dependent crates are compiled, the Rust compiler does not analyze them again when dumping the MIR for the file/crate under editing. Under this incremental compilation scenario, the Rust compiler generates the MIR file usually within one second. Other analysis routines and visualization of our tool take a negligible time.

We expect our tool to be run when a Rust programmer conducts manual code inspection after implementing a function. Our tool aims to help avoid bugs at a relatively large code granularity (e.g., function, code block). It does not work like code completion, and it does not need to be executed very frequently.

@songlh
Copy link
Author

songlh commented Jan 26, 2021

@burdges happy new year! How do you feel about my last answers? Do you have any further questions?

@burdges
Copy link
Contributor

burdges commented Feb 17, 2021

We should support this probably. I've no opinion on how the tool should interact with rustc and/or users, i.e. RLS, CI phase, etc.

@Lederstrumpf Lederstrumpf self-assigned this Feb 18, 2021
@Lederstrumpf
Copy link
Contributor

Hi @songlh,

Thanks a lot for your application and apologies that this fell between the cracks for some time. Building this specifically as a VS Code plugin seems to miss a lot of the ecosystem benefit this tool could have by being made available generically via the language server protocol. Jeff already mentioned RLS - and since your prototype is based on MIR analysis, this should be compatible with both RLS and rust-analyzer?

I'm thinking what makes sense as a first step is to build this without integration into VS Code, but first just with CLI output, and then ensure compatibility with both RLS & rust-analyzer?

@songlh
Copy link
Author

songlh commented Feb 19, 2021

@Lederstrumpf Thanks a lot for the comments!

I agree with you that we can change the steps of the project and generate CLI output firstly.

@songlh
Copy link
Author

songlh commented Feb 20, 2021

@Lederstrumpf Should I change the descriptions in my proposal to reflect that we will generate CLI output firstly?

@burdges
Copy link
Contributor

burdges commented Mar 16, 2021

An off topic remark, all our developers who write this sort of code work for Parity, but they employ many tools like https://github.com/sile/rustracing_jaeger and such. You might find it useful to chat with some of them sometime, not even specifically about this grant, but just to understand their debugging workflows.

@songlh
Copy link
Author

songlh commented Mar 20, 2021

@Noc2 how was the discussion? If you need more information about our proposal, feel free to let us know.

@Noc2
Copy link
Contributor

Noc2 commented Mar 21, 2021

@songlh The grant committee wants to go ahead with your proposal, but we still need the approval from the foundation council, since the grant is above 30k. This might take a little bit longer. Alternatively you could also reduce the scope of this grant and we could initially start with a grant below 30k. I suggest that we continue the conversation via email. Feel free to send us an email at grants@web3.foundation

@songlh
Copy link
Author

songlh commented Mar 21, 2021

@Noc2 Thanks a lot for the help! I just sent an email to grants@web3.foundation.

@semuelle
Copy link
Member

@songlh Email received, thank you.

@gilescope
Copy link

RLS is being deprecated and rolled into Rust Analyser. Personally I think it would be amazing to see this in vscode. (Once there's something that works for vscode it can be broadened out to other rust analyser platforms - vi, emacs etc. and at that point there will be others to help you!)

@alxs
Copy link
Contributor

alxs commented Jul 19, 2021

@gilescope thanks for the input! The current scope of this grant would bring this to VS Code (see milestone 3) - and the work done up to M2 would make it possible to integrate it into other platforms as well. It's good to see that there's outside interest in it.

@songlh in that case it probably makes sense to modify M2 to integrate into rust-analyzer instead of the RLS? I'm assuming we can also amend the contract before signature without requiring reapproval - this is just a technicality.

@songlh
Copy link
Author

songlh commented Jul 20, 2021

@alxs that works for me.

| ------------- | ------------- | ------------- |
| 0a. | License | Apache 2.0 |
| 0b. | Documentation | We will provide both inline documentation of the code and a basic tutorial that explains how to install and use the changed RLS. |
| 0c. | Testing Guide | We will also use the 10 toy programs designed in the last milestone to test whether the bug detection capability is successfully integrated into RLS. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| 0c. | Testing Guide | We will also use the 10 toy programs designed in the last milestone to test whether the bug detection capability is successfully integrated into RLS. |
| 0c. | Testing Guide | We will also use the 10 toy programs designed in the last milestone to test whether the bug detection capability is successfully integrated into rust-analyzer. |

| Number | Deliverable | Specification |
| ------------- | ------------- | ------------- |
| 0a. | License | Apache 2.0 |
| 0b. | Documentation | We will provide both inline documentation of the code and a basic tutorial that explains how to install and use the changed RLS. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| 0b. | Documentation | We will provide both inline documentation of the code and a basic tutorial that explains how to install and use the changed RLS. |
| 0b. | Documentation | We will provide both inline documentation of the code and a basic tutorial that explains how to install and use the changed rust-analyzer. |

| 0b. | Documentation | We will provide both inline documentation of the code and a basic tutorial that explains how to install and use the changed RLS. |
| 0c. | Testing Guide | We will also use the 10 toy programs designed in the last milestone to test whether the bug detection capability is successfully integrated into RLS. |
| 1. | Extend Language Server Protocol (LSP) | We will extend LSP to contain the key information related to deadlocks (e.g., the start and the end of a critical section, blocking operations in a critical section).|
| 2. | Change RLS to emit MIR | We will change the build module of RLS to emit MIR for our bug detection functionalities. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure whether "build module" makes sense here.

Suggested change
| 2. | Change RLS to emit MIR | We will change the build module of RLS to emit MIR for our bug detection functionalities. |
| 2. | Change rust-analyzer to emit MIR | We will change the build module of rust-analyzer to emit MIR for our bug detection functionalities. |

| 0c. | Testing Guide | We will also use the 10 toy programs designed in the last milestone to test whether the bug detection capability is successfully integrated into RLS. |
| 1. | Extend Language Server Protocol (LSP) | We will extend LSP to contain the key information related to deadlocks (e.g., the start and the end of a critical section, blocking operations in a critical section).|
| 2. | Change RLS to emit MIR | We will change the build module of RLS to emit MIR for our bug detection functionalities. |
| 3. | Conduct bug detection in RLS | We will change the analysis crate of RLS to execute the code for bug detection and change the server module to send the detection results out. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Does rust-analyzer have an analysis crate and a server module, or otherwise what would be the equivalent here?

Suggested change
| 3. | Conduct bug detection in RLS | We will change the analysis crate of RLS to execute the code for bug detection and change the server module to send the detection results out. |
| 3. | Conduct bug detection in rust-analyzer | We will change the analysis crate of rust-analyzer to execute the code for bug detection and change the server module to send the detection results out. |

@alxs
Copy link
Contributor

alxs commented Aug 2, 2021

@songlh just a quick reminder to review the changes.

@songlh
Copy link
Author

songlh commented Aug 3, 2021

@alxs I forwarded the new contract to the university. Let me send them another email and check the current status.

@alxs
Copy link
Contributor

alxs commented Aug 3, 2021

Sounds good, thanks for following up. Just making sure, this is just for review? I think we haven't shared the latest version with you for signature. As soon as you're happy with the changes above, please merge them and we'll send it to you.

@songlh
Copy link
Author

songlh commented Aug 4, 2021

@alxs I just heard from the university. They wish to have a word version of the contract. Is it possible to send me a version? I can copy-paste the version you sent to me in a word document if it is allowed.

@alxs
Copy link
Contributor

alxs commented Aug 4, 2021

@songlh feel free to copy the contents of the Google Doc into a Word document.

@alxs
Copy link
Contributor

alxs commented Aug 19, 2021

@songlh any news here?

@songlh
Copy link
Author

songlh commented Aug 31, 2021

@alxs Sorry for my late response! I was traveling these days. The university asked whether we can use the BSD license. I copy-pasted their original sentence as follows: "The open source license granted under Apache 2.0 (section 4.1.2) has liability implications for others at PSU as this type of license can bring all university background IP into play (not just yours). They mentioned a BSD license would be more suitable." They also told me they would schedule a meeting with me this week to have a discussion. I will post the latest news here.

@alxs
Copy link
Contributor

alxs commented Aug 31, 2021

Hey @songlh, sounds good. I'm curious as to what they're referring to, but BSD also works for us. You can go ahead and update the application.

@burdges
Copy link
Contributor

burdges commented Sep 21, 2021

Related: https://github.com/willcrichton/flowistry

@songlh
Copy link
Author

songlh commented Sep 23, 2021

@burdges that one looks interesting. Let us take a careful look. Thanks a lot for pointing the repo to us!

@alxs
Copy link
Contributor

alxs commented Sep 24, 2021

@songlh any updates on the application though? We were waiting for it to go through before archiving the repository, but I think we would go ahead and close it if we can't move forward within the next 2 weeks. Feel free to just re-open it in the new repo.

@burdges
Copy link
Contributor

burdges commented Sep 27, 2021

Also tangentially related: https://twitter.com/bascule/status/1441485735249334272

I'd not noticed this working group before..

@gilescope
Copy link

You didn't notice it because it wasn't there. The formal methods group has just been rebooted (thanks to efforts from concordium's Dev-X initiative).

There's a few interesting compile time deadlock prevention attempts like https://docs.rs/locktree/0.3.0/locktree/ . I do think though that lifetime visualization has huge benefits in comprehension, debuging and teaching as well as 'just' avoiding deadlocks. There are many benefits to.

@alxs
Copy link
Contributor

alxs commented Sep 30, 2021

@Duongxuantuan2345 @songlh we're waiting for my suggestions above to be reviewed and the contract to be signed before this can be merged. However, as I mentioned above, if we don't get an update from @songlh soon I would close the PR.

@songlh
Copy link
Author

songlh commented Oct 8, 2021

@alxs the university lawyer asked me several questions for clarification this week and last week. It seems that the process moves faster recently.

@alxs
Copy link
Contributor

alxs commented Oct 12, 2021

@songlh please review my suggestions above.

@songlh
Copy link
Author

songlh commented Oct 25, 2021

@alxs sorry for my late response! I heard back from the university lawyer. He told me that he sent a contract with comments to "contracts at web3 do foundation" around two weeks ago. Is there any place you suggest I should forward new contract and the comment?

@songlh
Copy link
Author

songlh commented Oct 25, 2021

@alxs sorry for blocking the process to move to the new repo. I can make a new pull request in the new repo, if that works better.

@alxs
Copy link
Contributor

alxs commented Oct 25, 2021

Yes, please send it to legal@web3.foundation with grants@web3.foundation in cc. And it would be great if you could create a pull request in the new repo, thanks. I will go ahead and close this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants