[cfe-commits] r126797 - in /cfe/trunk: lib/Analysis/CFG.cpp lib/StaticAnalyzer/Core/ExprEngine.cpp test/Analysis/misc-ps.m test/SemaCXX/array-bounds.cpp
Nico Weber
thakis at chromium.org
Sat Mar 5 23:14:20 PST 2011
Filed http://llvm.org/bugs/show_bug.cgi?id=9412 with repro case.
On Sat, Mar 5, 2011 at 8:53 PM, Nico Weber <thakis at chromium.org> wrote:
> Hi Ted,
>
> when updating clang on the chrome buildbots, clang now crashes when
> compiling webkit. I binary searched, and it looks like this change is
> what caused it. It's a bit tricky to reproduce, since for some reason
> the crash happens only on the buildbots, not on my local box. I'll try
> to get a reliable repro, but the code this crashes on looks something
> like this:
>
> namespace {
>
> enum MatchType {
> Exact,
> UpperBound,
> LowerBound
> };
>
> template <MatchType type> int getScaledValue(const Vector<int>&
> scaledValues, int valueToMatch, int searchStart)
> {
> if (scaledValues.isEmpty())
> return valueToMatch;
>
> const int* dataStart = scaledValues.data();
> const int* dataEnd = dataStart + scaledValues.size();
> const int* matched = std::lower_bound(dataStart + searchStart,
> dataEnd, valueToMatch);
> switch (type) {
> case Exact:
> return matched != dataEnd && *matched == valueToMatch ?
> matched - dataStart : -1;
> case LowerBound:
> return matched != dataEnd && *matched == valueToMatch ?
> matched - dataStart : matched - dataStart - 1;
> case UpperBound:
> default:
> return matched != dataEnd ? matched - dataStart : -1;
> }
> }
>
> }
>
> // ...
>
> int ImageDecoder::upperBoundScaledY(int origY, int searchStart)
> {
> return getScaledValue<UpperBound>(m_scaledRows, origY, searchStart);
> }
>
> int ImageDecoder::lowerBoundScaledY(int origY, int searchStart)
> {
> return getScaledValue<LowerBound>(m_scaledRows, origY, searchStart);
> }
>
> int ImageDecoder::scaledY(int origY, int searchStart)
> {
> return getScaledValue<Exact>(m_scaledRows, origY, searchStart);
> }
>
>
> Maybe it's obvious to you what's wrong?
>
>
> The complete code is here:
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.cpp
>
> The stack this crashes with is:
>
> 0 clang 0x00000000017c964f
> 1 clang 0x00000000017cb8c2
> 2 libpthread.so.0 0x00002b3feb2528f0
> 3 clang 0x0000000000c65aa6
> clang::CFGBlock::FilterEdge(clang::CFGBlock::FilterOptions const&,
> clang::CFGBlock const*, clang::CFGBlock const*) + 38
> 4 clang 0x0000000000c82f83
> clang::reachable_code::ScanReachableFromBlock(clang::CFGBlock const&,
> llvm::BitVector&) + 323
> 5 clang 0x0000000000b55a2e
> clang::sema::AnalysisBasedWarnings::IssueWarnings(clang::sema::AnalysisBasedWarnings::Policy,
> clang::sema::FunctionScopeInfo*, clang::Decl const*, clang::BlockExpr
> const*) + 1198
> 6 clang 0x0000000000909c04
> clang::Sema::PopFunctionOrBlockScope(clang::sema::AnalysisBasedWarnings::Policy
> const*, clang::Decl const*, clang::BlockExpr const*) + 180
> 7 clang 0x0000000000968d89
> clang::Sema::ActOnFinishFunctionBody(clang::Decl*, clang::Stmt*, bool)
> + 553
> 8 clang 0x0000000000b307d1
> clang::Sema::InstantiateFunctionDefinition(clang::SourceLocation,
> clang::FunctionDecl*, bool, bool) + 1121
> 9 clang 0x0000000000b3009d
> clang::Sema::PerformPendingInstantiations(bool) + 365
> 10 clang 0x0000000000909d62
> clang::Sema::ActOnEndOfTranslationUnit() + 306
> 11 clang 0x00000000008cac68
> clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&)
> + 200
> 12 clang 0x00000000008b161b clang::ParseAST(clang::Sema&, bool) + 139
> 13 clang 0x000000000078dcf4 clang::CodeGenAction::ExecuteAction() + 68
> 14 clang 0x000000000067322b
> clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 347
> 15 clang 0x000000000064da54
> clang::ExecuteCompilerInvocation(clang::CompilerInstance*) + 1268
> 16 clang 0x00000000006453df cc1_main(char const**, char
> const**, char const*, void*) + 655
> 17 clang 0x000000000064caa7 main + 4679
> 18 libc.so.6 0x00002b3febe32c4d __libc_start_main + 253
> 19 clang 0x0000000000643a69
> Stack dump:
> 0. Program arguments: /* very long command line */
> 1. <eof> parser at end of file
> 2. third_party/WebKit/Source/WebCore/platform/image-decoders/ImageDecoder.cpp:257:31:
> instantiating function definition 'getScaledValue'
> clang: error: unable to execute command: Segmentation fault
> clang: error: clang frontend command failed due to signal 1 (use -v to
> see invocation)
>
>
> Thanks,
> Nico
>
> On Tue, Mar 1, 2011 at 3:12 PM, Ted Kremenek <kremenek at apple.com> wrote:
>> Author: kremenek
>> Date: Tue Mar 1 17:12:55 2011
>> New Revision: 126797
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=126797&view=rev
>> Log:
>> Teach CFGBuilder to prune trivially unreachable case statements.
>>
>> Modified:
>> cfe/trunk/lib/Analysis/CFG.cpp
>> cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
>> cfe/trunk/test/Analysis/misc-ps.m
>> cfe/trunk/test/SemaCXX/array-bounds.cpp
>>
>> Modified: cfe/trunk/lib/Analysis/CFG.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/CFG.cpp?rev=126797&r1=126796&r2=126797&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Analysis/CFG.cpp (original)
>> +++ cfe/trunk/lib/Analysis/CFG.cpp Tue Mar 1 17:12:55 2011
>> @@ -210,6 +210,25 @@
>> LocalScope::const_iterator scopePosition;
>> };
>>
>> +/// TryResult - a class representing a variant over the values
>> +/// 'true', 'false', or 'unknown'. This is returned by tryEvaluateBool,
>> +/// and is used by the CFGBuilder to decide if a branch condition
>> +/// can be decided up front during CFG construction.
>> +class TryResult {
>> + int X;
>> +public:
>> + TryResult(bool b) : X(b ? 1 : 0) {}
>> + TryResult() : X(-1) {}
>> +
>> + bool isTrue() const { return X == 1; }
>> + bool isFalse() const { return X == 0; }
>> + bool isKnown() const { return X >= 0; }
>> + void negate() {
>> + assert(isKnown());
>> + X ^= 0x1;
>> + }
>> +};
>> +
>> /// CFGBuilder - This class implements CFG construction from an AST.
>> /// The builder is stateful: an instance of the builder should be used to only
>> /// construct a single CFG.
>> @@ -238,7 +257,7 @@
>> CFGBlock* SwitchTerminatedBlock;
>> CFGBlock* DefaultCaseBlock;
>> CFGBlock* TryTerminatedBlock;
>> -
>> +
>> // Current position in local scope.
>> LocalScope::const_iterator ScopePos;
>>
>> @@ -257,12 +276,17 @@
>>
>> bool badCFG;
>> CFG::BuildOptions BuildOpts;
>> +
>> + // State to track for building switch statements.
>> + bool switchExclusivelyCovered;
>> + Expr::EvalResult switchCond;
>>
>> public:
>> explicit CFGBuilder() : cfg(new CFG()), // crew a new CFG
>> Block(NULL), Succ(NULL),
>> SwitchTerminatedBlock(NULL), DefaultCaseBlock(NULL),
>> - TryTerminatedBlock(NULL), badCFG(false) {}
>> + TryTerminatedBlock(NULL), badCFG(false),
>> + switchExclusivelyCovered(false) {}
>>
>> // buildCFG - Used by external clients to construct the CFG.
>> CFG* buildCFG(const Decl *D, Stmt *Statement, ASTContext *C,
>> @@ -387,45 +411,34 @@
>> B->addSuccessor(S, cfg->getBumpVectorContext());
>> }
>>
>> - /// TryResult - a class representing a variant over the values
>> - /// 'true', 'false', or 'unknown'. This is returned by tryEvaluateBool,
>> - /// and is used by the CFGBuilder to decide if a branch condition
>> - /// can be decided up front during CFG construction.
>> - class TryResult {
>> - int X;
>> - public:
>> - TryResult(bool b) : X(b ? 1 : 0) {}
>> - TryResult() : X(-1) {}
>> -
>> - bool isTrue() const { return X == 1; }
>> - bool isFalse() const { return X == 0; }
>> - bool isKnown() const { return X >= 0; }
>> - void negate() {
>> - assert(isKnown());
>> - X ^= 0x1;
>> - }
>> - };
>> + /// Try and evaluate an expression to an integer constant.
>> + bool tryEvaluate(Expr *S, Expr::EvalResult &outResult) {
>> + if (!BuildOpts.PruneTriviallyFalseEdges)
>> + return false;
>> + return !S->isTypeDependent() &&
>> + !S->isValueDependent() &&
>> + S->Evaluate(outResult, *Context);
>> + }
>>
>> /// tryEvaluateBool - Try and evaluate the Stmt and return 0 or 1
>> /// if we can evaluate to a known value, otherwise return -1.
>> TryResult tryEvaluateBool(Expr *S) {
>> - if (!BuildOpts.PruneTriviallyFalseEdges)
>> + Expr::EvalResult Result;
>> + if (!tryEvaluate(S, Result))
>> return TryResult();
>> +
>> + if (Result.Val.isInt())
>> + return Result.Val.getInt().getBoolValue();
>>
>> - Expr::EvalResult Result;
>> - if (!S->isTypeDependent() && !S->isValueDependent() &&
>> - S->Evaluate(Result, *Context)) {
>> - if (Result.Val.isInt())
>> - return Result.Val.getInt().getBoolValue();
>> - if (Result.Val.isLValue()) {
>> - Expr *e = Result.Val.getLValueBase();
>> - const CharUnits &c = Result.Val.getLValueOffset();
>> - if (!e && c.isZero())
>> - return false;
>> - }
>> + if (Result.Val.isLValue()) {
>> + Expr *e = Result.Val.getLValueBase();
>> + const CharUnits &c = Result.Val.getLValueOffset();
>> + if (!e && c.isZero())
>> + return false;
>> }
>> return TryResult();
>> }
>> +
>> };
>>
>> // FIXME: Add support for dependent-sized array types in C++?
>> @@ -2180,6 +2193,17 @@
>> assert(Terminator->getBody() && "switch must contain a non-NULL body");
>> Block = NULL;
>>
>> + // For pruning unreachable case statements, save the current state
>> + // for tracking the condition value.
>> + SaveAndRestore<bool> save_switchExclusivelyCovered(switchExclusivelyCovered,
>> + false);
>> + SaveAndRestore<Expr::EvalResult> save_switchCond(switchCond);
>> +
>> +
>> + // Determine if the switch condition can be explicitly evaluated.
>> + assert(Terminator->getCond() && "switch condition must be non-NULL");
>> + tryEvaluate(Terminator->getCond(), switchCond);
>> +
>> // If body is not a compound statement create implicit scope
>> // and add destructors.
>> if (!isa<CompoundStmt>(Terminator->getBody()))
>> @@ -2193,11 +2217,11 @@
>>
>> // If we have no "default:" case, the default transition is to the code
>> // following the switch body.
>> - addSuccessor(SwitchTerminatedBlock, DefaultCaseBlock);
>> + addSuccessor(SwitchTerminatedBlock,
>> + switchExclusivelyCovered ? 0 : DefaultCaseBlock);
>>
>> // Add the terminator and condition in the switch block.
>> SwitchTerminatedBlock->setTerminator(Terminator);
>> - assert(Terminator->getCond() && "switch condition must be non-NULL");
>> Block = SwitchTerminatedBlock;
>> Block = addStmt(Terminator->getCond());
>>
>> @@ -2213,6 +2237,44 @@
>>
>> return Block;
>> }
>> +
>> +static bool shouldAddCase(bool &switchExclusivelyCovered,
>> + const Expr::EvalResult &switchCond,
>> + const CaseStmt *CS,
>> + ASTContext &Ctx) {
>> + bool addCase = false;
>> +
>> + if (!switchExclusivelyCovered) {
>> + if (switchCond.Val.isInt()) {
>> + // Evaluate the LHS of the case value.
>> + Expr::EvalResult V1;
>> + CS->getLHS()->Evaluate(V1, Ctx);
>> + assert(V1.Val.isInt());
>> + const llvm::APSInt &condInt = switchCond.Val.getInt();
>> + const llvm::APSInt &lhsInt = V1.Val.getInt();
>> +
>> + if (condInt == lhsInt) {
>> + addCase = true;
>> + switchExclusivelyCovered = true;
>> + }
>> + else if (condInt < lhsInt) {
>> + if (const Expr *RHS = CS->getRHS()) {
>> + // Evaluate the RHS of the case value.
>> + Expr::EvalResult V2;
>> + RHS->Evaluate(V2, Ctx);
>> + assert(V2.Val.isInt());
>> + if (V2.Val.getInt() <= condInt) {
>> + addCase = true;
>> + switchExclusivelyCovered = true;
>> + }
>> + }
>> + }
>> + }
>> + else
>> + addCase = true;
>> + }
>> + return addCase;
>> +}
>>
>> CFGBlock* CFGBuilder::VisitCaseStmt(CaseStmt* CS) {
>> // CaseStmts are essentially labels, so they are the first statement in a
>> @@ -2232,9 +2294,12 @@
>> else
>> TopBlock = currentBlock;
>>
>> - addSuccessor(SwitchTerminatedBlock, currentBlock);
>> - LastBlock = currentBlock;
>> + addSuccessor(SwitchTerminatedBlock,
>> + shouldAddCase(switchExclusivelyCovered, switchCond,
>> + CS, *Context)
>> + ? currentBlock : 0);
>>
>> + LastBlock = currentBlock;
>> CS = cast<CaseStmt>(Sub);
>> Sub = CS->getSubStmt();
>> }
>> @@ -2256,7 +2321,10 @@
>> // Add this block to the list of successors for the block with the switch
>> // statement.
>> assert(SwitchTerminatedBlock);
>> - addSuccessor(SwitchTerminatedBlock, CaseBlock);
>> + addSuccessor(SwitchTerminatedBlock,
>> + shouldAddCase(switchExclusivelyCovered, switchCond,
>> + CS, *Context)
>> + ? CaseBlock : 0);
>>
>> // We set Block to NULL to allow lazy creation of a new block (if necessary)
>> Block = NULL;
>>
>> Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp?rev=126797&r1=126796&r2=126797&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp (original)
>> +++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp Tue Mar 1 17:12:55 2011
>> @@ -1043,6 +1043,10 @@
>> bool defaultIsFeasible = I == EI;
>>
>> for ( ; I != EI; ++I) {
>> + // Successor may be pruned out during CFG construction.
>> + if (!I.getBlock())
>> + continue;
>> +
>> const CaseStmt* Case = I.getCase();
>>
>> // Evaluate the LHS of the case value.
>>
>> Modified: cfe/trunk/test/Analysis/misc-ps.m
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/misc-ps.m?rev=126797&r1=126796&r2=126797&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/Analysis/misc-ps.m (original)
>> +++ cfe/trunk/test/Analysis/misc-ps.m Tue Mar 1 17:12:55 2011
>> @@ -1269,3 +1269,22 @@
>> }
>> }
>>
>> +void test_switch() {
>> + switch (4) {
>> + case 1: {
>> + int *p = 0;
>> + *p = 0xDEADBEEF; // no-warning
>> + break;
>> + }
>> + case 4: {
>> + int *p = 0;
>> + *p = 0xDEADBEEF; // expected-warning {{null}}
>> + break;
>> + }
>> + default: {
>> + int *p = 0;
>> + *p = 0xDEADBEEF; // no-warning
>> + break;
>> + }
>> + }
>> +}
>> \ No newline at end of file
>>
>> Modified: cfe/trunk/test/SemaCXX/array-bounds.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/array-bounds.cpp?rev=126797&r1=126796&r2=126797&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/SemaCXX/array-bounds.cpp (original)
>> +++ cfe/trunk/test/SemaCXX/array-bounds.cpp Tue Mar 1 17:12:55 2011
>> @@ -127,4 +127,23 @@
>> return sizeof(char) == sizeof(char) ? arr[2] : arr[1]; // expected-warning {{array index of '2' indexes past the end of an array (that contains 2 elements)}}
>> }
>>
>> +void test_switch() {
>> + switch (4) {
>> + case 1: {
>> + int arr[2];
>> + arr[2] = 1; // no-warning
>> + break;
>> + }
>> + case 4: {
>> + int arr[2]; // expected-note {{array 'arr' declared here}}
>> + arr[2] = 1; // expected-warning {{array index of '2' indexes past the end of an array (that contains 2 elements)}}
>> + break;
>> + }
>> + default: {
>> + int arr[2];
>> + arr[2] = 1; // no-warning
>> + break;
>> + }
>> + }
>> +}
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
More information about the cfe-commits
mailing list