xazax.hun created this revision.
xazax.hun added reviewers: mgehre, rsmith, gribozavr.
Herald added subscribers: cfe-commits, Charusso, gamesh411, Szelethus, dkrupp,
rnkovacs.
Herald added a project: clang.
This patch extends some existing warnings to utilize the knowledge about the
gsl::Pointer and gsl::Owner attributes.
The implicit assumption of this patch is that if a class annotated with
gsl::Pointer is created from a gsl::Owner (through conversion operator or a
constructor taking the owner as the first argument) the Pointer will point to
the buffer of the Owner, so it will dangle after the lifetime of the Owner ends.
Repository:
rC Clang
https://reviews.llvm.org/D64256
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaInit.cpp
clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===================================================================
--- /dev/null
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -0,0 +1,104 @@
+// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s
+struct [[gsl::Owner(int)]] MyOwner {
+ MyOwner();
+ int &operator*();
+};
+
+struct T;
+
+struct [[gsl::Pointer(int)]] MyPointer {
+ MyPointer(int *p = 0);
+ MyPointer(const MyOwner &);
+ int &operator*();
+ T toOwner();
+};
+
+struct [[gsl::Owner(int)]] T {
+ T();
+ operator MyPointer();
+ int &operator*();
+ MyPointer release();
+ int *release2();
+ int *c_str() const;
+};
+
+void f() {
+ new MyPointer(T{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+ new MyPointer(MyOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+void g() {
+ int i;
+ MyPointer p{&i}; // ok
+ new MyPointer(MyPointer{p}); // ok
+}
+
+MyPointer g2() {
+ T t;
+ return t.release(); // ok
+}
+
+int *g3() {
+ T t;
+ return t.release2(); // ok
+}
+
+int *g4() {
+ T t;
+ return t.c_str(); // TODO
+}
+
+int *g5() {
+ MyPointer p;
+ return p.toOwner().c_str(); // TODO
+}
+
+struct Y {
+ int a[4];
+};
+
+void h() {
+ MyPointer 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 S {
+ MyPointer p; // expected-note 3{{pointer member declared here}}
+ S(int i) : p(&i) {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}}
+ S() : p(T{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+ S(double) : p(MyOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+};
+
+MyPointer i() {
+ int j;
+ return &j; // expected-warning {{address of stack memory associated with local variable 'j' returned}}
+}
+
+MyPointer i2() {
+ T t;
+ return t; // TODO
+}
+
+MyPointer i3() {
+ return T{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyPointer global;
+
+void j() {
+ MyPointer p = MyOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+ p = MyOwner{}; // TODO ?
+ global = MyOwner{}; // TODO ?
+ p = T{}; // TODO ?
+ global = T{}; // TODO ?
+}
+
+struct IntVector {
+ int *begin();
+ int *end();
+};
+
+void future_work() {
+ int *it = IntVector{}.begin(); // TODO ?
+ (void)it;
+}
Index: clang/lib/Sema/SemaInit.cpp
===================================================================
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6507,6 +6507,8 @@
VarInit,
LValToRVal,
LifetimeBoundCall,
+ LifetimePointerInit,
+ LifetimeTempOwner
} Kind;
Expr *E;
const Decl *D = nullptr;
@@ -6543,6 +6545,13 @@
});
}
+static bool
+pathInitializeLifetimePointer(llvm::ArrayRef<IndirectLocalPathEntry> Path) {
+ return Path.size() > 0 && llvm::all_of(Path, [=](IndirectLocalPathEntry E) {
+ return E.Kind == IndirectLocalPathEntry::LifetimePointerInit;
+ });
+}
+
static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
Expr *Init, LocalVisitor Visit,
bool RevisitSubinits);
@@ -6568,32 +6577,53 @@
return false;
}
+template<typename T>
+static bool recordHasAttr(QualType Type) {
+ if (auto *RD = Type->getAsCXXRecordDecl())
+ return RD->getCanonicalDecl()->hasAttr<T>();
+ return false;
+}
+
static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
LocalVisitor Visit) {
const FunctionDecl *Callee;
ArrayRef<Expr*> Args;
+ bool CtorOfLifetimePointer = false;
if (auto *CE = dyn_cast<CallExpr>(Call)) {
Callee = CE->getDirectCallee();
Args = llvm::makeArrayRef(CE->getArgs(), CE->getNumArgs());
} else {
auto *CCE = cast<CXXConstructExpr>(Call);
- Callee = CCE->getConstructor();
+ const auto* Ctor = CCE->getConstructor();
+ Callee = Ctor;
+ const CXXRecordDecl *RD = Ctor->getParent()->getCanonicalDecl();
+ if (RD->hasAttr<OwnerAttr>() && isa<CXXTemporaryObjectExpr>(Call)) {
+ Path.push_back({IndirectLocalPathEntry::LifetimeTempOwner, Call, RD});
+ Visit(Path, Call, RK_ReferenceBinding);
+ Path.pop_back();
+ } else
+ CtorOfLifetimePointer = RD->hasAttr<PointerAttr>();
Args = llvm::makeArrayRef(CCE->getArgs(), CCE->getNumArgs());
}
if (!Callee)
return;
+ bool ConvToLifetimePointer = false;
Expr *ObjectArg = nullptr;
if (isa<CXXOperatorCallExpr>(Call) && Callee->isCXXInstanceMember()) {
ObjectArg = Args[0];
Args = Args.slice(1);
} else if (auto *MCE = dyn_cast<CXXMemberCallExpr>(Call)) {
ObjectArg = MCE->getImplicitObjectArgument();
+ if (auto *Conv = dyn_cast<CXXConversionDecl>(Callee))
+ ConvToLifetimePointer =
+ recordHasAttr<PointerAttr>(Conv->getConversionType());
}
- auto VisitLifetimeBoundArg = [&](const Decl *D, Expr *Arg) {
- Path.push_back({IndirectLocalPathEntry::LifetimeBoundCall, Arg, D});
+ auto VisitLifetimeArg = [&](const Decl *D, Expr *Arg,
+ IndirectLocalPathEntry::EntryKind Kind) {
+ Path.push_back({Kind, Arg, D});
if (Arg->isGLValue())
visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
Visit);
@@ -6602,14 +6632,21 @@
Path.pop_back();
};
- if (ObjectArg && implicitObjectParamIsLifetimeBound(Callee))
- VisitLifetimeBoundArg(Callee, ObjectArg);
+ if (ObjectArg &&
+ (implicitObjectParamIsLifetimeBound(Callee) || ConvToLifetimePointer))
+ VisitLifetimeArg(Callee, ObjectArg,
+ ConvToLifetimePointer ? IndirectLocalPathEntry::LifetimePointerInit :
+ IndirectLocalPathEntry::LifetimeBoundCall);
for (unsigned I = 0,
N = std::min<unsigned>(Callee->getNumParams(), Args.size());
I != N; ++I) {
if (Callee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>())
- VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]);
+ VisitLifetimeArg(Callee->getParamDecl(I), Args[I],
+ IndirectLocalPathEntry::LifetimeBoundCall);
+ if (I == 0 && CtorOfLifetimePointer)
+ VisitLifetimeArg(Callee->getParamDecl(I), Args[I],
+ IndirectLocalPathEntry::LifetimePointerInit);
}
}
@@ -6979,6 +7016,8 @@
case IndirectLocalPathEntry::AddressOf:
case IndirectLocalPathEntry::LValToRVal:
case IndirectLocalPathEntry::LifetimeBoundCall:
+ case IndirectLocalPathEntry::LifetimePointerInit:
+ case IndirectLocalPathEntry::LifetimeTempOwner:
// These exist primarily to mark the path as not permitting or
// supporting lifetime extension.
break;
@@ -7007,12 +7046,32 @@
SourceRange DiagRange = nextPathEntryRange(Path, 0, L);
SourceLocation DiagLoc = DiagRange.getBegin();
+ // Skipping a chain of initializing lifetime Pointer objects.
+ // We are looking only for the final value to find out if it was
+ // a temporary owner or a local variable/param.
+ if (pathInitializeLifetimePointer(Path))
+ return true;
+
+ bool IsLifetimePtrInitWithTempOwner = false;
+ if (!Path.empty() &&
+ Path.back().Kind == IndirectLocalPathEntry::LifetimeTempOwner) {
+ auto Prefix = llvm::makeArrayRef(Path).drop_back();
+ if (pathInitializeLifetimePointer(Prefix))
+ IsLifetimePtrInitWithTempOwner = true;
+ else
+ return false;
+ }
+
switch (LK) {
case LK_FullExpression:
llvm_unreachable("already handled this");
case LK_Extended: {
auto *MTE = dyn_cast<MaterializeTemporaryExpr>(L);
+ if (IsLifetimePtrInitWithTempOwner) {
+ 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.
@@ -7048,10 +7107,10 @@
if (pathContainsInit(Path))
return false;
+ const ValueDecl *ExtendingDecl = ExtendingEntity->getDecl();
Diag(DiagLoc, diag::warn_dangling_variable)
- << RK << !Entity.getParent()
- << ExtendingEntity->getDecl()->isImplicit()
- << ExtendingEntity->getDecl() << Init->isGLValue() << DiagRange;
+ << RK << !Entity.getParent() << ExtendingDecl->isImplicit()
+ << ExtendingDecl << Init->isGLValue() << DiagRange;
}
break;
}
@@ -7093,17 +7152,26 @@
if (pathContainsInit(Path))
return false;
+ auto *Member = ExtendingEntity ? ExtendingEntity->getDecl() : nullptr;
auto *DRE = dyn_cast<DeclRefExpr>(L);
auto *VD = DRE ? dyn_cast<VarDecl>(DRE->getDecl()) : nullptr;
if (!VD) {
+ if (IsLifetimePtrInitWithTempOwner && Member) {
+ Diag(DiagLoc, diag::warn_dangling_lifetime_pointer_member)
+ << Member << DiagRange;
+ Diag(Member->getLocation(),
+ diag::note_ref_or_ptr_member_declared_here)
+ << true;
+ return false;
+ }
// A member was initialized to a local block.
// FIXME: Warn on this.
return false;
}
- if (auto *Member =
- ExtendingEntity ? ExtendingEntity->getDecl() : nullptr) {
- bool IsPointer = Member->getType()->isAnyPointerType();
+ if (Member) {
+ bool IsPointer = Member->getType()->isAnyPointerType() ||
+ recordHasAttr<PointerAttr>(Member->getType());
Diag(DiagLoc, IsPointer ? diag::warn_init_ptr_member_to_parameter_addr
: diag::warn_bind_ref_member_to_parameter)
<< Member << VD << isa<ParmVarDecl>(VD) << DiagRange;
@@ -7122,6 +7190,8 @@
: diag::warn_new_dangling_initializer_list)
<< !Entity.getParent() << DiagRange;
} else {
+ if (IsLifetimePtrInitWithTempOwner)
+ Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) << DiagRange;
// We can't determine if the allocation outlives the local declaration.
return false;
}
@@ -7163,7 +7233,9 @@
break;
case IndirectLocalPathEntry::LifetimeBoundCall:
- // FIXME: Consider adding a note for this.
+ case IndirectLocalPathEntry::LifetimePointerInit:
+ case IndirectLocalPathEntry::LifetimeTempOwner:
+ // FIXME: Consider adding a note for these.
break;
case IndirectLocalPathEntry::DefaultInit: {
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<DanglingField>;
+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<DanglingField>;
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<DanglingField>;
+def warn_dangling_lifetime_pointer : Warning<
+ "object backing the pointer "
+ "will be destroyed at the end of the full-expression">,
+ InGroup<Dangling>;
def warn_new_dangling_initializer_list : Warning<
"array backing "
"%select{initializer list subobject of the allocated object|"
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
