xazax.hun updated this revision to Diff 211580.
xazax.hun added a comment.
- Fix a false positive case found by running over ~200 open source projects
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64256/new/
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,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;
+}
Index: clang/lib/Sema/SemaInit.cpp
===================================================================
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6507,6 +6507,8 @@
VarInit,
LValToRVal,
LifetimeBoundCall,
+ GslPointerInit,
+ GslOwnerTemporaryInit
} Kind;
Expr *E;
const Decl *D = nullptr;
@@ -6551,6 +6553,47 @@
Expr *Init, ReferenceKind RK,
LocalVisitor Visit);
+template<typename T>
+static bool isRecordWithAttr(QualType Type) {
+ if (auto *RD = Type->getAsCXXRecordDecl())
+ return RD->getCanonicalDecl()->hasAttr<T>();
+ 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<CXXMemberCallExpr>(Call)) {
+ const FunctionDecl *Callee = MCE->getDirectCallee();
+ if (auto *Conv = dyn_cast_or_null<CXXConversionDecl>(Callee))
+ if (isRecordWithAttr<PointerAttr>(Conv->getConversionType()))
+ VisitPointerArg(Callee, MCE->getImplicitObjectArgument());
+ return;
+ }
+
+ if (auto *CCE = dyn_cast<CXXConstructExpr>(Call)) {
+ const auto* Ctor = CCE->getConstructor();
+ const CXXRecordDecl *RD = Ctor->getParent()->getCanonicalDecl();
+ if (RD->hasAttr<OwnerAttr>() && isa<CXXTemporaryObjectExpr>(Call)) {
+ Path.push_back({IndirectLocalPathEntry::GslOwnerTemporaryInit, Call, RD});
+ Visit(Path, Call, RK_ReferenceBinding);
+ Path.pop_back();
+ } else {
+ if (RD->hasAttr<PointerAttr>() && CCE->getNumArgs() > 0)
+ VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0]);
+ }
+ }
+}
+
static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) {
const TypeSourceInfo *TSI = FD->getTypeSourceInfo();
if (!TSI)
@@ -6672,8 +6715,10 @@
true);
}
- if (isa<CallExpr>(Init))
+ if (isa<CallExpr>(Init)) {
+ handleGslAnnotatedTypes(Path, Init, Visit);
return visitLifetimeBoundArguments(Path, Init, Visit);
+ }
switch (Init->getStmtClass()) {
case Stmt::DeclRefExprClass: {
@@ -6896,8 +6941,10 @@
}
}
- if (isa<CallExpr>(Init) || isa<CXXConstructExpr>(Init))
+ if (isa<CallExpr>(Init) || isa<CXXConstructExpr>(Init)) {
+ handleGslAnnotatedTypes(Path, Init, Visit);
return visitLifetimeBoundArguments(Path, Init, Visit);
+ }
switch (Init->getStmtClass()) {
case Stmt::UnaryOperatorClass: {
@@ -6979,6 +7026,8 @@
case IndirectLocalPathEntry::AddressOf:
case IndirectLocalPathEntry::LValToRVal:
case IndirectLocalPathEntry::LifetimeBoundCall:
+ case IndirectLocalPathEntry::GslPointerInit:
+ case IndirectLocalPathEntry::GslOwnerTemporaryInit:
// These exist primarily to mark the path as not permitting or
// supporting lifetime extension.
break;
@@ -6991,6 +7040,17 @@
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 && !pathContainsInit(Path);
+}
+
void Sema::checkInitializerLifetime(const InitializedEntity &Entity,
Expr *Init) {
LifetimeResult LR = getEntityLifetime(&Entity);
@@ -7007,12 +7067,35 @@
SourceRange DiagRange = nextPathEntryRange(Path, 0, L);
SourceLocation DiagLoc = DiagRange.getBegin();
+ // Skipping a chain of initializing gsl::Pointer annotated objects.
+ // We are looking only for the final temporary value to find out if it was
+ // an owner or the address of a local variable/param. For
+ // 'return LocalVar;' the path will only contain GslPtrInits but a
+ // DeclRefExpr will close the initialization chain.
+ if (pathOnlyInitializesGslPointer(Path) &&
+ !(isa<DeclRefExpr>(L) && isRecordWithAttr<OwnerAttr>(L->getType())))
+ return true;
+
+ bool IsGslPtrInitWithGslTempOwner = false;
+ if (!Path.empty() &&
+ Path.back().Kind == IndirectLocalPathEntry::GslOwnerTemporaryInit) {
+ IndirectLocalPathEntry LastEntry = Path.pop_back_val();
+ IsGslPtrInitWithGslTempOwner = pathOnlyInitializesGslPointer(Path);
+ Path.push_back(LastEntry);
+ if (!IsGslPtrInitWithGslTempOwner)
+ return false;
+ }
+
switch (LK) {
case LK_FullExpression:
llvm_unreachable("already handled this");
case LK_Extended: {
auto *MTE = dyn_cast<MaterializeTemporaryExpr>(L);
+ if (IsGslPtrInitWithGslTempOwner) {
+ 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.
@@ -7093,17 +7176,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 (IsGslPtrInitWithGslTempOwner && 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() ||
+ isRecordWithAttr<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 +7214,8 @@
: diag::warn_new_dangling_initializer_list)
<< !Entity.getParent() << DiagRange;
} else {
+ if (IsGslPtrInitWithGslTempOwner)
+ Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) << DiagRange;
// We can't determine if the allocation outlives the local declaration.
return false;
}
@@ -7163,7 +7257,9 @@
break;
case IndirectLocalPathEntry::LifetimeBoundCall:
- // FIXME: Consider adding a note for this.
+ case IndirectLocalPathEntry::GslPointerInit:
+ case IndirectLocalPathEntry::GslOwnerTemporaryInit:
+ // 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
