-
Notifications
You must be signed in to change notification settings - Fork 79
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
Conversation
…interface type of LValueToRValue casts and pointer arithmetic
… type of pointer dereferences and array subscripts
/// 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) { |
There was a problem hiding this comment.
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>
.
…into issue-810-bounds-safe-interfaces
There was a problem hiding this 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.
clang/lib/Sema/SemaExpr.cpp
Outdated
// 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() || |
There was a problem hiding this comment.
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.
clang/lib/Sema/SemaExpr.cpp
Outdated
@@ -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 |
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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.
clang/include/clang/Sema/Sema.h
Outdated
@@ -10565,6 +10565,10 @@ class Sema { | |||
/// Returns a null QualType if there isn't one. | |||
QualType GetCheckedCInteropType(ExprResult LHS); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
…into issue-810-bounds-safe-interfaces
This PR addresses issue #810 by filling in two missing cases for bounds-safe interface assignment compatibility:
Testing: