Hi rnk,
This patch fixes issues with unused result detection which were found in patch
http://reviews.llvm.org/D9743.
For detection of unused result the method CodeGenFunction::EmitCall uses
similar approach to Sema::DiagnoseUnusedExprResult.
Sema::DiagnoseUnusedExprResult uses underlying method
Expr::isUnusedResultAWarning(const Expr *&WarnExpr, SourceLocation &Loc,
SourceRange &R1, SourceRange &R2, ASTContext &Ctx) for accurate detection. This
method has few drawbacks for using outside of Sema class:
* it checks warn_unused_result attribute which in general case is not set;
* it modifies several parameters which are not required for unused result
checking.
In general case, we want to see smth similar to this method but without
warn_unused_attribute checking and without modifiable parameters. The most
expected solution in this case may be to provide Expr::isUnusedResult method
which the method isUnusedResultAWarning method should use. However, this can't
be easily done because in this case all context information for diagnostic
should be detected repeatedly inside isUnusedResultAWarning method. Same for
warn_unused_attribute. Another solution might be to duplicate logic of
isUnusedResultAWarning inside inUnusedResult, but this duplication is too big
(few hundred LOC). Moreover, all changes inside isUnusedResultAWarning should
be duplicated inside isUnusedResult too. So this doesn't look acceptable.
This patch provides solution which uses isUnusedResultAWarning as an underlying
method for isUnusedResult. It extends the list of parameters of
isUnusedResultAWarning with extra parameter alwaysWarnIfUnused (default value
is false). Based on the isUnusedResult method patch detects unused result
during the code generation.
REPOSITORY
rL LLVM
http://reviews.llvm.org/D10042
Files:
include/clang/AST/Expr.h
lib/AST/Expr.cpp
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGStmt.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGenCXX/stack-reuse.cpp
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
Index: include/clang/AST/Expr.h
===================================================================
--- include/clang/AST/Expr.h
+++ include/clang/AST/Expr.h
@@ -232,7 +232,12 @@
/// for a warning.
bool isUnusedResultAWarning(const Expr *&WarnExpr, SourceLocation &Loc,
SourceRange &R1, SourceRange &R2,
- ASTContext &Ctx) const;
+ ASTContext &Ctx, bool alwaysWarnIfUnused = false)
+ const;
+
+ /// isUnusedResult - Return true if this immediate expression has the unused
+ /// result.
+ bool isUnusedResult(ASTContext &Ctx) const;
/// isLValue - True if this expression is an "l-value" according to
/// the rules of the current language. C and C++ give somewhat
Index: lib/AST/Expr.cpp
===================================================================
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -2009,9 +2009,10 @@
/// be warned about if the result is unused. If so, fill in Loc and Ranges
/// with location to warn on and the source range[s] to report with the
/// warning.
-bool Expr::isUnusedResultAWarning(const Expr *&WarnE, SourceLocation &Loc,
+bool Expr::isUnusedResultAWarning(const Expr *&WarnE, SourceLocation &Loc,
SourceRange &R1, SourceRange &R2,
- ASTContext &Ctx) const {
+ ASTContext &Ctx,
+ bool alwaysWarnIfUnused) const {
// Don't warn if the expr is type dependent. The type could end up
// instantiating to void.
if (isTypeDependent())
@@ -2026,14 +2027,15 @@
R1 = getSourceRange();
return true;
case ParenExprClass:
- return cast<ParenExpr>(this)->getSubExpr()->
- isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+ return cast<ParenExpr>(this)->getSubExpr()->isUnusedResultAWarning(
+ WarnE, Loc, R1, R2, Ctx, alwaysWarnIfUnused);
case GenericSelectionExprClass:
- return cast<GenericSelectionExpr>(this)->getResultExpr()->
- isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+ return cast<GenericSelectionExpr>(this)
+ ->getResultExpr()
+ ->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx, alwaysWarnIfUnused);
case ChooseExprClass:
- return cast<ChooseExpr>(this)->getChosenSubExpr()->
- isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+ return cast<ChooseExpr>(this)->getChosenSubExpr()->isUnusedResultAWarning(
+ WarnE, Loc, R1, R2, Ctx, alwaysWarnIfUnused);
case UnaryOperatorClass: {
const UnaryOperator *UO = cast<UnaryOperator>(this);
@@ -2058,7 +2060,8 @@
return false;
break;
case UO_Extension:
- return UO->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+ return UO->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+ alwaysWarnIfUnused);
}
WarnE = this;
Loc = UO->getOperatorLoc();
@@ -2079,12 +2082,15 @@
dyn_cast<IntegerLiteral>(BO->getRHS()->IgnoreParens()))
if (IE->getValue() == 0)
return false;
- return BO->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+ return BO->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+ alwaysWarnIfUnused);
// Consider '||', '&&' to have side effects if the LHS or RHS does.
case BO_LAnd:
case BO_LOr:
- if (!BO->getLHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx) ||
- !BO->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx))
+ if (!BO->getLHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+ alwaysWarnIfUnused) ||
+ !BO->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+ alwaysWarnIfUnused))
return false;
break;
}
@@ -2106,11 +2112,13 @@
// be being used for control flow. Only warn if both the LHS and
// RHS are warnings.
const ConditionalOperator *Exp = cast<ConditionalOperator>(this);
- if (!Exp->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx))
+ if (!Exp->getRHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+ alwaysWarnIfUnused))
return false;
if (!Exp->getLHS())
return true;
- return Exp->getLHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+ return Exp->getLHS()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+ alwaysWarnIfUnused);
}
case MemberExprClass:
@@ -2165,13 +2173,15 @@
bool HasWarnUnusedResultAttr = Func ? Func->hasUnusedResultAttr()
: FD->hasAttr<WarnUnusedResultAttr>();
+ HasWarnUnusedResultAttr = alwaysWarnIfUnused || HasWarnUnusedResultAttr;
+
// If the callee has attribute pure, const, or warn_unused_result, warn
// about it. void foo() { strlen("bar"); } should warn.
//
// Note: If new cases are added here, DiagnoseUnusedExprResult should be
// updated to match for QoI.
- if (HasWarnUnusedResultAttr ||
- FD->hasAttr<PureAttr>() || FD->hasAttr<ConstAttr>()) {
+ if (HasWarnUnusedResultAttr || FD->hasAttr<PureAttr>() ||
+ FD->hasAttr<ConstAttr>()) {
WarnE = this;
Loc = CE->getCallee()->getLocStart();
R1 = CE->getCallee()->getSourceRange();
@@ -2216,7 +2226,7 @@
}
if (const ObjCMethodDecl *MD = ME->getMethodDecl())
- if (MD->hasAttr<WarnUnusedResultAttr>()) {
+ if (alwaysWarnIfUnused || MD->hasAttr<WarnUnusedResultAttr>()) {
WarnE = this;
Loc = getExprLoc();
return true;
@@ -2254,10 +2264,12 @@
const CompoundStmt *CS = cast<StmtExpr>(this)->getSubStmt();
if (!CS->body_empty()) {
if (const Expr *E = dyn_cast<Expr>(CS->body_back()))
- return E->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+ return E->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+ alwaysWarnIfUnused);
if (const LabelStmt *Label = dyn_cast<LabelStmt>(CS->body_back()))
if (const Expr *E = dyn_cast<Expr>(Label->getSubStmt()))
- return E->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+ return E->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+ alwaysWarnIfUnused);
}
if (getType()->isVoidType())
@@ -2279,17 +2291,18 @@
dyn_cast<DeclRefExpr>(CE->getSubExpr()->IgnoreParens());
if (!(DRE && isa<VarDecl>(DRE->getDecl()) &&
cast<VarDecl>(DRE->getDecl())->hasLocalStorage())) {
- return CE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc,
- R1, R2, Ctx);
+ return CE->getSubExpr()->isUnusedResultAWarning(
+ WarnE, Loc, R1, R2, Ctx, alwaysWarnIfUnused);
}
}
return false;
}
// If this is a cast to a constructor conversion, check the operand.
// Otherwise, the result of the cast is unused.
if (CE->getCastKind() == CK_ConstructorConversion)
- return CE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+ return CE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+ alwaysWarnIfUnused);
WarnE = this;
if (const CXXFunctionalCastExpr *CXXCE =
@@ -2311,29 +2324,41 @@
ICE->getSubExpr()->getType().isVolatileQualified())
return false;
- return ICE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx);
+ return ICE->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx,
+ alwaysWarnIfUnused);
}
case CXXDefaultArgExprClass:
- return (cast<CXXDefaultArgExpr>(this)
- ->getExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx));
+ return (cast<CXXDefaultArgExpr>(this)->getExpr()->isUnusedResultAWarning(
+ WarnE, Loc, R1, R2, Ctx, alwaysWarnIfUnused));
case CXXDefaultInitExprClass:
- return (cast<CXXDefaultInitExpr>(this)
- ->getExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx));
+ return (cast<CXXDefaultInitExpr>(this)->getExpr()->isUnusedResultAWarning(
+ WarnE, Loc, R1, R2, Ctx, alwaysWarnIfUnused));
case CXXNewExprClass:
// FIXME: In theory, there might be new expressions that don't have side
// effects (e.g. a placement new with an uninitialized POD).
case CXXDeleteExprClass:
return false;
case CXXBindTemporaryExprClass:
- return (cast<CXXBindTemporaryExpr>(this)
- ->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx));
+ return (
+ cast<CXXBindTemporaryExpr>(this)->getSubExpr()->isUnusedResultAWarning(
+ WarnE, Loc, R1, R2, Ctx, alwaysWarnIfUnused));
case ExprWithCleanupsClass:
- return (cast<ExprWithCleanups>(this)
- ->getSubExpr()->isUnusedResultAWarning(WarnE, Loc, R1, R2, Ctx));
+ return (cast<ExprWithCleanups>(this)->getSubExpr()->isUnusedResultAWarning(
+ WarnE, Loc, R1, R2, Ctx, alwaysWarnIfUnused));
}
}
+/// isUnusedResult - Return true if this immediate expression has the unused
+/// result.
+bool Expr::isUnusedResult(ASTContext &Ctx) const {
+ const Expr *WarnExpr;
+ SourceLocation Loc;
+ SourceRange R1, R2;
+
+ return isUnusedResultAWarning(WarnExpr, Loc, R1, R2, Ctx, true);
+}
+
/// isOBJCGCCandidate - Check if an expression is objc gc'able.
/// returns true, if it is; false otherwise.
bool Expr::isOBJCGCCandidate(ASTContext &Ctx) const {
Index: lib/CodeGen/CGCall.cpp
===================================================================
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -3037,6 +3037,14 @@
DeferredReplacements.push_back(std::make_pair(Old, New));
}
+bool CodeGenFunction::isResultUnused(const Stmt* S) {
+ if (const LabelStmt *Label = dyn_cast_or_null<LabelStmt>(S))
+ return isResultUnused(Label->getSubStmt());
+
+ const Expr *E = dyn_cast_or_null<Expr>(S);
+ return E && E->isUnusedResult(getContext());
+}
+
RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
llvm::Value *Callee,
ReturnValueSlot ReturnValue,
@@ -3077,10 +3085,18 @@
// If the call returns a temporary with struct return, create a temporary
// alloca to hold the result, unless one is given to us.
llvm::Value *SRetPtr = nullptr;
+ size_t UnusedReturnSize = 0;
if (RetAI.isIndirect() || RetAI.isInAlloca()) {
SRetPtr = ReturnValue.getValue();
- if (!SRetPtr)
+ if (!SRetPtr) {
SRetPtr = CreateMemTemp(RetTy);
+ if (HaveInsertPoint() && CurStmt && isResultUnused(CurStmt)) {
+ uint64_t size =
+ CGM.getDataLayout().getTypeAllocSize(ConvertTypeForMem(RetTy));
+ if (EmitLifetimeStart(size, SRetPtr))
+ UnusedReturnSize = size;
+ }
+ }
if (IRFunctionArgs.hasSRetArg()) {
IRCallArgs[IRFunctionArgs.getSRetArgNo()] = SRetPtr;
} else {
@@ -3412,6 +3428,10 @@
// insertion point; this allows the rest of IRgen to discard
// unreachable code.
if (CS.doesNotReturn()) {
+ if (UnusedReturnSize)
+ EmitLifetimeEnd(llvm::ConstantInt::get(Int64Ty, UnusedReturnSize),
+ SRetPtr);
+
Builder.CreateUnreachable();
Builder.ClearInsertionPoint();
@@ -3440,8 +3460,13 @@
RValue Ret = [&] {
switch (RetAI.getKind()) {
case ABIArgInfo::InAlloca:
- case ABIArgInfo::Indirect:
- return convertTempToRValue(SRetPtr, RetTy, SourceLocation());
+ case ABIArgInfo::Indirect: {
+ RValue ret = convertTempToRValue(SRetPtr, RetTy, SourceLocation());
+ if (UnusedReturnSize)
+ EmitLifetimeEnd(llvm::ConstantInt::get(Int64Ty, UnusedReturnSize),
+ SRetPtr);
+ return ret;
+ }
case ABIArgInfo::Ignore:
// If we are ignoring an argument that had a result, make sure to
@@ -3530,4 +3555,4 @@
llvm::Value *CodeGenFunction::EmitVAArg(llvm::Value *VAListAddr, QualType Ty) {
return CGM.getTypes().getABIInfo().EmitVAArg(VAListAddr, Ty, *this);
-}
+}
\ No newline at end of file
Index: lib/CodeGen/CGStmt.cpp
===================================================================
--- lib/CodeGen/CGStmt.cpp
+++ lib/CodeGen/CGStmt.cpp
@@ -44,6 +44,19 @@
void CodeGenFunction::EmitStmt(const Stmt *S) {
assert(S && "Null statement?");
+
+ struct CurStmtScope {
+ const Stmt*& CurStmt;
+ CurStmtScope(const Stmt* S, const Stmt*& CurS) : CurStmt(CurS) {
+ CurStmt = S;
+ }
+ ~CurStmtScope() {
+ CurStmt = 0;
+ }
+ };
+
+ CurStmtScope curStmtScope(S, CurStmt);
+
PGO.setCurrentStmt(S);
// These statements have their own debug info handling.
Index: lib/CodeGen/CodeGenFunction.cpp
===================================================================
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -38,7 +38,7 @@
: CodeGenTypeCache(cgm), CGM(cgm), Target(cgm.getTarget()),
Builder(cgm.getModule().getContext(), llvm::ConstantFolder(),
CGBuilderInserterTy(this)),
- CurFn(nullptr), CapturedStmtInfo(nullptr),
+ CurFn(nullptr), CurStmt(nullptr), CapturedStmtInfo(nullptr),
SanOpts(CGM.getLangOpts().Sanitize), IsSanitizerScope(false),
CurFuncIsThunk(false), AutoreleaseResult(false), SawAsmBlock(false),
IsOutlinedSEHHelper(false), BlockInfo(nullptr), BlockPointer(nullptr),
Index: lib/CodeGen/CodeGenFunction.h
===================================================================
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -148,6 +148,7 @@
const CGFunctionInfo *CurFnInfo;
QualType FnRetTy;
llvm::Function *CurFn;
+ const Stmt *CurStmt;
/// CurGD - The GlobalDecl for the current function being compiled.
GlobalDecl CurGD;
@@ -1046,6 +1047,9 @@
void EmitOpenCLKernelMetadata(const FunctionDecl *FD,
llvm::Function *Fn);
+ /// Is result of statement unused
+ bool isResultUnused(const Stmt* S);
+
public:
CodeGenFunction(CodeGenModule &cgm, bool suppressNewContext=false);
~CodeGenFunction();
Index: test/CodeGenCXX/stack-reuse.cpp
===================================================================
--- test/CodeGenCXX/stack-reuse.cpp
+++ test/CodeGenCXX/stack-reuse.cpp
@@ -0,0 +1,125 @@
+// RUN: %clang -target armv7l-unknown-linux-gnueabihf -S %s -o - -emit-llvm -O1 -disable-llvm-optzns | FileCheck %s
+
+// Stack should be reused when possible, no need to allocate two separate slots
+// if they have disjoint lifetime.
+
+// Sizes of objects are related to previously existed threshold of 32. In case
+// of S_large stack size is rounded to 40 bytes.
+
+// 32B
+struct S_small {
+ int a[8];
+};
+
+// 36B
+struct S_large {
+ int a[9];
+};
+
+extern S_small foo_small();
+extern S_large foo_large();
+extern void bar_small(S_small*);
+extern void bar_large(S_large*);
+
+// Prevent mangling of function names.
+extern "C" {
+
+void small_rvoed_unnamed_temporary_object() {
+// CHECK-LABEL: define void @small_rvoed_unnamed_temporary_object
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_smallv
+// CHECK: call void @llvm.lifetime.end
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_smallv
+// CHECK: call void @llvm.lifetime.end
+
+ foo_small();
+ foo_small();
+}
+
+void large_rvoed_unnamed_temporary_object() {
+// CHECK-LABEL: define void @large_rvoed_unnamed_temporary_object
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_largev
+// CHECK: call void @llvm.lifetime.end
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_largev
+// CHECK: call void @llvm.lifetime.end
+
+ foo_large();
+ foo_large();
+}
+
+void small_rvoed_named_temporary_object() {
+// CHECK-LABEL: define void @small_rvoed_named_temporary_object
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_smallv
+// CHECK: call void @llvm.lifetime.end
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_smallv
+// CHECK: call void @llvm.lifetime.end
+
+ {
+ S_small s = foo_small();
+ }
+ {
+ S_small s = foo_small();
+ }
+}
+
+void large_rvoed_named_temporary_object() {
+// CHECK-LABEL: define void @large_rvoed_named_temporary_object
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_largev
+// CHECK: call void @llvm.lifetime.end
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9foo_largev
+// CHECK: call void @llvm.lifetime.end
+
+ {
+ S_large s = foo_large();
+ }
+ {
+ S_large s = foo_large();
+ }
+}
+
+void small_auto_object() {
+// CHECK-LABEL: define void @small_auto_object
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9bar_smallP7S_small
+// CHECK: call void @llvm.lifetime.end
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9bar_smallP7S_small
+// CHECK: call void @llvm.lifetime.end
+
+ {
+ S_small s;
+ bar_small(&s);
+ }
+ {
+ S_small s;
+ bar_small(&s);
+ }
+}
+
+void large_auto_object() {
+// CHECK-LABEL: define void @large_auto_object
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9bar_largeP7S_large
+// CHECK: call void @llvm.lifetime.end
+// CHECK: call void @llvm.lifetime.start
+// CHECK: call void @_Z9bar_largeP7S_large
+// CHECK: call void @llvm.lifetime.end
+
+ {
+ S_large s;
+ bar_large(&s);
+ }
+ {
+ S_large s;
+ bar_large(&s);
+ }
+}
+
+}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
