Skip to content

Commit

Permalink
Reland "[clang] Merge lifetimebound and GSL code paths for lifetime a…
Browse files Browse the repository at this point in the history 
…nalysis (#104906)" (#105838)

Reland without the `EnableLifetimeWarnings` removal. I will remove the
EnableLifetimeWarnings in a follow-up patch.

I have added a test to prevent regression.
  • Loading branch information
@hokein
hokein authored Aug 23, 2024
1 parent a9f6224 commit b1560bd
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 72 deletions.
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,8 @@ Improvements to Clang's diagnostics

- Clang now diagnoses when the result of a [[nodiscard]] function is discarded after being cast in C. Fixes #GH104391.

- Don't emit duplicated dangling diagnostics. (#GH93386).

- Improved diagnostic when trying to befriend a concept. (#GH45182).

Improvements to Clang's time-trace
Expand Down
127 changes: 55 additions & 72 deletions clang/lib/Sema/CheckExprLifetime.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,66 +326,6 @@ static bool shouldTrackFirstArgument(const FunctionDecl *FD) {
return false;
}

static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr *Call,
LocalVisitor Visit) {
auto VisitPointerArg = [&](const Decl *D, Expr *Arg, bool Value) {
// We are not interested in the temporary base objects of gsl Pointers:
// Temp().ptr; // Here ptr might not dangle.
if (isa<MemberExpr>(Arg->IgnoreImpCasts()))
return;
// Once we initialized a value with a reference, it can no longer dangle.
if (!Value) {
for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) {
if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit)
continue;
if (PE.Kind == IndirectLocalPathEntry::GslPointerInit ||
PE.Kind == IndirectLocalPathEntry::GslPointerAssignment)
return;
break;
}
}
Path.push_back({Value ? IndirectLocalPathEntry::GslPointerInit
: IndirectLocalPathEntry::GslReferenceInit,
Arg, D});
if (Arg->isGLValue())
visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
Visit,
/*EnableLifetimeWarnings=*/true);
else
visitLocalsRetainedByInitializer(Path, Arg, Visit, true,
/*EnableLifetimeWarnings=*/true);
Path.pop_back();
};

if (auto *MCE = dyn_cast<CXXMemberCallExpr>(Call)) {
const auto *MD = cast_or_null<CXXMethodDecl>(MCE->getDirectCallee());
if (MD && shouldTrackImplicitObjectArg(MD))
VisitPointerArg(MD, MCE->getImplicitObjectArgument(),
!MD->getReturnType()->isReferenceType());
return;
} else if (auto *OCE = dyn_cast<CXXOperatorCallExpr>(Call)) {
FunctionDecl *Callee = OCE->getDirectCallee();
if (Callee && Callee->isCXXInstanceMember() &&
shouldTrackImplicitObjectArg(cast<CXXMethodDecl>(Callee)))
VisitPointerArg(Callee, OCE->getArg(0),
!Callee->getReturnType()->isReferenceType());
return;
} else if (auto *CE = dyn_cast<CallExpr>(Call)) {
FunctionDecl *Callee = CE->getDirectCallee();
if (Callee && shouldTrackFirstArgument(Callee))
VisitPointerArg(Callee, CE->getArg(0),
!Callee->getReturnType()->isReferenceType());
return;
}

if (auto *CCE = dyn_cast<CXXConstructExpr>(Call)) {
const auto *Ctor = CCE->getConstructor();
const CXXRecordDecl *RD = Ctor->getParent();
if (CCE->getNumArgs() > 0 && RD->hasAttr<PointerAttr>())
VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0], true);
}
}

static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) {
const TypeSourceInfo *TSI = FD->getTypeSourceInfo();
if (!TSI)
Expand Down Expand Up @@ -423,8 +363,10 @@ static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) {
return false;
}

static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
LocalVisitor Visit) {
// Visit lifetimebound or gsl-pointer arguments.
static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
LocalVisitor Visit,
bool EnableLifetimeWarnings) {
const FunctionDecl *Callee;
ArrayRef<Expr *> Args;

Expand Down Expand Up @@ -458,6 +400,34 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
/*EnableLifetimeWarnings=*/false);
Path.pop_back();
};
auto VisitGSLPointerArg = [&](const Decl *D, Expr *Arg, bool Value) {
// We are not interested in the temporary base objects of gsl Pointers:
// Temp().ptr; // Here ptr might not dangle.
if (isa<MemberExpr>(Arg->IgnoreImpCasts()))
return;
// Once we initialized a value with a reference, it can no longer dangle.
if (!Value) {
for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) {
if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit)
continue;
if (PE.Kind == IndirectLocalPathEntry::GslPointerInit ||
PE.Kind == IndirectLocalPathEntry::GslPointerAssignment)
return;
break;
}
}
Path.push_back({Value ? IndirectLocalPathEntry::GslPointerInit
: IndirectLocalPathEntry::GslReferenceInit,
Arg, D});
if (Arg->isGLValue())
visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
Visit,
/*EnableLifetimeWarnings=*/true);
else
visitLocalsRetainedByInitializer(Path, Arg, Visit, true,
/*EnableLifetimeWarnings=*/true);
Path.pop_back();
};

bool CheckCoroCall = false;
if (const auto *RD = Callee->getReturnType()->getAsRecordDecl()) {
Expand All @@ -478,13 +448,30 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
CheckCoroObjArg = false;
if (implicitObjectParamIsLifetimeBound(Callee) || CheckCoroObjArg)
VisitLifetimeBoundArg(Callee, ObjectArg);
else if (EnableLifetimeWarnings) {
if (auto *CME = dyn_cast<CXXMethodDecl>(Callee);
CME && shouldTrackImplicitObjectArg(CME))
VisitGSLPointerArg(Callee, ObjectArg,
!Callee->getReturnType()->isReferenceType());
}
}

for (unsigned I = 0,
N = std::min<unsigned>(Callee->getNumParams(), Args.size());
I != N; ++I) {
if (CheckCoroCall || Callee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>())
VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]);
else if (EnableLifetimeWarnings && I == 0) {
if (shouldTrackFirstArgument(Callee)) {
VisitGSLPointerArg(Callee, Args[0],
!Callee->getReturnType()->isReferenceType());
} else {
if (auto *CCE = dyn_cast<CXXConstructExpr>(Call);
CCE && CCE->getConstructor()->getParent()->hasAttr<PointerAttr>())
VisitGSLPointerArg(CCE->getConstructor()->getParamDecl(0), Args[0],
true);
}
}
}
}

Expand Down Expand Up @@ -557,11 +544,9 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
EnableLifetimeWarnings);
}

if (isa<CallExpr>(Init)) {
if (EnableLifetimeWarnings)
handleGslAnnotatedTypes(Path, Init, Visit);
return visitLifetimeBoundArguments(Path, Init, Visit);
}
if (isa<CallExpr>(Init))
return visitFunctionCallArguments(Path, Init, Visit,
EnableLifetimeWarnings);

switch (Init->getStmtClass()) {
case Stmt::DeclRefExprClass: {
Expand Down Expand Up @@ -835,11 +820,9 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
}
}

if (isa<CallExpr>(Init) || isa<CXXConstructExpr>(Init)) {
if (EnableLifetimeWarnings)
handleGslAnnotatedTypes(Path, Init, Visit);
return visitLifetimeBoundArguments(Path, Init, Visit);
}
if (isa<CallExpr>(Init) || isa<CXXConstructExpr>(Init))
return visitFunctionCallArguments(Path, Init, Visit,
EnableLifetimeWarnings);

switch (Init->getStmtClass()) {
case Stmt::UnaryOperatorClass: {
Expand Down
20 changes: 20 additions & 0 deletions clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -479,3 +479,23 @@ void testForBug49342()
{
auto it = std::iter<char>{} - 2; // Used to be false positive.
}

namespace GH93386 {
// verify no duplicated diagnostics are emitted.
struct [[gsl::Pointer]] S {
S(const std::vector<int>& abc [[clang::lifetimebound]]);
};

S test(std::vector<int> a) {
return S(a); // expected-warning {{address of stack memory associated with}}
}

auto s = S(std::vector<int>()); // expected-warning {{temporary whose address is used as value of local variable}}

// Verify no regression on the follow case.
std::string_view test2(int i, std::optional<std::string_view> a) {
if (i)
return std::move(*a);
return std::move(a.value());
}
}

0 comments on commit b1560bd

Please sign in to comment.