Skip to content

Do not normalize bounds for invalid decls#1090

Merged
kkjeer merged 3 commits into
masterfrom
normalize-bounds-crash
Jun 11, 2021
Merged

Do not normalize bounds for invalid decls#1090
kkjeer merged 3 commits into
masterfrom
normalize-bounds-crash

Conversation

@kkjeer

@kkjeer kkjeer commented Jun 9, 2021

Copy link
Copy Markdown
Contributor

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

@sulekhark sulekhark left a comment

Copy link
Copy Markdown
Contributor

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

mgrang commented Jun 10, 2021

Copy link
Copy Markdown

LGTM. Thanks.

@sulekhark sulekhark left a comment

Copy link
Copy Markdown
Contributor

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