Skip to content

Fill in some missing cases for bounds-safe interface assignments #813

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 8 commits into from
May 13, 2020

Conversation

kkjeer
Copy link
Contributor

@kkjeer kkjeer commented Mar 31, 2020

This PR addresses issue #810 by filling in two missing cases for bounds-safe interface assignment compatibility:

  1. Extend GetCheckedCInteropType to get the bounds-safe interface type for pointer dereferences and array subscripts
  2. Allow assignment of an unchecked nt_array_ptr with a bounds-safe interface to a checked nt_array_ptr

Testing:

  • Added Checked C tests to test the new assignment compatibility cases (PR #398)
  • Passed manual testing on Windows
  • Passed automated testing on Windows/Linux

kakje added 3 commits March 30, 2020 23:14
/// deference or an array subscript. For rvalue expressions appearing as
/// part of the left-hand side of an assignment, only lvalue-to-rvalue casts
/// and pointer arithmetic have bounds-safe interfaces.
QualType Sema::GetCheckedCRValueInteropType(ExprResult RHS) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to returning the bounds-safe interface type for *e and p +/- i, should this method also return the bounds-safe interface type for &e? If e has bounds-safe interface type T, should &e have bounds-safe interface type ptr<T>?

As an example, making this change would allow the following to compile:

void address_of(int *interop_ptr : itype(ptr<int>), ptr<int> checked_ptr) {
  *(&interop_ptr) = checked_ptr;
}

Currently, this example does not compile since &interop_ptr has an unchecked pointer type int **. If GetCheckedCRValueInteropType were to return the bounds-safe interface type for address-of expressions, then &interop_ptr would have a bounds-safe interface type of ptr<ptr<int>> and so *(&interop_ptr) would have a bounds-safe interface type of ptr<int>.

Copy link
Member

@dtarditi dtarditi left a comment

Choose a reason for hiding this comment

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

Overall, looks good. I have a few suggestions. I also suggest that the issue you raised by treated separately from this initial change. I have some feedback on the issue that you raised.

// If the LHS is a checked nt array type and the RHS is an unchecked
// nt array type with a bounds-safe interface, use the bounds-safe
// interface of the RHS to check the types.
if ((LHSType->isNtCheckedArrayType() ||
Copy link
Member

Choose a reason for hiding this comment

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

Is the test isNtCheckedArrayType needed? I don't think C allows you to make assignments to values with array type.

@@ -9116,6 +9128,23 @@ QualType Sema::GetCheckedCInteropType(ExprResult LHS) {
IsParam = isa<ParmVarDecl>(Var);
}
}
// If `e` has bounds-safe interface type with referent type T, then
Copy link
Member

Choose a reason for hiding this comment

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

I think the wording on this comment is somewhat unclear. It's subtle to say that e has a referent type. How about e has a bounds-safe interface type that is a pointer to T?

// If `e1` has bounds-safe interface type with referent type T and `e2`
// is an integer, then `e1[e2]` and `e2[e1`] have bounds-safe interface
// type T.
else if (ArraySubscriptExpr *Array = dyn_cast<ArraySubscriptExpr>(LHSExpr)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the same change applies here.

@@ -10565,6 +10565,10 @@ class Sema {
/// Returns a null QualType if there isn't one.
QualType GetCheckedCInteropType(ExprResult LHS);
Copy link
Member

Choose a reason for hiding this comment

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

Does this need a clarifying comment? Or to be renamed?

Copy link
Member

@dtarditi dtarditi left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

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.

2 participants