Index: clang/include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- clang/include/clang/Basic/DiagnosticSemaKinds.td +++ clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -8056,6 +8056,10 @@ "%select{binds to|is}2 a temporary object " "whose lifetime is shorter than the lifetime of the constructed object">, InGroup; +def warn_dangling_lifetime_pointer_member : Warning< + "initializing pointer member %0 to point to a temporary object " + "whose lifetime is shorter than the lifetime of the constructed object">, + InGroup; def note_lifetime_extending_member_declared_here : Note< "%select{%select{reference|'std::initializer_list'}0 member|" "member with %select{reference|'std::initializer_list'}0 subobject}1 " @@ -8074,6 +8078,10 @@ "temporary bound to reference member of allocated object " "will be destroyed at the end of the full-expression">, InGroup; +def warn_dangling_lifetime_pointer : Warning< + "object backing the pointer " + "will be destroyed at the end of the full-expression">, + InGroup; def warn_new_dangling_initializer_list : Warning< "array backing " "%select{initializer list subobject of the allocated object|" Index: clang/lib/Sema/SemaInit.cpp =================================================================== --- clang/lib/Sema/SemaInit.cpp +++ clang/lib/Sema/SemaInit.cpp @@ -6507,6 +6507,7 @@ VarInit, LValToRVal, LifetimeBoundCall, + GslPointerInit } Kind; Expr *E; const Decl *D = nullptr; @@ -6551,6 +6552,41 @@ Expr *Init, ReferenceKind RK, LocalVisitor Visit); +template +static bool isRecordWithAttr(QualType Type) { + if (auto *RD = Type->getAsCXXRecordDecl()) + return RD->getCanonicalDecl()->hasAttr(); + return false; +} + +static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr *Call, + LocalVisitor Visit) { + auto VisitPointerArg = [&](const Decl *D, Expr *Arg) { + Path.push_back({IndirectLocalPathEntry::GslPointerInit, Arg, D}); + if (Arg->isGLValue()) + visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding, + Visit); + else + visitLocalsRetainedByInitializer(Path, Arg, Visit, true); + Path.pop_back(); + }; + + if (auto *MCE = dyn_cast(Call)) { + const FunctionDecl *Callee = MCE->getDirectCallee(); + if (auto *Conv = dyn_cast_or_null(Callee)) + if (isRecordWithAttr(Conv->getConversionType())) + VisitPointerArg(Callee, MCE->getImplicitObjectArgument()); + return; + } + + if (auto *CCE = dyn_cast(Call)) { + const auto* Ctor = CCE->getConstructor(); + const CXXRecordDecl *RD = Ctor->getParent()->getCanonicalDecl(); + if (CCE->getNumArgs() > 0 && RD->hasAttr()) + VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0]); + } +} + static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) { const TypeSourceInfo *TSI = FD->getTypeSourceInfo(); if (!TSI) @@ -6672,8 +6708,10 @@ true); } - if (isa(Init)) + if (isa(Init)) { + handleGslAnnotatedTypes(Path, Init, Visit); return visitLifetimeBoundArguments(Path, Init, Visit); + } switch (Init->getStmtClass()) { case Stmt::DeclRefExprClass: { @@ -6896,8 +6934,10 @@ } } - if (isa(Init) || isa(Init)) + if (isa(Init) || isa(Init)) { + handleGslAnnotatedTypes(Path, Init, Visit); return visitLifetimeBoundArguments(Path, Init, Visit); + } switch (Init->getStmtClass()) { case Stmt::UnaryOperatorClass: { @@ -6979,18 +7019,33 @@ case IndirectLocalPathEntry::AddressOf: case IndirectLocalPathEntry::LValToRVal: case IndirectLocalPathEntry::LifetimeBoundCall: + case IndirectLocalPathEntry::GslPointerInit: // These exist primarily to mark the path as not permitting or // supporting lifetime extension. break; - case IndirectLocalPathEntry::DefaultInit: case IndirectLocalPathEntry::VarInit: + if (cast(Path[I].D)->isImplicit()) + return SourceRange(); + LLVM_FALLTHROUGH; + case IndirectLocalPathEntry::DefaultInit: return Path[I].E->getSourceRange(); } } return E->getSourceRange(); } +static bool pathOnlyInitializesGslPointer(IndirectLocalPath &Path) { + int GslPointerInits = 0; + for (auto It = Path.rbegin(), End = Path.rend(); It != End; ++It) { + if (It->Kind == IndirectLocalPathEntry::GslPointerInit) + ++GslPointerInits; + else + break; + } + return GslPointerInits; +} + void Sema::checkInitializerLifetime(const InitializedEntity &Entity, Expr *Init) { LifetimeResult LR = getEntityLifetime(&Entity); @@ -7007,12 +7062,35 @@ SourceRange DiagRange = nextPathEntryRange(Path, 0, L); SourceLocation DiagLoc = DiagRange.getBegin(); + auto *MTE = dyn_cast(L); + bool IsTempGslOwner = MTE && isRecordWithAttr(MTE->getType()); + bool IsLocalGslOwner = + isa(L) && isRecordWithAttr(L->getType()); + + // Skipping a chain of initializing gsl::Pointer annotated objects. + // We are looking only for the final source to find out if it was + // a local or temporary owner or the address of a local variable/param. We + // do not want to follow the references when returning a pointer originating + // from a local owner to avoid the following false positive: + // int &p = *localOwner; + // someContainer.add(std::move(localOWner)); + // return p; + if (!IsTempGslOwner && pathOnlyInitializesGslPointer(Path) && + !(IsLocalGslOwner && !pathContainsInit(Path))) + return true; + + bool IsGslPtrInitWithGslTempOwner = + IsTempGslOwner && pathOnlyInitializesGslPointer(Path); + switch (LK) { case LK_FullExpression: llvm_unreachable("already handled this"); case LK_Extended: { - auto *MTE = dyn_cast(L); + if (IsGslPtrInitWithGslTempOwner && DiagLoc.isValid()) { + Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) << DiagRange; + return false; + } if (!MTE) { // The initialized entity has lifetime beyond the full-expression, // and the local entity does too, so don't warn. @@ -7063,6 +7141,14 @@ // temporary, the program is ill-formed. if (auto *ExtendingDecl = ExtendingEntity ? ExtendingEntity->getDecl() : nullptr) { + if (IsGslPtrInitWithGslTempOwner) { + Diag(DiagLoc, diag::warn_dangling_lifetime_pointer_member) + << ExtendingDecl << DiagRange; + Diag(ExtendingDecl->getLocation(), + diag::note_ref_or_ptr_member_declared_here) + << true; + return false; + } bool IsSubobjectMember = ExtendingEntity != &Entity; Diag(DiagLoc, shouldLifetimeExtendThroughPath(Path) ? diag::err_dangling_member @@ -7103,7 +7189,7 @@ if (auto *Member = ExtendingEntity ? ExtendingEntity->getDecl() : nullptr) { - bool IsPointer = Member->getType()->isAnyPointerType(); + bool IsPointer = !Member->getType()->isReferenceType(); Diag(DiagLoc, IsPointer ? diag::warn_init_ptr_member_to_parameter_addr : diag::warn_bind_ref_member_to_parameter) << Member << VD << isa(VD) << DiagRange; @@ -7117,10 +7203,13 @@ case LK_New: if (isa(L)) { - Diag(DiagLoc, RK == RK_ReferenceBinding - ? diag::warn_new_dangling_reference - : diag::warn_new_dangling_initializer_list) - << !Entity.getParent() << DiagRange; + if (IsGslPtrInitWithGslTempOwner) + Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) << DiagRange; + else + Diag(DiagLoc, RK == RK_ReferenceBinding + ? diag::warn_new_dangling_reference + : diag::warn_new_dangling_initializer_list) + << !Entity.getParent() << DiagRange; } else { // We can't determine if the allocation outlives the local declaration. return false; @@ -7163,7 +7252,8 @@ break; case IndirectLocalPathEntry::LifetimeBoundCall: - // FIXME: Consider adding a note for this. + case IndirectLocalPathEntry::GslPointerInit: + // FIXME: Consider adding a note for these. break; case IndirectLocalPathEntry::DefaultInit: { Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp =================================================================== --- /dev/null +++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp @@ -0,0 +1,135 @@ +// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s +struct [[gsl::Owner(int)]] MyIntOwner { + MyIntOwner(); + int &operator*(); + int *c_str() const; +}; + +struct [[gsl::Pointer(int)]] MyIntPointer { + MyIntPointer(int *p = nullptr); + // Conversion operator and constructor conversion will result in two + // different ASTs. The former is tested with another owner and + // pointer type. + MyIntPointer(const MyIntOwner &); + int &operator*(); + MyIntOwner toOwner(); +}; + +struct [[gsl::Pointer(long)]] MyLongPointerFromConversion { + MyLongPointerFromConversion(long *p = nullptr); + long &operator*(); +}; + +struct [[gsl::Owner(long)]] MyLongOwnerWithConversion { + MyLongOwnerWithConversion(); + operator MyLongPointerFromConversion(); + long &operator*(); + MyIntPointer releaseAsMyPointer(); + long *releaseAsRawPointer(); +}; + +void danglingHeapObject() { + new MyLongPointerFromConversion(MyLongOwnerWithConversion{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}} + new MyIntPointer(MyIntOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}} +} + +void intentionalFalseNegative() { + int i; + MyIntPointer p{&i}; + // In this case we do not have enough information in a statement local + // analysis to detect the problem. + new MyIntPointer(p); + new MyIntPointer(MyIntPointer{p}); +} + +MyIntPointer ownershipTransferToMyPointer() { + MyLongOwnerWithConversion t; + return t.releaseAsMyPointer(); // ok +} + +long *ownershipTransferToRawPointer() { + MyLongOwnerWithConversion t; + return t.releaseAsRawPointer(); // ok +} + +int *danglingRawPtrFromLocal() { + MyIntOwner t; + return t.c_str(); // TODO +} + +int *danglingRawPtrFromTemp() { + MyIntPointer p; + return p.toOwner().c_str(); // TODO +} + +struct Y { + int a[4]; +}; + +void dangligGslPtrFromTemporary() { + MyIntPointer p = Y{}.a; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}} + (void)p; +} + +struct DanglingGslPtrField { + MyIntPointer p; // expected-note 2{{pointer member declared here}} + MyLongPointerFromConversion p2; // expected-note {{pointer member declared here}} + DanglingGslPtrField(int i) : p(&i) {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}} + DanglingGslPtrField() : p2(MyLongOwnerWithConversion{}) {} // expected-warning {{initializing pointer member 'p2' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}} + DanglingGslPtrField(double) : p(MyIntOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}} +}; + +MyIntPointer danglingGslPtrFromLocal() { + int j; + return &j; // expected-warning {{address of stack memory associated with local variable 'j' returned}} +} + +MyIntPointer returningLocalPointer() { + MyIntPointer localPointer; + return localPointer; // ok +} + +MyIntPointer daglingGslPtrFromLocalOwner() { + MyIntOwner localOwner; + return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}} +} + +MyLongPointerFromConversion daglingGslPtrFromLocalOwnerConv() { + MyLongOwnerWithConversion localOwner; + return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}} +} + +MyIntPointer danglingGslPtrFromTemporary() { + return MyIntOwner{}; // expected-warning {{returning address of local temporary object}} +} + +MyLongPointerFromConversion danglingGslPtrFromTemporaryConv() { + return MyLongOwnerWithConversion{}; // expected-warning {{returning address of local temporary object}} +} + +int *noFalsePositive(MyIntOwner &o) { + MyIntPointer p = o; + return &*p; // ok +} + +MyIntPointer global; +MyLongPointerFromConversion global2; + +void initLocalGslPtrWithTempOwner() { + MyIntPointer p = MyIntOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}} + p = MyIntOwner{}; // TODO ? + global = MyIntOwner{}; // TODO ? + MyLongPointerFromConversion p2 = MyLongOwnerWithConversion{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}} + p2 = MyLongOwnerWithConversion{}; // TODO ? + global2 = MyLongOwnerWithConversion{}; // TODO ? +} + +struct IntVector { + int *begin(); + int *end(); +}; + +void modelIterators() { + int *it = IntVector{}.begin(); // TODO ? + (void)it; +}