Page MenuHomePhabricator

[analyzer] Fix SimpleSValBuilder::simplifySVal
ClosedPublic

Authored by alexshap on Aug 9 2017, 5:21 PM.

Details

Summary

This diff attempts to address a crash (triggered assert) on the newly-added test case.
The assert being discussed is inside SValBuilder::evalBinOp

if (Optional<Loc> RV = rhs.getAs<Loc>()) {
   // Support pointer arithmetic where the addend is on the left
   // and the pointer on the right.
   assert(op == BO_Add);

but the root cause seems to be in a different place
(if I'm not missing smth).

The method simplifySVal appears to be doing the wrong thing for SVal "(char*)End - (char*)Begin".
Call stack: evalIntegralCast -> evalBinOpNN -> simplifySVal -> ... -> simplifySVal -> Simplifier::VisitSymSymExpr -> ...

Inside Simplifier::VisitSymSymExpr the call Visit(S->getLHS()) returns a NonLoc
(where S->getLHS() represents char* End) while the call Visit(S->getRHS()) returns a Loc.

For Visit(S->getRHS()) this happens because in VisitSymbolData(const SymbolData *S) the "if" condition

if (const llvm::APSInt *I =
   SVB.getKnownValue(State, nonloc::SymbolVal(S)))

is satisfied and Loc is correctly chosen.
For Visit(S->getLHS()) nonloc::SymbolVal(S) is used instead.
Next those values will passed to SValBuilder::evalBinOp and the code path

if (Optional<Loc> RV = rhs.getAs<Loc>()) {
   // Support pointer arithmetic where the addend is on the left
   // and the pointer on the right.
  assert(op == BO_Add);
   // Commute the operands.
  return evalBinOpLN(state, op, *RV, lhs.castAs<NonLoc>(), type);
}

will be executed instead of the correct one

if (Optional<Loc> LV = lhs.getAs<Loc>()) {
  if (Optional<Loc> RV = rhs.getAs<Loc>())
    return evalBinOpLL(state, op, *LV, *RV, type);

  return evalBinOpLN(state, op, *LV, rhs.castAs<NonLoc>(), type);
}

Test plan: make check-all (green)

Diff Detail

Repository
rL LLVM

Event Timeline

alexshap created this revision.Aug 9 2017, 5:21 PM
alexshap edited the summary of this revision. (Show Details)Aug 9 2017, 5:22 PM
alexshap edited the summary of this revision. (Show Details)Aug 10 2017, 8:27 AM
alexshap edited the summary of this revision. (Show Details)Aug 10 2017, 2:04 PM
NoQ edited edge metadata.Aug 14 2017, 7:06 AM

Hmm, looks correct. Thanks! I guess this problem appeared when we enabled SymSymExprs, otherwise (End - Begin) would have been UnknownVal.

NoQ accepted this revision.Aug 14 2017, 7:06 AM
This revision is now accepted and ready to land.Aug 14 2017, 7:06 AM
This revision was automatically updated to reflect the committed changes.
alexfh added a subscriber: alexfh.Aug 24 2017, 6:28 AM

I suspect this might have caused http://llvm.org/PR34309. Could you take a look?

I suspect this might have caused http://llvm.org/PR34309. Could you take a look?

FYI, PR34309 doesn't reproduce with this patch reverted.

NoQ added a comment.Aug 24 2017, 7:11 AM

I see, it seems that we've constructed $x - 1 somewhere, where $x is a pointer, while such stuff normally looks as &element{SymRegion{$x}, -1}. I guess i'd have to take a more careful look at it soon.

@alexfh, thanks for letting me know, i will take a closer look at https://bugs.llvm.org/show_bug.cgi?id=34309 soon.