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

Fix compiler assert. #585

Merged
merged 3 commits into from
Nov 3, 2018
Merged

Fix compiler assert. #585

merged 3 commits into from
Nov 3, 2018

Conversation

dtarditi
Copy link
Contributor

@dtarditi dtarditi commented Nov 3, 2018

This change fixes the compiler assert reported in Github issue #575. We are inserting temporary variables for strings so that we can describe their bounds. The assert was checking that we don't compute a temporary variable more than once in an expression.

The problem is that we are copying the expression that creates and binds the temporary into the bounds. The bounds is used at runtime in the dynamic_bounds_cast. This causes the temporary to be computed twice, which is what the assert is protecting against. Since we've already computed the value
into a temporary, the fix is to replace the binding with a read of the temporary.

Testing:

  • Added simple repro case that caused a compiler crash.
  • Added a runtime test that will be covered by a PR to the Checked C repo
  • Passed testing locally for Window x64.
  • Passed automated testing for Linux.

This change fixes the compiler assert reported in Github issue #575. We are
inserting temporary variables for strings so that we can describe their
bounds.  The assert was checking that we don't compute a temporary
variable more than once in an expression.

The problem is that we are copying the expression that creates and binds the
temporary into the bounds.   The bounds is used at runtime in the
dynamic_bounds_cast.  This causes the temporary to computed twice, which is
what the assert is protecting against.  Since we've already computed the value
into a temporary, the fix is to replace the binding with a read of the
temporary.

Testing:
- Added simple repro case that caused a compiler crash.
- Passed local testing and repro case.  Still needs a runtime test that the
  right thing is happening.
dtarditi added a commit to microsoft/checkedc that referenced this pull request Nov 3, 2018
There was a compiler assert on a dynamic_bounds_cast of a string literal (microsoft/checkedc-clang#575).  This is fixed by microsoft/checkedc-clang#585.   Add runtime tests that check that the right results are being computed in this case.
@dtarditi dtarditi merged commit 18a77c6 into master Nov 3, 2018
@dtarditi dtarditi deleted the issue575 branch November 6, 2018 23:31
sulekhark pushed a commit that referenced this pull request Jul 8, 2021
Iterate over a BoundsKey set instead of a ProgramVar* set so that the
iteration order does not depend on the address of the program variables.
This fixes the issue, but it is still potentially a problem that the
function depends on the order it iterations over the bounds keys. It is
also likely that other loops over sets of pointers exbibit this same
problem.
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.

None yet

1 participant