-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[Clang] Diagnose forming references to nullptr #143667
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
base: main
Are you sure you want to change the base?
Changes from all commits
a3f8598
07ae0de
43cb1b4
a8352d1
1d1a635
cd750c9
aee78c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1529,7 +1529,7 @@ CallStackFrame::~CallStackFrame() { | |
|
||
static bool isRead(AccessKinds AK) { | ||
return AK == AK_Read || AK == AK_ReadObjectRepresentation || | ||
AK == AK_IsWithinLifetime; | ||
AK == AK_IsWithinLifetime || AK == AK_Dereference; | ||
} | ||
|
||
static bool isModification(AccessKinds AK) { | ||
|
@@ -1540,6 +1540,7 @@ static bool isModification(AccessKinds AK) { | |
case AK_DynamicCast: | ||
case AK_TypeId: | ||
case AK_IsWithinLifetime: | ||
case AK_Dereference: | ||
return false; | ||
case AK_Assign: | ||
case AK_Increment: | ||
|
@@ -1558,15 +1559,16 @@ static bool isAnyAccess(AccessKinds AK) { | |
/// Is this an access per the C++ definition? | ||
static bool isFormalAccess(AccessKinds AK) { | ||
return isAnyAccess(AK) && AK != AK_Construct && AK != AK_Destroy && | ||
AK != AK_IsWithinLifetime; | ||
AK != AK_IsWithinLifetime && AK != AK_Dereference; | ||
} | ||
|
||
/// Is this kind of axcess valid on an indeterminate object value? | ||
/// Is this kind of access valid on an indeterminate object value? | ||
static bool isValidIndeterminateAccess(AccessKinds AK) { | ||
switch (AK) { | ||
case AK_Read: | ||
case AK_Increment: | ||
case AK_Decrement: | ||
case AK_Dereference: | ||
// These need the object's value. | ||
return false; | ||
|
||
|
@@ -1733,7 +1735,10 @@ namespace { | |
bool checkNullPointerForFoldAccess(EvalInfo &Info, const Expr *E, | ||
AccessKinds AK) { | ||
return checkNullPointerDiagnosingWith([&Info, E, AK] { | ||
Info.FFDiag(E, diag::note_constexpr_access_null) << AK; | ||
if (AK == AccessKinds::AK_Dereference) | ||
Info.FFDiag(E, diag::note_constexpr_dereferencing_null); | ||
else | ||
Info.FFDiag(E, diag::note_constexpr_access_null) << AK; | ||
}); | ||
} | ||
|
||
|
@@ -4305,7 +4310,10 @@ static CompleteObject findCompleteObject(EvalInfo &Info, const Expr *E, | |
} | ||
|
||
if (!LVal.Base) { | ||
Info.FFDiag(E, diag::note_constexpr_access_null) << AK; | ||
if (AK == AccessKinds::AK_Dereference) | ||
Info.FFDiag(E, diag::note_constexpr_dereferencing_null); | ||
else | ||
Info.FFDiag(E, diag::note_constexpr_access_null) << AK; | ||
return CompleteObject(); | ||
} | ||
|
||
|
@@ -4407,8 +4415,9 @@ static CompleteObject findCompleteObject(EvalInfo &Info, const Expr *E, | |
ConstexprVar = VD->isConstexpr(); | ||
|
||
// Unless we're looking at a local variable or argument in a constexpr call, | ||
// the variable we're reading must be const. | ||
if (!Frame) { | ||
// the variable we're reading must be const (unless we are binding to a | ||
// reference). | ||
if (AK != clang::AK_Dereference && !Frame) { | ||
if (IsAccess && isa<ParmVarDecl>(VD)) { | ||
// Access of a parameter that's not associated with a frame isn't going | ||
// to work out, but we can leave it to evaluateVarDeclInit to provide a | ||
|
@@ -4472,12 +4481,16 @@ static CompleteObject findCompleteObject(EvalInfo &Info, const Expr *E, | |
} | ||
} | ||
|
||
if (!evaluateVarDeclInit(Info, E, VD, Frame, LVal.getLValueVersion(), BaseVal)) | ||
// When binding to a reference, the variable does not need to be constexpr | ||
// or have constant initalization. | ||
if (AK != clang::AK_Dereference && | ||
!evaluateVarDeclInit(Info, E, VD, Frame, LVal.getLValueVersion(), | ||
BaseVal)) | ||
return CompleteObject(); | ||
// If evaluateVarDeclInit sees a constexpr-unknown variable, it returns | ||
// a null BaseVal. Any constexpr-unknown variable seen here is an error: | ||
// we can't access a constexpr-unknown object. | ||
if (!BaseVal) { | ||
if (AK != clang::AK_Dereference && !BaseVal) { | ||
Info.FFDiag(E, diag::note_constexpr_access_unknown_variable, 1) | ||
<< AK << VD; | ||
Info.Note(VD->getLocation(), diag::note_declared_at); | ||
|
@@ -4491,7 +4504,10 @@ static CompleteObject findCompleteObject(EvalInfo &Info, const Expr *E, | |
} | ||
return CompleteObject(LVal.Base, &(*Alloc)->Value, | ||
LVal.Base.getDynamicAllocType()); | ||
} else { | ||
} | ||
// When binding to a reference, the variable does not need to be | ||
// within its lifetime. | ||
else if (AK != clang::AK_Dereference) { | ||
const Expr *Base = LVal.Base.dyn_cast<const Expr*>(); | ||
|
||
if (!Frame) { | ||
|
@@ -4572,7 +4588,7 @@ static CompleteObject findCompleteObject(EvalInfo &Info, const Expr *E, | |
NoteLValueLocation(Info, LVal.Base); | ||
return CompleteObject(); | ||
} | ||
} else { | ||
} else if (AK != clang::AK_Dereference) { | ||
AaronBallman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
BaseVal = Frame->getTemporary(Base, LVal.Base.getVersion()); | ||
assert(BaseVal && "missing value for temporary"); | ||
} | ||
|
@@ -5200,6 +5216,29 @@ enum EvalStmtResult { | |
ESR_CaseNotFound | ||
}; | ||
} | ||
/// Evaluates the initializer of a reference. | ||
static bool EvaluateInitForDeclOfReferenceType(EvalInfo &Info, | ||
const ValueDecl *D, | ||
const Expr *Init, LValue &Result, | ||
APValue &Val) { | ||
assert(Init->isGLValue() && D->getType()->isReferenceType()); | ||
// A reference is an lvalue. | ||
if (!EvaluateLValue(Init, Result, Info)) | ||
return false; | ||
// [C++26][decl.ref] | ||
// The object designated by such a glvalue can be outside its lifetime | ||
// Because a null pointer value or a pointer past the end of an object | ||
// does not point to an object, a reference in a well-defined program cannot | ||
// refer to such things; | ||
if (!Result.Designator.Invalid && Result.Designator.isOnePastTheEnd()) { | ||
Info.FFDiag(Init, diag::note_constexpr_access_past_end) << AK_Dereference; | ||
return false; | ||
} | ||
|
||
// Save the result. | ||
Result.moveInto(Val); | ||
return true; | ||
} | ||
|
||
static bool EvaluateVarDecl(EvalInfo &Info, const VarDecl *VD) { | ||
if (VD->isInvalidDecl()) | ||
|
@@ -5221,7 +5260,11 @@ static bool EvaluateVarDecl(EvalInfo &Info, const VarDecl *VD) { | |
if (InitE->isValueDependent()) | ||
return false; | ||
|
||
if (!EvaluateInPlace(Val, Info, Result, InitE)) { | ||
// For references to objects, check they do not designate a one-past-the-end | ||
// object. | ||
if (VD->getType()->isReferenceType()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So all the rules are the same for lvalue and rvalue references? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, although I did interpret your question as a veiled request for more tests :):) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I was trying to suss out whether I should ask for more tests or not. :-D |
||
return EvaluateInitForDeclOfReferenceType(Info, VD, InitE, Result, Val); | ||
} else if (!EvaluateInPlace(Val, Info, Result, InitE)) { | ||
// Wipe out any partially-computed value, to allow tracking that this | ||
// evaluation failed. | ||
Val = APValue(); | ||
|
@@ -6851,9 +6894,18 @@ static bool HandleConstructorCall(const Expr *E, const LValue &This, | |
ThisOverrideRAII ThisOverride(*Info.CurrentCall, &SubobjectParent, | ||
isa<CXXDefaultInitExpr>(Init)); | ||
FullExpressionRAII InitScope(Info); | ||
if (!EvaluateInPlace(*Value, Info, Subobject, Init) || | ||
(FD && FD->isBitField() && | ||
!truncateBitfieldValue(Info, Init, *Value, FD))) { | ||
if (FD && FD->getType()->isReferenceType() && | ||
!FD->getType()->isFunctionReferenceType()) { | ||
LValue Result; | ||
if (!EvaluateInitForDeclOfReferenceType(Info, FD, Init, Result, | ||
*Value)) { | ||
if (!Info.noteFailure()) | ||
return false; | ||
Success = false; | ||
} | ||
} else if (!EvaluateInPlace(*Value, Info, Subobject, Init) || | ||
(FD && FD->isBitField() && | ||
!truncateBitfieldValue(Info, Init, *Value, FD))) { | ||
// If we're checking for a potential constant expression, evaluate all | ||
// initializers even if some of them fail. | ||
if (!Info.noteFailure()) | ||
|
@@ -9287,7 +9339,13 @@ bool LValueExprEvaluator::VisitArraySubscriptExpr(const ArraySubscriptExpr *E) { | |
} | ||
|
||
bool LValueExprEvaluator::VisitUnaryDeref(const UnaryOperator *E) { | ||
return evaluatePointer(E->getSubExpr(), Result); | ||
bool Success = evaluatePointer(E->getSubExpr(), Result); | ||
// [C++26][expr.unary.op] | ||
// If the operand points to an object or function, the result | ||
// denotes that object or function; otherwise, the behavior is undefined. | ||
return Success && | ||
(!E->getType().getNonReferenceType()->isObjectType() || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment explaining this additional check added after the |
||
findCompleteObject(Info, E, AK_Dereference, Result, E->getType())); | ||
} | ||
|
||
bool LValueExprEvaluator::VisitUnaryReal(const UnaryOperator *E) { | ||
|
@@ -10906,9 +10964,17 @@ bool RecordExprEvaluator::VisitCXXParenListOrInitListExpr( | |
isa<CXXDefaultInitExpr>(Init)); | ||
|
||
APValue &FieldVal = Result.getStructField(Field->getFieldIndex()); | ||
if (!EvaluateInPlace(FieldVal, Info, Subobject, Init) || | ||
(Field->isBitField() && !truncateBitfieldValue(Info, Init, | ||
FieldVal, Field))) { | ||
if (Field->getType()->isReferenceType()) { | ||
LValue Result; | ||
if (!EvaluateInitForDeclOfReferenceType(Info, Field, Init, Result, | ||
FieldVal)) { | ||
if (!Info.noteFailure()) | ||
return false; | ||
Success = false; | ||
} | ||
} else if (!EvaluateInPlace(FieldVal, Info, Subobject, Init) || | ||
(Field->isBitField() && | ||
!truncateBitfieldValue(Info, Init, FieldVal, Field))) { | ||
if (!Info.noteFailure()) | ||
return false; | ||
Success = false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -413,7 +413,7 @@ namespace DeriveFailures { | |
|
||
constexpr Derived(int i) : OtherVal(i) {} // ref-error {{never produces a constant expression}} \ | ||
// both-note {{non-constexpr constructor 'Base' cannot be used in a constant expression}} \ | ||
// ref-note {{non-constexpr constructor 'Base' cannot be used in a constant expression}} | ||
// ref-note {{non-constexpr constructor 'Base' cannot be used in a constant expression}} | ||
}; | ||
|
||
constexpr Derived D(12); // both-error {{must be initialized by a constant expression}} \ | ||
|
@@ -1660,9 +1660,11 @@ namespace NullptrCast { | |
constexpr A *na = nullptr; | ||
constexpr B *nb = nullptr; | ||
constexpr A &ra = *nb; // both-error {{constant expression}} \ | ||
// both-note {{cannot access base class of null pointer}} | ||
// ref-note {{dereferencing a null pointer}} \ | ||
// expected-note {{cannot access base class of null pointer}} | ||
constexpr B &rb = (B&)*na; // both-error {{constant expression}} \ | ||
// both-note {{cannot access derived class of null pointer}} | ||
// ref-note {{dereferencing a null pointer}} \ | ||
// expected-note {{cannot access derived class of null pointer}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some of the new diagnostics seem like regressions. |
||
constexpr bool test() { | ||
auto a = (A*)(B*)nullptr; | ||
|
||
|
@@ -1740,7 +1742,7 @@ namespace CtorOfInvalidClass { | |
#if __cplusplus >= 202002L | ||
template <typename T, auto Q> | ||
concept ReferenceOf = Q; | ||
/// This calls a valid and constexpr copy constructor of InvalidCtor, | ||
/// This calls a valid and constexpr copy constructor of InvalidCtor, | ||
/// but should still be rejected. | ||
template<ReferenceOf<InvalidCtor> auto R, typename Rep> int F; // both-error {{non-type template argument is not a constant expression}} | ||
#endif | ||
|
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.
Not sure I like "read of". Maybe we should explicitly mention reference binding?
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 like we're still using
read of
, is that something you were thinking of changing @cor3ntin ?