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

Documentation: Limitations #714

Merged
merged 10 commits into from
Jan 4, 2022
Merged

Conversation

adpaco-aws
Copy link
Contributor

@adpaco-aws adpaco-aws commented Dec 22, 2021

Description of changes:

No changes to RMC. Adds a new section "Limitations" to documentation in order to summarize the current status in regards to Rust feature support using a table.

Preview here

Call-outs:

Open as draft so I can get some feedback.

Testing:

  • How is this change tested? Running remote server for documentation.

  • Is this a refactor change? No.

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. I just have a few minor comments.

Can you please add that we currently do not support stack unwinding?

rmc-docs/src/limitations.md Outdated Show resolved Hide resolved
rmc-docs/src/limitations.md Outdated Show resolved Hide resolved
rmc-docs/src/limitations.md Outdated Show resolved Hide resolved
### Not inlined standard library functions

At present, RMC is able to link in functions from the standard library but the
generated code will not contain them unless they are inlined. This results in
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add that we support to generic functions and macros at this point. They get monomorphized / expanded during the compilation. BTW, I don't think we are running the rustc pass to inline functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why you want to add it here. I think the table already includes this information and I'm not sure what it has to do with the standard library.

Removed "unless they are inlined" from this block.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I wasn't clear. I was talking about the "unless they are inlined". The correct would be "unless they are generic, intrinsics, inlined or macros." :)

verification failures if the code under verification, for example, includes a
`println!` statement.

We have done some experiments to embed the standard library into the generated
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we run these experiments before or after enabling the code slicing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you refer to --drop-unused-functions, I tried both when doing these experiments. It reduced the generated code considerably, but still kept many other functions around which made this approach unfeasible.

Copy link
Contributor

Choose a reason for hiding this comment

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

did your experiments include tests with println?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in fact they just included println.

rmc-docs/src/limitations.md Outdated Show resolved Hide resolved
@adpaco-aws adpaco-aws marked this pull request as ready for review December 30, 2021 21:55
@adpaco-aws adpaco-aws requested a review from a team as a code owner December 30, 2021 21:55
@adpaco-aws
Copy link
Contributor Author

Thanks @celinval ! Updated the PR with some of your suggestions, let me know if you have other comments 😄

@adpaco-aws
Copy link
Contributor Author

Forgot to mention stack unwinding - Added a new section on panic strategies just now.

rmc-docs/src/limitations.md Outdated Show resolved Hide resolved
statement when compiling any unsupported feature. This will cause proofs to fail
if the statement is reachable during the verification stage. However, the
analysis will not affected if the statement is not reachable from the code under
verification, so users can still verify components of their code not using
Copy link
Contributor

Choose a reason for hiding this comment

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

not using that do not use

(I think your phrasing is technically fine, it is just potentially ambiguous what 'not using' refers to (the code, not the verification) until you read after it in the sentence.)

(Hopefully not too nitpicky here, but it's public facing documentation, so I'm looking for these things)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, feedback regarding these subtleties is welcome too! It's changed now.


While RMC is capable of generating code for SIMD instructions. Unfortunately,
CBMC does not provide support for some operations like vector comparison (e.g.,
`simd_eq`) or shuffling (e.g., `simd_shuffle8`).
Copy link
Contributor

Choose a reason for hiding this comment

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

wait, I thought I added shuffle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I thought this was not supported by CBMC but after reviewing related issues I see that you implemented shuffle operations in RMC. So I removed shuffling from here.

issue](https://github.com/model-checking/rmc/issues/581), but we have some ideas
for future work in this direction.

### Advanced features
Copy link
Contributor

Choose a reason for hiding this comment

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

I worry this section might be too vague. It might be a good idea to back-reference what we say in the table? (E.g. there's a lot of links from the table to here that seem to be about projections, and I think our goal is to have to solid by 1.0.)

I think over-technical would be better than vague. If this section answered for us "what would it take for us to remove this section from this doc because we know we support it now" I think that's be better than being vague.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure how to make this more technical, so I highlighted some outstanding issues here.

Please let me know how it looks and if it does not look good to you we can work together on this section.

Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

It looks good to me. Please make sure you address Ted's feedback before pushing. :)

Copy link
Contributor

@zhassan-aws zhassan-aws left a comment

Choose a reason for hiding this comment

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

First batch of comments.

As with all software, bugs may be found anywhere regardless of the level of support. In such cases, we
would greatly appreciate that you [filed a bug report](https://github.com/model-checking/rmc/issues/new?assignees=&labels=bug&template=bug_report.md).

Reference | Feature | Support | Observations |
Copy link
Contributor

Choose a reason for hiding this comment

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

Would "Remarks" or "Notes" be more suitable than "Observations"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with "Notes".


### Code generation for unsupported features

RMC aims to be a industrial verification tool. Most industrial crates may
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
RMC aims to be a industrial verification tool. Most industrial crates may
RMC aims to be an industrial verification tool. Most industrial crates may

Not sure I like the word "industrial", but can't think of an alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, let me know if you think of another word (I don't like it that much neither).

Copy link
Contributor

Choose a reason for hiding this comment

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

One possibility (I don't feel strongly about it either): RMC is intended to be used on real-world Rust code. Most real-world crates ...


RMC aims to be a industrial verification tool. Most industrial crates may
include unsupported features in parts of their code that do not need to be
verified. In general, this should not prevent users from verifying their code.
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
verified. In general, this should not prevent users from verifying their code.
verified. In general, this should not prevent users from using RMC to verify their code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

include unsupported features in parts of their code that do not need to be
verified. In general, this should not prevent users from verifying their code.

Because of that, the general rule is that RMC generates an `assert(false)`
Copy link
Contributor

@zhassan-aws zhassan-aws Jan 3, 2022

Choose a reason for hiding this comment

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

Technically, it generates assert(false) followed by assume(false). I think this piece of detail is worth mentioning since it may cause some asserts to become unreachable, and hence vacuously pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, added both and included a short sentence to explain what they do. Let me know how it looks to you.

verified. In general, this should not prevent users from verifying their code.

Because of that, the general rule is that RMC generates an `assert(false)`
statement when compiling any unsupported feature. This will cause proofs to fail
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
statement when compiling any unsupported feature. This will cause proofs to fail
statement when compiling any unsupported feature. This will cause verification to fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

verification, so users can still verify components of their code not using
unsupported features.

### Assembly
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the official term is "Inline Assembly".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two options in Rust for adding assembly code: Inline and global. Note that below we link an issue for each of them, so I thought "Assembly" was a good name for this section.


### Concurrency

Concurrent features are out of scope for RMC. In general, the verification of
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add "currently"?

Suggested change
Concurrent features are out of scope for RMC. In general, the verification of
Concurrent features are currently out of scope for RMC. In general, the verification of

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

concurrent programs continues to be an open research problem where most tools
that analyze concurrent code lack support for other features. Because of this,
RMC emits a warning whenever it encounters concurrent code and compiles as if it
was sequential code.
Copy link
Contributor

@zhassan-aws zhassan-aws Jan 3, 2022

Choose a reason for hiding this comment

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

Does RMC pick some order for execution of the concurrent code? If so, we should point out that this may lead to RMC missing bugs that may occur with a different execution order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK it assumes all code to be sequential. I think there are some CBMC options for concurrent code verification but we do not make use of them at the moment.

### Standard library functions

At present, RMC is able to link in functions from the standard library but the
generated code will not contain them. This results in verification failures if
Copy link
Contributor

Choose a reason for hiding this comment

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

but the generated code will not contain them

This sentence a bit vague. Can we clarify it? Does RMC generate these functions with an empty body?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a sentence to mention these are treated in a similar way to unsupported features. Let me know what you think!

rmc-docs/src/limitations.md Outdated Show resolved Hide resolved
### Standard library functions

At present, RMC is able to link in functions from the standard library but the
generated code will not contain them unless they generic, intrinsics, inlined or
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
generated code will not contain them unless they generic, intrinsics, inlined or
generated code will not contain them unless they are generic, intrinsics, inlined or

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

#### Atomics

All atomic intrinsics are compiled as an atomic block where the operation is
performed. But as noted in [Notes - Concurrency](#concurrency), CBMC
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we replace the two occurrences of CBMC in this section by RMC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, done.

@adpaco-aws adpaco-aws merged commit 1f3e3b2 into model-checking:main Jan 4, 2022
@adpaco-aws adpaco-aws mentioned this pull request Jan 5, 2022
tedinski pushed a commit to tedinski/rmc that referenced this pull request Apr 22, 2022
* Initial version of support table

* Define support values for table

* Add intrinsics table and other content

* Observations on stdlib

* Address Celina's comments

* More comments

* Add section on panic strategies

* Address more comments

* Minor comments
tedinski pushed a commit to tedinski/rmc that referenced this pull request Apr 25, 2022
* Initial version of support table

* Define support values for table

* Add intrinsics table and other content

* Observations on stdlib

* Address Celina's comments

* More comments

* Add section on panic strategies

* Address more comments

* Minor comments
tedinski pushed a commit to tedinski/rmc that referenced this pull request Apr 26, 2022
* Initial version of support table

* Define support values for table

* Add intrinsics table and other content

* Observations on stdlib

* Address Celina's comments

* More comments

* Add section on panic strategies

* Address more comments

* Minor comments
tedinski pushed a commit that referenced this pull request Apr 27, 2022
* Initial version of support table

* Define support values for table

* Add intrinsics table and other content

* Observations on stdlib

* Address Celina's comments

* More comments

* Add section on panic strategies

* Address more comments

* Minor comments
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.

4 participants