[BoundsSafety] Do not check for bounds-attributed output arguments in dependent contexts #9733
Conversation
ziqingluo-90
requested review from
delcypher,
hnrklssn,
rapidsna and
patrykstefanski
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
|
|
||
| template<typename T> | ||
| struct S { | ||
| void f(T p) { | ||
| g(sizeof(p)); | ||
| g(nullptr, sizeof(p)); |
There was a problem hiding this comment.
Are we certain this still exercises the same path as caused the crash previously, or should we keep the old function and add another?
There was a problem hiding this comment.
The old one was testing for no type checking in a dependent context, if there is no bounds attribute. Now we are testing that the same path will not be reached under a stronger condition, i.e., no type checking in a dependent context.
So the new test is stronger and covers the old one after the change of this PR.
| @@ -8332,7 +8332,7 @@ static bool checkDynamicCountPointerAsParameter(Sema &S, FunctionDecl *FDecl, | |||
| if (!ArgInfo.isCountInParamOrCountPointer() && !ArgInfo.isCountInRet() && | |||
| !ParmInfo.isCountInParamOrCountPointer() && !ParmInfo.isCountInRet()) | |||
| continue; | |||
| assert(!ActualArgExp->isValueDependent()); | |||
There was a problem hiding this comment.
Is is it still necessary to remove this, despite checking !TheCall->getDependence() before calling checkDynamicCountPointerAsParameter? Or is this something else?
There was a problem hiding this comment.
!TheCall->getDependence() implies no argument isValueDependent(). So I think that assertion becomes unreachable.
This is where I got the sense that Call->getDependence() includes dependence info of its arguments:
https://github.com/llvm/llvm-project/blob/2916352936097a35cdcaaf38a9097465adbf8cf5/clang/lib/AST/ComputeDependence.cpp#L642C1-L654C2
| if (!checkDynamicCountPointerAsParameter(*this, FDecl, TheCall)) | ||
| if (!TheCall // Attr-only mode may be applied to C++ but we | ||
| ->getDependence() // don't want to check for any dependent | ||
| // constructs |
There was a problem hiding this comment.
Nit: Maybe instead of casting enum to bool, we could be explicit TheCall->getDependence() == ExprDependence::None? We could also move the comment before if.
ziqingluo-90
force-pushed
the
eng/PR-140895392-2
branch
from
3257260 to
0938253
Compare
|
@swift-ci test |
| @@ -8903,7 +8904,11 @@ ExprResult Sema::BuildResolvedCallExpr(Expr *Fn, NamedDecl *NDecl, | |||
| if (getLangOpts().BoundsSafetyAttributes && FDecl) { | |||
| // FIXME: We need to support function pointers and blocks that don't have | |||
| // function decl. | |||
| if (!checkDynamicCountPointerAsParameter(*this, FDecl, TheCall)) | |||
|
|
|||
| // Attr-only mode may be applied to C++ but we don't want to check for any | |||
There was a problem hiding this comment.
Nit: "For C++, we don't want to check for any dependent constructs."
…dent contexts The bounds-attribute-only mode may be applied to C++ programs, where attributed functions/fields may be used in dependent contexts such as a template. DO NOT type check at those places. Their instantiations will still be checked. (rdar://141708643)
ziqingluo-90
force-pushed
the
eng/PR-140895392-2
branch
from
0938253 to
2c953d2
Compare
…dent contexts (swiftlang#9733) The bounds-attribute-only mode may be applied to C++ programs, where attributed functions/fields may be used in dependent contexts such as a template. DO NOT type check at those places. Their instantiations will still be checked. (rdar://141708643) (cherry picked from commit d4329b6)
The bounds-attribute-only mode may be applied to C++ programs, where attributed functions/fields can be used in dependent contexts such as a template. We DO NOT want the type check at those places. Their instantiations will still be checked.
Note: bounds-attributed output arguments are the only ones being type checked in the attribute-only mode.
(rdar://140895392)
(rdar://141708643) // <- feel like better have a new radar for this issue