[PATCH] Mostly correct conditional handling for Consumed analysis

[Delesley Hutchins]

Aaron is right, and this is a blocker for submission of the patch.
(Thanks for the catch, Aaron!)  The original code didn't have leaks,
but the short-circuit returns are introducing them.  His suggestion to
wrap the pointers in OwningPtr is a good one, and it would prevent
future changes from causing this problem.  Please update addInfo, too,
so that it accepts an OwningPtr reference, and take()s ownership.
That would eliminate the need to document the memory management in
comments.

  -DeLesley


On Wed, Aug 28, 2013 at 12:03 PM, Aaron Ballman <aaron.ballman at gmail.com> wrote:
> On Wed, Aug 28, 2013 at 2:43 PM, Delesley Hutchins <delesley at google.com> wrote:
>>>> > -    CS_None,
>>>> > +    CS_None = 0,
>>>>
>>>> Any particular reason for this change?
>>>
>>> So that CS_None evaluates to false if a ConsumedState value is used in a
>>> conditional.
>>>
>>> It would still evaluate to false; it's value when unspecified will be zero.
>>
>> I don't like relying on auto-conversion from an enum to a bool; for
>> readability, I would prefer values of type ConsumedState to be tested
>> against a particular enum value.  (Otherwise, I have to go look at the
>> enum definition to see which value is false.)
>
> I'm in agreement with that.
>
>> I've gone over the memory management strategy with Chris, and I
>> believe it to be sound.
>
> Sound is good, but I'm still confused at the reticence over using
> managed pointers like OwningPtr.  Then you don't need to document the
> strategy because the code documents it for you, and it's considerably
> safer.
>
>>
>> LGTM, on condition that the enum tests are made explicit, and the
>> memory management strategy is suitably documented in a future patch.
>
> There are still leaks to be solved, and one tiny nit about spacing.
> Otherwise, the patch LGTM.
>
> @@ -694,42 +917,91 @@
>    return false;
>  }
>
> -// TODO: Handle other forms of branching with precision, including while- and
> -//       for-loops. (Deferred)
> -void ConsumedAnalyzer::splitState(const CFGBlock *CurrBlock,
> -                                  const IfStmt *Terminator) {
> +bool ConsumedAnalyzer::splitState(const CFGBlock *CurrBlock,
> +                                  const ConsumedStmtVisitor &Visitor) {
>
> -  TestedVarsVisitor Visitor(CurrStates);
> -  Visitor.TraverseStmt(const_cast<Expr*>(Terminator->getCond()));
> +  ConsumedStateMap *FalseStates = new ConsumedStateMap(*CurrStates);
> +  PropagationInfo   PInfo;
>
> -  bool HasElse = Terminator->getElse() != NULL;
> -
> -  ConsumedStateMap *ElseOrMergeStates = new ConsumedStateMap(*CurrStates);
> -
> -  if (Visitor.IsUsefulConditional) {
> -    ConsumedState VarState = CurrStates->getState(Visitor.Test.Var);
> +  if (const IfStmt *IfNode =
> +    dyn_cast_or_null<IfStmt>(CurrBlock->getTerminator().getStmt())) {
>
> -    if (VarState != CS_Unknown) {
> -      // FIXME: Make this not warn if the test is from a macro expansion.
> -      //        (Deferred)
> -      WarningsHandler.warnUnnecessaryTest(Visitor.Test.Var->getNameAsString(),
> -        stateToString(VarState), Visitor.Test.Loc);
> -    }
> +    bool HasElse = IfNode->getElse() != NULL;
> +    const Stmt *Cond = IfNode->getCond();
>
> -    if (Visitor.Test.UnconsumedInTrueBranch) {
> -      CurrStates->setState(Visitor.Test.Var, CS_Unconsumed);
> -      if (HasElse) ElseOrMergeStates->setState(Visitor.Test.Var, CS_Consumed);
> +    PInfo = Visitor.getInfo(Cond);
> +    if (!PInfo.isValid() && isa<BinaryOperator>(Cond))
> +      PInfo = Visitor.getInfo(cast<BinaryOperator>(Cond)->getRHS());
> +
> +    if (PInfo.isTest()) {
> +      CurrStates->setSource(Cond);
> +      FalseStates->setSource(Cond);
> +      splitVarStateForIf(IfNode, PInfo.getTest(), CurrStates,
> +                         HasElse ? FalseStates : NULL);
> +
> +    } else if (PInfo.isBinTest()) {
> +      CurrStates->setSource(PInfo.testSourceNode());
> +      FalseStates->setSource(PInfo.testSourceNode());
> +      splitVarStateForIfBinOp(PInfo, CurrStates, HasElse ? FalseStates :NULL);
>
> Missing a space after the colon.
>
>      } else {
> -      CurrStates->setState(Visitor.Test.Var, CS_Consumed);
> -      if (HasElse) ElseOrMergeStates->setState(Visitor.Test.Var,
> CS_Unconsumed);
> +      return false;
>
> This is leaking FalseStates
>
>      }
> -  }
>
> +  } else if (const BinaryOperator *BinOp =
> +    dyn_cast_or_null<BinaryOperator>(CurrBlock->getTerminator().getStmt())) {
> +
> +    PInfo = Visitor.getInfo(BinOp->getLHS());
> +    if (!PInfo.isTest()) {
> +      if ((BinOp = dyn_cast_or_null<BinaryOperator>(BinOp->getLHS()))) {
> +        PInfo = Visitor.getInfo(BinOp->getRHS());
> +
> +        if (!PInfo.isTest())
> +          return false;
> +
> +      } else {
> +        return false;
>
> Both of these returns are leaking FalseStates.
>
> +      }
> +    }
> +
> +    CurrStates->setSource(BinOp);
> +    FalseStates->setSource(BinOp);
> +
> +    const VarTestResult &Test = PInfo.getTest();
> +    ConsumedState VarState = CurrStates->getState(Test.Var);
> +
> +    if (BinOp->getOpcode() == BO_LAnd) {
> +      if (VarState == CS_Unknown)
> +        CurrStates->setState(Test.Var, Test.TestsFor);
> +      else if (VarState == invertConsumedUnconsumed(Test.TestsFor))
> +        CurrStates->markUnreachable();
> +
> +    } else if (BinOp->getOpcode() == BO_LOr) {
> +      if (VarState == CS_Unknown)
> +        FalseStates->setState(Test.Var,
> +                              invertConsumedUnconsumed(Test.TestsFor));
> +      else if (VarState == Test.TestsFor)
> +        FalseStates->markUnreachable();
> +    }
> +
> +  } else {
> +    return false;
> +  }
>
> Leaking FalseStates here as well.
>
> +
>    CFGBlock::const_succ_iterator SI = CurrBlock->succ_begin();
>
> -  if (*SI)   BlockInfo.addInfo(*SI,        CurrStates);
> -  if (*++SI) BlockInfo.addInfo(*SI, ElseOrMergeStates);
> +  if (*SI)
> +    BlockInfo.addInfo(*SI,  CurrStates);
> +  else
> +    delete CurrStates;
> +
> +  if (*++SI)
> +    BlockInfo.addInfo(*SI, FalseStates);
> +  else
> +    delete FalseStates;
> +
> +  CurrStates = NULL;
> +  return true;
>  }
>
> ~Aaron



-- 
DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315