Skip to content

Commit 75b4765

Browse files
yronglinyronglin
committed
[Analyzer][CFG] Correctly handle rebuilt default arg and default init expression
Signed-off-by: yronglin <[email protected]>
1 parent aa2d084 commit 75b4765

File tree

14 files changed

+229
-90
lines changed

14 files changed

+229
-90
lines changed

‎clang/include/clang/AST/ExprCXX.h

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1277,7 +1277,8 @@ class CXXDefaultArgExpr final
12771277
DeclContext *UsedContext;
12781278

12791279
CXXDefaultArgExpr(StmtClass SC, SourceLocation Loc, ParmVarDecl *Param,
1280-
Expr *RewrittenExpr, DeclContext *UsedContext)
1280+
DeclContext *UsedContext, Expr *RewrittenExpr,
1281+
bool HasRebuiltInit)
12811282
: Expr(SC,
12821283
Param->hasUnparsedDefaultArg()
12831284
? Param->getType().getNonReferenceType()
@@ -1287,25 +1288,27 @@ class CXXDefaultArgExpr final
12871288
Param(Param), UsedContext(UsedContext) {
12881289
CXXDefaultArgExprBits.Loc = Loc;
12891290
CXXDefaultArgExprBits.HasRewrittenInit = RewrittenExpr != nullptr;
1291+
CXXDefaultArgExprBits.HasRebuiltInit = HasRebuiltInit;
12901292
if (RewrittenExpr)
12911293
*getTrailingObjects<Expr *>() = RewrittenExpr;
12921294
setDependence(computeDependence(this));
12931295
}
12941296

1295-
CXXDefaultArgExpr(EmptyShell Empty, bool HasRewrittenInit)
1297+
CXXDefaultArgExpr(EmptyShell Empty, bool HasRewrittenInit, bool HasRebuiltInit)
12961298
: Expr(CXXDefaultArgExprClass, Empty) {
12971299
CXXDefaultArgExprBits.HasRewrittenInit = HasRewrittenInit;
1300+
CXXDefaultArgExprBits.HasRebuiltInit = HasRebuiltInit;
12981301
}
12991302

13001303
public:
13011304
static CXXDefaultArgExpr *CreateEmpty(const ASTContext &C,
1302-
bool HasRewrittenInit);
1305+
bool HasRewrittenInit, bool HasRebuiltInit);
13031306

13041307
// \p Param is the parameter whose default argument is used by this
13051308
// expression.
13061309
static CXXDefaultArgExpr *Create(const ASTContext &C, SourceLocation Loc,
1307-
ParmVarDecl *Param, Expr *RewrittenExpr,
1308-
DeclContext *UsedContext);
1310+
ParmVarDecl *Param, DeclContext *UsedContext,
1311+
Expr *RewrittenExpr, bool HasRebuiltInit);
13091312
// Retrieve the parameter that the argument was created from.
13101313
const ParmVarDecl *getParam() const { return Param; }
13111314
ParmVarDecl *getParam() { return Param; }
@@ -1314,6 +1317,10 @@ class CXXDefaultArgExpr final
13141317
return CXXDefaultArgExprBits.HasRewrittenInit;
13151318
}
13161319

1320+
bool hasRebuiltInit() const {
1321+
return CXXDefaultArgExprBits.HasRebuiltInit;
1322+
}
1323+
13171324
// Retrieve the argument to the function call.
13181325
Expr *getExpr();
13191326
const Expr *getExpr() const {
@@ -1385,26 +1392,31 @@ class CXXDefaultInitExpr final
13851392

13861393
CXXDefaultInitExpr(const ASTContext &Ctx, SourceLocation Loc,
13871394
FieldDecl *Field, QualType Ty, DeclContext *UsedContext,
1388-
Expr *RewrittenInitExpr);
1395+
Expr *RewrittenInitExpr, bool HasRebuiltInit);
13891396

1390-
CXXDefaultInitExpr(EmptyShell Empty, bool HasRewrittenInit)
1397+
CXXDefaultInitExpr(EmptyShell Empty, bool HasRewrittenInit, bool HasRebuiltInit)
13911398
: Expr(CXXDefaultInitExprClass, Empty) {
13921399
CXXDefaultInitExprBits.HasRewrittenInit = HasRewrittenInit;
1400+
CXXDefaultInitExprBits.HasRebuiltInit = HasRebuiltInit;
13931401
}
13941402

13951403
public:
13961404
static CXXDefaultInitExpr *CreateEmpty(const ASTContext &C,
1397-
bool HasRewrittenInit);
1405+
bool HasRewrittenInit, bool HasRebuiltInit);
13981406
/// \p Field is the non-static data member whose default initializer is used
13991407
/// by this expression.
14001408
static CXXDefaultInitExpr *Create(const ASTContext &Ctx, SourceLocation Loc,
14011409
FieldDecl *Field, DeclContext *UsedContext,
1402-
Expr *RewrittenInitExpr);
1410+
Expr *RewrittenInitExpr, bool HasRebuiltInit);
14031411

14041412
bool hasRewrittenInit() const {
14051413
return CXXDefaultInitExprBits.HasRewrittenInit;
14061414
}
14071415

1416+
bool hasRebuiltInit() const {
1417+
return CXXDefaultInitExprBits.HasRebuiltInit;
1418+
}
1419+
14081420
/// Get the field whose initializer will be used.
14091421
FieldDecl *getField() { return Field; }
14101422
const FieldDecl *getField() const { return Field; }

‎clang/include/clang/AST/Stmt.h

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -839,6 +839,10 @@ class alignas(void *) Stmt {
839839
LLVM_PREFERRED_TYPE(bool)
840840
unsigned HasRewrittenInit : 1;
841841

842+
/// Whether this CXXDefaultArgExpr rebuild its argument and stores a copy.
843+
LLVM_PREFERRED_TYPE(bool)
844+
unsigned HasRebuiltInit : 1;
845+
842846
/// The location where the default argument expression was used.
843847
SourceLocation Loc;
844848
};
@@ -850,11 +854,16 @@ class alignas(void *) Stmt {
850854
LLVM_PREFERRED_TYPE(ExprBitfields)
851855
unsigned : NumExprBits;
852856

853-
/// Whether this CXXDefaultInitExprBitfields rewrote its argument and stores
854-
/// a copy.
857+
/// Whether this CXXDefaultInitExpr rewrote its argument and stores
858+
/// a copy, unlike HasRebuiltInit, when this flag is true, the argument may
859+
/// be partial rebuilt.
855860
LLVM_PREFERRED_TYPE(bool)
856861
unsigned HasRewrittenInit : 1;
857862

863+
/// Whether this CXXDefaultInitExpr fully rebuild its argument and stores a copy.
864+
LLVM_PREFERRED_TYPE(bool)
865+
unsigned HasRebuiltInit : 1;
866+
858867
/// The location where the default initializer expression was used.
859868
SourceLocation Loc;
860869
};

‎clang/lib/AST/ASTImporter.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8154,8 +8154,8 @@ ExpectedStmt ASTNodeImporter::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *E) {
81548154
RewrittenInit = ExprOrErr.get();
81558155
}
81568156
return CXXDefaultArgExpr::Create(Importer.getToContext(), *ToUsedLocOrErr,
8157-
*ToParamOrErr, RewrittenInit,
8158-
*UsedContextOrErr);
8157+
*ToParamOrErr, *UsedContextOrErr,
8158+
RewrittenInit, E->hasRebuiltInit());
81598159
}
81608160

81618161
ExpectedStmt
@@ -8863,7 +8863,8 @@ ExpectedStmt ASTNodeImporter::VisitCXXDefaultInitExpr(CXXDefaultInitExpr *E) {
88638863
}
88648864

88658865
return CXXDefaultInitExpr::Create(Importer.getToContext(), *ToBeginLocOrErr,
8866-
ToField, *UsedContextOrErr, RewrittenInit);
8866+
ToField, *UsedContextOrErr, RewrittenInit,
8867+
E->hasRebuiltInit());
88678868
}
88688869

88698870
ExpectedStmt ASTNodeImporter::VisitCXXNamedCastExpr(CXXNamedCastExpr *E) {

‎clang/lib/AST/ExprCXX.cpp

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1009,21 +1009,23 @@ const IdentifierInfo *UserDefinedLiteral::getUDSuffix() const {
10091009
}
10101010

10111011
CXXDefaultArgExpr *CXXDefaultArgExpr::CreateEmpty(const ASTContext &C,
1012-
bool HasRewrittenInit) {
1012+
bool HasRewrittenInit,
1013+
bool HasRebuiltInit) {
10131014
size_t Size = totalSizeToAlloc<Expr *>(HasRewrittenInit);
10141015
auto *Mem = C.Allocate(Size, alignof(CXXDefaultArgExpr));
1015-
return new (Mem) CXXDefaultArgExpr(EmptyShell(), HasRewrittenInit);
1016+
return new (Mem)
1017+
CXXDefaultArgExpr(EmptyShell(), HasRewrittenInit, HasRebuiltInit);
10161018
}
10171019

1018-
CXXDefaultArgExpr *CXXDefaultArgExpr::Create(const ASTContext &C,
1019-
SourceLocation Loc,
1020-
ParmVarDecl *Param,
1021-
Expr *RewrittenExpr,
1022-
DeclContext *UsedContext) {
1020+
CXXDefaultArgExpr *
1021+
CXXDefaultArgExpr::Create(const ASTContext &C, SourceLocation Loc,
1022+
ParmVarDecl *Param, DeclContext *UsedContext,
1023+
Expr *RewrittenExpr, bool HasRebuiltInit) {
10231024
size_t Size = totalSizeToAlloc<Expr *>(RewrittenExpr != nullptr);
10241025
auto *Mem = C.Allocate(Size, alignof(CXXDefaultArgExpr));
1025-
return new (Mem) CXXDefaultArgExpr(CXXDefaultArgExprClass, Loc, Param,
1026-
RewrittenExpr, UsedContext);
1026+
return new (Mem)
1027+
CXXDefaultArgExpr(CXXDefaultArgExprClass, Loc, Param, UsedContext,
1028+
RewrittenExpr, HasRebuiltInit);
10271029
}
10281030

10291031
Expr *CXXDefaultArgExpr::getExpr() {
@@ -1044,7 +1046,7 @@ Expr *CXXDefaultArgExpr::getAdjustedRewrittenExpr() {
10441046
CXXDefaultInitExpr::CXXDefaultInitExpr(const ASTContext &Ctx,
10451047
SourceLocation Loc, FieldDecl *Field,
10461048
QualType Ty, DeclContext *UsedContext,
1047-
Expr *RewrittenInitExpr)
1049+
Expr *RewrittenInitExpr, bool hasRebuiltInit)
10481050
: Expr(CXXDefaultInitExprClass, Ty.getNonLValueExprType(Ctx),
10491051
Ty->isLValueReferenceType() ? VK_LValue
10501052
: Ty->isRValueReferenceType() ? VK_XValue
@@ -1053,6 +1055,7 @@ CXXDefaultInitExpr::CXXDefaultInitExpr(const ASTContext &Ctx,
10531055
Field(Field), UsedContext(UsedContext) {
10541056
CXXDefaultInitExprBits.Loc = Loc;
10551057
CXXDefaultInitExprBits.HasRewrittenInit = RewrittenInitExpr != nullptr;
1058+
CXXDefaultInitExprBits.HasRebuiltInit = hasRebuiltInit;
10561059

10571060
if (CXXDefaultInitExprBits.HasRewrittenInit)
10581061
*getTrailingObjects<Expr *>() = RewrittenInitExpr;
@@ -1063,22 +1066,24 @@ CXXDefaultInitExpr::CXXDefaultInitExpr(const ASTContext &Ctx,
10631066
}
10641067

10651068
CXXDefaultInitExpr *CXXDefaultInitExpr::CreateEmpty(const ASTContext &C,
1066-
bool HasRewrittenInit) {
1069+
bool HasRewrittenInit,
1070+
bool HasRebuiltInit) {
10671071
size_t Size = totalSizeToAlloc<Expr *>(HasRewrittenInit);
10681072
auto *Mem = C.Allocate(Size, alignof(CXXDefaultInitExpr));
1069-
return new (Mem) CXXDefaultInitExpr(EmptyShell(), HasRewrittenInit);
1073+
return new (Mem)
1074+
CXXDefaultInitExpr(EmptyShell(), HasRewrittenInit, HasRebuiltInit);
10701075
}
10711076

1072-
CXXDefaultInitExpr *CXXDefaultInitExpr::Create(const ASTContext &Ctx,
1073-
SourceLocation Loc,
1074-
FieldDecl *Field,
1075-
DeclContext *UsedContext,
1076-
Expr *RewrittenInitExpr) {
1077+
CXXDefaultInitExpr *
1078+
CXXDefaultInitExpr::Create(const ASTContext &Ctx, SourceLocation Loc,
1079+
FieldDecl *Field, DeclContext *UsedContext,
1080+
Expr *RewrittenInitExpr, bool HasRebuiltInit) {
10771081

10781082
size_t Size = totalSizeToAlloc<Expr *>(RewrittenInitExpr != nullptr);
10791083
auto *Mem = Ctx.Allocate(Size, alignof(CXXDefaultInitExpr));
1080-
return new (Mem) CXXDefaultInitExpr(Ctx, Loc, Field, Field->getType(),
1081-
UsedContext, RewrittenInitExpr);
1084+
return new (Mem)
1085+
CXXDefaultInitExpr(Ctx, Loc, Field, Field->getType(), UsedContext,
1086+
RewrittenInitExpr, HasRebuiltInit);
10821087
}
10831088

10841089
Expr *CXXDefaultInitExpr::getExpr() {

‎clang/lib/AST/ParentMap.cpp

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include "clang/AST/ParentMap.h"
1414
#include "clang/AST/Decl.h"
1515
#include "clang/AST/Expr.h"
16+
#include "clang/AST/ExprCXX.h"
1617
#include "clang/AST/StmtObjC.h"
1718
#include "llvm/ADT/DenseMap.h"
1819

@@ -96,6 +97,22 @@ static void BuildParentMap(MapTy& M, Stmt* S,
9697
BuildParentMap(M, SubStmt, OVMode);
9798
}
9899
break;
100+
case Stmt::CXXDefaultArgExprClass:
101+
if (auto *Arg = dyn_cast<CXXDefaultArgExpr>(S)) {
102+
if (Arg->hasRebuiltInit()) {
103+
M[Arg->getExpr()] = S;
104+
BuildParentMap(M, Arg->getExpr(), OVMode);
105+
}
106+
}
107+
break;
108+
case Stmt::CXXDefaultInitExprClass:
109+
if (auto *Init = dyn_cast<CXXDefaultInitExpr>(S)) {
110+
if (Init->hasRebuiltInit()) {
111+
M[Init->getExpr()] = S;
112+
BuildParentMap(M, Init->getExpr(), OVMode);
113+
}
114+
}
115+
break;
99116
default:
100117
for (Stmt *SubStmt : S->children()) {
101118
if (SubStmt) {

‎clang/lib/Analysis/CFG.cpp

Lines changed: 41 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -556,6 +556,10 @@ class CFGBuilder {
556556

557557
private:
558558
// Visitors to walk an AST and construct the CFG.
559+
CFGBlock *VisitCXXDefaultArgExpr(CXXDefaultArgExpr *Default,
560+
AddStmtChoice asc);
561+
CFGBlock *VisitCXXDefaultInitExpr(CXXDefaultInitExpr *Default,
562+
AddStmtChoice asc);
559563
CFGBlock *VisitInitListExpr(InitListExpr *ILE, AddStmtChoice asc);
560564
CFGBlock *VisitAddrLabelExpr(AddrLabelExpr *A, AddStmtChoice asc);
561565
CFGBlock *VisitAttributedStmt(AttributedStmt *A, AddStmtChoice asc);
@@ -2261,16 +2265,10 @@ CFGBlock *CFGBuilder::Visit(Stmt * S, AddStmtChoice asc,
22612265
asc, ExternallyDestructed);
22622266

22632267
case Stmt::CXXDefaultArgExprClass:
2268+
return VisitCXXDefaultArgExpr(cast<CXXDefaultArgExpr>(S), asc);
2269+
22642270
case Stmt::CXXDefaultInitExprClass:
2265-
// FIXME: The expression inside a CXXDefaultArgExpr is owned by the
2266-
// called function's declaration, not by the caller. If we simply add
2267-
// this expression to the CFG, we could end up with the same Expr
2268-
// appearing multiple times (PR13385).
2269-
//
2270-
// It's likewise possible for multiple CXXDefaultInitExprs for the same
2271-
// expression to be used in the same function (through aggregate
2272-
// initialization).
2273-
return VisitStmt(S, asc);
2271+
return VisitCXXDefaultInitExpr(cast<CXXDefaultInitExpr>(S), asc);
22742272

22752273
case Stmt::CXXBindTemporaryExprClass:
22762274
return VisitCXXBindTemporaryExpr(cast<CXXBindTemporaryExpr>(S), asc);
@@ -2440,6 +2438,40 @@ CFGBlock *CFGBuilder::VisitChildren(Stmt *S) {
24402438
return B;
24412439
}
24422440

2441+
CFGBlock *CFGBuilder::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *Arg,
2442+
AddStmtChoice asc) {
2443+
if (Arg->hasRebuiltInit()) {
2444+
if (asc.alwaysAdd(*this, Arg)) {
2445+
autoCreateBlock();
2446+
appendStmt(Block, Arg);
2447+
}
2448+
return VisitStmt(Arg->getExpr(), asc);
2449+
}
2450+
2451+
// We can't add the default argument if it's not rewritten because the
2452+
// expression inside a CXXDefaultArgExpr is owned by the called function's
2453+
// declaration, not by the caller, we could end up with the same expression
2454+
// appearing multiple times.
2455+
return VisitStmt(Arg, asc);
2456+
}
2457+
2458+
CFGBlock *CFGBuilder::VisitCXXDefaultInitExpr(CXXDefaultInitExpr *Init,
2459+
AddStmtChoice asc) {
2460+
if (Init->hasRebuiltInit()) {
2461+
if (asc.alwaysAdd(*this, Init)) {
2462+
autoCreateBlock();
2463+
appendStmt(Block, Init);
2464+
}
2465+
return VisitStmt(Init->getExpr(), asc);
2466+
}
2467+
2468+
// We can't add the default initializer if it's not rewritten because multiple
2469+
// CXXDefaultInitExprs for the same sub-expression to be used in the same
2470+
// function (through aggregate initialization). we could end up with the same
2471+
// expression appearing multiple times.
2472+
return VisitStmt(Init, asc);
2473+
}
2474+
24432475
CFGBlock *CFGBuilder::VisitInitListExpr(InitListExpr *ILE, AddStmtChoice asc) {
24442476
if (asc.alwaysAdd(*this, ILE)) {
24452477
autoCreateBlock();

‎clang/lib/Analysis/ReachableCode.cpp

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -454,11 +454,10 @@ bool DeadCodeScan::isDeadCodeRoot(const clang::CFGBlock *Block) {
454454
return isDeadRoot;
455455
}
456456

457-
// Check if the given `DeadStmt` is a coroutine statement and is a substmt of
458-
// the coroutine statement. `Block` is the CFGBlock containing the `DeadStmt`.
459-
static bool isInCoroutineStmt(const Stmt *DeadStmt, const CFGBlock *Block) {
457+
template <class... Ts>
458+
static bool isDeadStmtIn(const Stmt *DeadStmt, const CFGBlock *Block) {
460459
// The coroutine statement, co_return, co_await, or co_yield.
461-
const Stmt *CoroStmt = nullptr;
460+
const Stmt *TargetStmt = nullptr;
462461
// Find the first coroutine statement after the DeadStmt in the block.
463462
bool AfterDeadStmt = false;
464463
for (CFGBlock::const_iterator I = Block->begin(), E = Block->end(); I != E;
@@ -467,32 +466,29 @@ static bool isInCoroutineStmt(const Stmt *DeadStmt, const CFGBlock *Block) {
467466
const Stmt *S = CS->getStmt();
468467
if (S == DeadStmt)
469468
AfterDeadStmt = true;
470-
if (AfterDeadStmt &&
471-
// For simplicity, we only check simple coroutine statements.
472-
(llvm::isa<CoreturnStmt>(S) || llvm::isa<CoroutineSuspendExpr>(S))) {
473-
CoroStmt = S;
469+
if (AfterDeadStmt && llvm::isa<Ts...>(S)) {
470+
TargetStmt = S;
474471
break;
475472
}
476473
}
477-
if (!CoroStmt)
474+
if (!TargetStmt)
478475
return false;
479476
struct Checker : DynamicRecursiveASTVisitor {
480477
const Stmt *DeadStmt;
481-
bool CoroutineSubStmt = false;
478+
bool IsSubStmtOfTargetStmt = false;
482479
Checker(const Stmt *S) : DeadStmt(S) {
483-
// Statements captured in the CFG can be implicit.
484480
ShouldVisitImplicitCode = true;
485481
}
486482

487483
bool VisitStmt(Stmt *S) override {
488484
if (S == DeadStmt)
489-
CoroutineSubStmt = true;
485+
IsSubStmtOfTargetStmt = true;
490486
return true;
491487
}
492488
};
493489
Checker checker(DeadStmt);
494-
checker.TraverseStmt(const_cast<Stmt *>(CoroStmt));
495-
return checker.CoroutineSubStmt;
490+
checker.TraverseStmt(const_cast<Stmt *>(TargetStmt));
491+
return checker.IsSubStmtOfTargetStmt;
496492
}
497493

498494
static bool isValidDeadStmt(const Stmt *S, const clang::CFGBlock *Block) {
@@ -503,7 +499,12 @@ static bool isValidDeadStmt(const Stmt *S, const clang::CFGBlock *Block) {
503499
// Coroutine statements are never considered dead statements, because removing
504500
// them may change the function semantic if it is the only coroutine statement
505501
// of the coroutine.
506-
return !isInCoroutineStmt(S, Block);
502+
//
503+
// If the dead stmt is a sub-stmt of CXXDefaultInitExpr and CXXDefaultArgExpr,
504+
// we would rather expect to find CXXDefaultInitExpr and CXXDefaultArgExpr as
505+
// a valid dead stmt.
506+
return !isDeadStmtIn<CoreturnStmt, CoroutineSuspendExpr, CXXDefaultArgExpr,
507+
CXXDefaultInitExpr>(S, Block);
507508
}
508509

509510
const Stmt *DeadCodeScan::findDeadCode(const clang::CFGBlock *Block) {

0 commit comments

Comments
 (0)