Skip to content

Commit 0ecb70e

Browse files
hokeincjdb
hokein
authored and
cjdb
committed
Reland "[clang] Merge lifetimebound and GSL code paths for lifetime analysis (llvm#104906)" (llvm#105838)
Reland without the `EnableLifetimeWarnings` removal. I will remove the EnableLifetimeWarnings in a follow-up patch. I have added a test to prevent regression.
1 parent 935382e commit 0ecb70e

File tree

3 files changed

+77
-72
lines changed

3 files changed

+77
-72
lines changed

‎clang/docs/ReleaseNotes.rst‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,8 @@ Improvements to Clang's diagnostics
234234

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

237+
- Don't emit duplicated dangling diagnostics. (#GH93386).
238+
237239
- Improved diagnostic when trying to befriend a concept. (#GH45182).
238240

239241
Improvements to Clang's time-trace

‎clang/lib/Sema/CheckExprLifetime.cpp‎

Lines changed: 55 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -326,66 +326,6 @@ static bool shouldTrackFirstArgument(const FunctionDecl *FD) {
326326
return false;
327327
}
328328

329-
static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr *Call,
330-
LocalVisitor Visit) {
331-
auto VisitPointerArg = [&](const Decl *D, Expr *Arg, bool Value) {
332-
// We are not interested in the temporary base objects of gsl Pointers:
333-
// Temp().ptr; // Here ptr might not dangle.
334-
if (isa<MemberExpr>(Arg->IgnoreImpCasts()))
335-
return;
336-
// Once we initialized a value with a reference, it can no longer dangle.
337-
if (!Value) {
338-
for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) {
339-
if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit)
340-
continue;
341-
if (PE.Kind == IndirectLocalPathEntry::GslPointerInit ||
342-
PE.Kind == IndirectLocalPathEntry::GslPointerAssignment)
343-
return;
344-
break;
345-
}
346-
}
347-
Path.push_back({Value ? IndirectLocalPathEntry::GslPointerInit
348-
: IndirectLocalPathEntry::GslReferenceInit,
349-
Arg, D});
350-
if (Arg->isGLValue())
351-
visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
352-
Visit,
353-
/*EnableLifetimeWarnings=*/true);
354-
else
355-
visitLocalsRetainedByInitializer(Path, Arg, Visit, true,
356-
/*EnableLifetimeWarnings=*/true);
357-
Path.pop_back();
358-
};
359-
360-
if (auto *MCE = dyn_cast<CXXMemberCallExpr>(Call)) {
361-
const auto *MD = cast_or_null<CXXMethodDecl>(MCE->getDirectCallee());
362-
if (MD && shouldTrackImplicitObjectArg(MD))
363-
VisitPointerArg(MD, MCE->getImplicitObjectArgument(),
364-
!MD->getReturnType()->isReferenceType());
365-
return;
366-
} else if (auto *OCE = dyn_cast<CXXOperatorCallExpr>(Call)) {
367-
FunctionDecl *Callee = OCE->getDirectCallee();
368-
if (Callee && Callee->isCXXInstanceMember() &&
369-
shouldTrackImplicitObjectArg(cast<CXXMethodDecl>(Callee)))
370-
VisitPointerArg(Callee, OCE->getArg(0),
371-
!Callee->getReturnType()->isReferenceType());
372-
return;
373-
} else if (auto *CE = dyn_cast<CallExpr>(Call)) {
374-
FunctionDecl *Callee = CE->getDirectCallee();
375-
if (Callee && shouldTrackFirstArgument(Callee))
376-
VisitPointerArg(Callee, CE->getArg(0),
377-
!Callee->getReturnType()->isReferenceType());
378-
return;
379-
}
380-
381-
if (auto *CCE = dyn_cast<CXXConstructExpr>(Call)) {
382-
const auto *Ctor = CCE->getConstructor();
383-
const CXXRecordDecl *RD = Ctor->getParent();
384-
if (CCE->getNumArgs() > 0 && RD->hasAttr<PointerAttr>())
385-
VisitPointerArg(Ctor->getParamDecl(0), CCE->getArgs()[0], true);
386-
}
387-
}
388-
389329
static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) {
390330
const TypeSourceInfo *TSI = FD->getTypeSourceInfo();
391331
if (!TSI)
@@ -423,8 +363,10 @@ static bool implicitObjectParamIsLifetimeBound(const FunctionDecl *FD) {
423363
return false;
424364
}
425365

426-
static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
427-
LocalVisitor Visit) {
366+
// Visit lifetimebound or gsl-pointer arguments.
367+
static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
368+
LocalVisitor Visit,
369+
bool EnableLifetimeWarnings) {
428370
const FunctionDecl *Callee;
429371
ArrayRef<Expr *> Args;
430372

@@ -458,6 +400,34 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
458400
/*EnableLifetimeWarnings=*/false);
459401
Path.pop_back();
460402
};
403+
auto VisitGSLPointerArg = [&](const Decl *D, Expr *Arg, bool Value) {
404+
// We are not interested in the temporary base objects of gsl Pointers:
405+
// Temp().ptr; // Here ptr might not dangle.
406+
if (isa<MemberExpr>(Arg->IgnoreImpCasts()))
407+
return;
408+
// Once we initialized a value with a reference, it can no longer dangle.
409+
if (!Value) {
410+
for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) {
411+
if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit)
412+
continue;
413+
if (PE.Kind == IndirectLocalPathEntry::GslPointerInit ||
414+
PE.Kind == IndirectLocalPathEntry::GslPointerAssignment)
415+
return;
416+
break;
417+
}
418+
}
419+
Path.push_back({Value ? IndirectLocalPathEntry::GslPointerInit
420+
: IndirectLocalPathEntry::GslReferenceInit,
421+
Arg, D});
422+
if (Arg->isGLValue())
423+
visitLocalsRetainedByReferenceBinding(Path, Arg, RK_ReferenceBinding,
424+
Visit,
425+
/*EnableLifetimeWarnings=*/true);
426+
else
427+
visitLocalsRetainedByInitializer(Path, Arg, Visit, true,
428+
/*EnableLifetimeWarnings=*/true);
429+
Path.pop_back();
430+
};
461431

462432
bool CheckCoroCall = false;
463433
if (const auto *RD = Callee->getReturnType()->getAsRecordDecl()) {
@@ -478,13 +448,30 @@ static void visitLifetimeBoundArguments(IndirectLocalPath &Path, Expr *Call,
478448
CheckCoroObjArg = false;
479449
if (implicitObjectParamIsLifetimeBound(Callee) || CheckCoroObjArg)
480450
VisitLifetimeBoundArg(Callee, ObjectArg);
451+
else if (EnableLifetimeWarnings) {
452+
if (auto *CME = dyn_cast<CXXMethodDecl>(Callee);
453+
CME && shouldTrackImplicitObjectArg(CME))
454+
VisitGSLPointerArg(Callee, ObjectArg,
455+
!Callee->getReturnType()->isReferenceType());
456+
}
481457
}
482458

483459
for (unsigned I = 0,
484460
N = std::min<unsigned>(Callee->getNumParams(), Args.size());
485461
I != N; ++I) {
486462
if (CheckCoroCall || Callee->getParamDecl(I)->hasAttr<LifetimeBoundAttr>())
487463
VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]);
464+
else if (EnableLifetimeWarnings && I == 0) {
465+
if (shouldTrackFirstArgument(Callee)) {
466+
VisitGSLPointerArg(Callee, Args[0],
467+
!Callee->getReturnType()->isReferenceType());
468+
} else {
469+
if (auto *CCE = dyn_cast<CXXConstructExpr>(Call);
470+
CCE && CCE->getConstructor()->getParent()->hasAttr<PointerAttr>())
471+
VisitGSLPointerArg(CCE->getConstructor()->getParamDecl(0), Args[0],
472+
true);
473+
}
474+
}
488475
}
489476
}
490477

@@ -557,11 +544,9 @@ static void visitLocalsRetainedByReferenceBinding(IndirectLocalPath &Path,
557544
EnableLifetimeWarnings);
558545
}
559546

560-
if (isa<CallExpr>(Init)) {
561-
if (EnableLifetimeWarnings)
562-
handleGslAnnotatedTypes(Path, Init, Visit);
563-
return visitLifetimeBoundArguments(Path, Init, Visit);
564-
}
547+
if (isa<CallExpr>(Init))
548+
return visitFunctionCallArguments(Path, Init, Visit,
549+
EnableLifetimeWarnings);
565550

566551
switch (Init->getStmtClass()) {
567552
case Stmt::DeclRefExprClass: {
@@ -835,11 +820,9 @@ static void visitLocalsRetainedByInitializer(IndirectLocalPath &Path,
835820
}
836821
}
837822

838-
if (isa<CallExpr>(Init) || isa<CXXConstructExpr>(Init)) {
839-
if (EnableLifetimeWarnings)
840-
handleGslAnnotatedTypes(Path, Init, Visit);
841-
return visitLifetimeBoundArguments(Path, Init, Visit);
842-
}
823+
if (isa<CallExpr>(Init) || isa<CXXConstructExpr>(Init))
824+
return visitFunctionCallArguments(Path, Init, Visit,
825+
EnableLifetimeWarnings);
843826

844827
switch (Init->getStmtClass()) {
845828
case Stmt::UnaryOperatorClass: {

‎clang/test/Sema/warn-lifetime-analysis-nocfg.cpp‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -479,3 +479,23 @@ void testForBug49342()
479479
{
480480
auto it = std::iter<char>{} - 2; // Used to be false positive.
481481
}
482+
483+
namespace GH93386 {
484+
// verify no duplicated diagnostics are emitted.
485+
struct [[gsl::Pointer]] S {
486+
S(const std::vector<int>& abc [[clang::lifetimebound]]);
487+
};
488+
489+
S test(std::vector<int> a) {
490+
return S(a); // expected-warning {{address of stack memory associated with}}
491+
}
492+
493+
auto s = S(std::vector<int>()); // expected-warning {{temporary whose address is used as value of local variable}}
494+
495+
// Verify no regression on the follow case.
496+
std::string_view test2(int i, std::optional<std::string_view> a) {
497+
if (i)
498+
return std::move(*a);
499+
return std::move(a.value());
500+
}
501+
}

0 commit comments

Comments
 (0)