Skip to content

Do not normalize bounds for invalid decls #1090

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

Merged
merged 3 commits into from
Jun 11, 2021
Merged

Conversation

kkjeer
Copy link
Contributor

@kkjeer kkjeer commented Jun 9, 2021

Fixes #1084

This PR fixes a crash that would occur in Sema::NormalizeBounds for certain kinds of (invalid) VarDecls. When expanding bounds to a range, Sema needs to create a DeclRefExpr for a VarDecl. In order to do so, it may call Sema::ConvertToFullyCheckedType, which asserts that the type of the VarDecl is not an unchecked type and does not contain an unchecked type.

Consider the following example:

_Checked void f(int **p : count(i), int i) {
  i = 0;
}

In a checked scope, this produces the error "parameter in a checked scope must have a bounds-safe interface type that uses only checked types or parameter/return types with bounds-safe interfaces". p is an invalid declaration.

Accounting for the bounds-safe interface, the type of p is _Array_ptr<int *>. If we attempt to normalize the declared bounds count(i) for p, the assertion in ConvertToFullyCheckedType fails since _Array_ptr<int *> contains the unchecked type int *. Therefore, we should not attempt to normalize the declared bounds for p. We do not include p in the observed bounds context, and we do not attempt to validate the bounds of p after each statement. The statement i = 0 results in no bounds validation errors.

@kkjeer kkjeer requested review from mgrang and sulekhark June 9, 2021 21:55
Copy link
Contributor

@sulekhark sulekhark left a comment

Choose a reason for hiding this comment

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

LGTM!
Do we want to remove the function Sema::ExpandBoundsToRange by fusing it with Sema::NormalizeBounds (with a check to avoid unnecessary expansion if already expanded)? This is because both NormalizeBounds and ExpandBoundsToRange can potentially be called from outside the class, and ExpandBoundsToRange does not have the same check for an invalid decl. Moreover, currently ExpandBoundsToRange is called only by NormalizeBounds. It does not make sense to add another check for invalid decl in ExpandBoundsToRange.

@mgrang
Copy link

mgrang commented Jun 10, 2021

LGTM. Thanks.

Copy link
Contributor

@sulekhark sulekhark left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@kkjeer kkjeer merged commit ba89be3 into master Jun 11, 2021
@kkjeer kkjeer deleted the normalize-bounds-crash branch June 11, 2021 17:29
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.

Crash while compilation
3 participants