[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
Ted Kremenek
kremenek at apple.com
Mon Mar 7 19:37:55 PST 2011
Thanks Nico. I was on vacation when you sent this email, so I didn't see it until now.
On Mar 5, 2011, at 11:14 PM, Nico Weber wrote:
> 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