Page MenuHomePhabricator

[analyzer] Handle modification of vars inside an expr with comma operator
ClosedPublic

Authored by djtodoro on Mar 3 2019, 11:43 PM.

Details

Summary

We should track mutation of a variable within a comma operator expression.
Current code in ExprMutationAnalyzer does not handle it.

This will handle cases like:

(a, b) ++ < == b is modified
(a, b) = c < == b is modifed

Diff Detail

Repository
rL LLVM

Event Timeline

djtodoro created this revision.Mar 3 2019, 11:43 PM

Thank you for working on this!

Is there any way to model this more generically?
I.e don't duplicate every naive matcher (ignoring possible presence of , op) with an variant that does not ignore ,.
E.g. will this handle (a,b)+=1 ?
What about (a,b).callNonConstMethod(), (a,b).callConstMethod() ?

@lebedev.ri Thanks for your comment!

Is there any way to model this more generically?
I.e don't duplicate every naive matcher (ignoring possible presence of , op) with an variant that does not ignore ,.
E.g. will this handle (a,b)+=1 ?

Hmm, I am not sure if there is a place for that, since this is very specific to comma operators. Any suggestions ?
It handles the situation ((a,b)+=1).

What about (a,b).callNonConstMethod(), (a,b).callConstMethod() ?

This needs to be extended to support function/method calls. I guess ExprMutationAnalyzer::findFunctionArgMutation requires an improvement.

@lebedev.ri Thanks for your comment!

Is there any way to model this more generically?
I.e don't duplicate every naive matcher (ignoring possible presence of , op) with an variant that does not ignore ,.
E.g. will this handle (a,b)+=1 ?

Hmm, I am not sure if there is a place for that, since this is very specific to comma operators. Any suggestions ?
It handles the situation ((a,b)+=1).

What about (a,b).callNonConstMethod(), (a,b).callConstMethod() ?

This needs to be extended to support function/method calls. I guess ExprMutationAnalyzer::findFunctionArgMutation requires an improvement.

Well, given

// LHS of any assignment operators.
const auto AsAssignmentLhs =
    binaryOperator(isAssignmentOperator(), hasLHS(equalsNode(Exp)));

The problem is that hasLHS() may get , op.
In isAssigmentWithCommma(), you check if that is , op, and if so, return it's right hand.
And this duplication is the problem as far as i can see.

Now, i don't know the final solution, but have you considered adding something like:

AST_MATCHER_P(Expr, skipCommaOps,
              ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
  const Expr* Result = Node;
  while (const auto *BOComma =
               dyn_cast_or_null<BinaryOperator>(Result->IgnoreParens())) {
    if (!BOComma->isCommaOp())
      break;
    Result = BOComma->getRHS();
  }
  return Result;
}

and then simply changing

// LHS of any assignment operators.
const auto AsAssignmentLhs =
    binaryOperator(isAssignmentOperator(), hasLHS(equalsNode(Exp)));

to

// LHS of any assignment operators.
const auto AsAssignmentLhs =
    binaryOperator(isAssignmentOperator(), hasLHS(skipCommaOps(equalsNode(Exp))));

?

@lebedev.ri I agree, thank you! I needed to be more precise in my previous reply, sorry for that. I thought it will be (somehow) overhead if I change existing, very basic, matchers.

I already implemented a static function that skips comma operands, and extended this to support member calls, functions, etc.
But, implementing it as a new matcher sounds like better idea.

Thanks!

@lebedev.ri I agree, thank you! I needed to be more precise in my previous reply, sorry for that. I thought it will be (somehow) overhead if I change existing, very basic, matchers.

I indeed don't think the existing matchers should be changed to ignore these , ops (or implicit casts, like some issue reports propose).

I already implemented a static function that skips comma operands, and extended this to support member calls, functions, etc.

But, implementing it as a new matcher sounds like better idea.

Yes. I think this matcher will be very baseline, and can just be added where needed
(with appropriate test coverage, of course), without matcher duplication like in this current diff.

Thanks!

Thanks for working on this.

djtodoro updated this revision to Diff 189461.Mar 6 2019, 2:09 AM

-add AST matcher that skips operands in comma expression
-add unit tests for extended support

@lebedev.ri I agree, thank you! I needed to be more precise in my previous reply, sorry for that. I thought it will be (somehow) overhead if I change existing, very basic, matchers.

I indeed don't think the existing matchers should be changed to ignore these , ops (or implicit casts, like some issue reports propose).

I already implemented a static function that skips comma operands, and extended this to support member calls, functions, etc.
But, implementing it as a new matcher sounds like better idea.

Yes. I think this matcher will be very baseline, and can just be added where needed
(with appropriate test coverage, of course), without matcher duplication like in this current diff.

@lebedev.ri I agree, we are on the same page! Thanks!

Nice, i like this!
I think the test coverage is good.
But what about other equalsNode() that you didn't change?
Do some of them need this change too?

lib/Analysis/ExprMutationAnalyzer.cpp
27 ↗(On Diff #189461)

(Can anyone suggest a better name maybe?
I'm not sure this is the most correct name, but i can't suggest a better alternative.)

223 ↗(On Diff #189461)

Not changed?

225–227 ↗(On Diff #189461)

Can these two happen with comma op?

240 ↗(On Diff #189461)

Same, can this happen with comma op?

248 ↗(On Diff #189461)

And here too

265 ↗(On Diff #189461)

And here

279 ↗(On Diff #189461)

And here

djtodoro marked an inline comment as done.Mar 6 2019, 2:40 AM

Nice, i like this!
I think the test coverage is good.

@lebedev.ri Thanks!

But what about other equalsNode() that you didn't change?
Do some of them need this change too?

For sure some of them (maybe all of them) needs usage of the new comma matcher.
I didn't check all of them, for those I checked I've created test cases.
But I agree, it should handle all possible situation.

lib/Analysis/ExprMutationAnalyzer.cpp
27 ↗(On Diff #189461)

Maybe evalCommaExpr?

lebedev.ri added inline comments.Mar 6 2019, 2:45 AM
lib/Analysis/ExprMutationAnalyzer.cpp
27 ↗(On Diff #189461)

Ah, yes, eval sounds better.
But, it won't always eval comma op or bailout.
It will eval if it is there.
So maybe maybeEvalCommaExpr (pun intended!).

djtodoro marked an inline comment as done.Mar 6 2019, 4:06 AM
djtodoro added inline comments.
lib/Analysis/ExprMutationAnalyzer.cpp
27 ↗(On Diff #189461)

Sure, maybeEvalCommaExpr sounds better.
Or, one more suggestion is tryEvalCommaExpr ?

lebedev.ri accepted this revision.Mar 6 2019, 9:46 AM

Anyways, this looks good in this state.
Thank you for working on this!

If you don't intend to immediately work on fixing the rest of , cases here,
*please* do file a bug so it won't get lost. (and link it here)

lib/Analysis/ExprMutationAnalyzer.cpp
27 ↗(On Diff #189461)

try, at least to me, has the same meaning as evalCommaExpr, i.e. one might expect that if it fails, it will fail.
So not sure.

This revision is now accepted and ready to land.Mar 6 2019, 9:46 AM
djtodoro marked an inline comment as done.Mar 7 2019, 12:58 AM

Anyways, this looks good in this state.
Thank you for working on this!

Thanks!

If you don't intend to immediately work on fixing the rest of , cases here,
*please* do file a bug so it won't get lost. (and link it here)

I already extended this in order to support the rest of direct mutations.

lib/Analysis/ExprMutationAnalyzer.cpp
27 ↗(On Diff #189461)

Hmm, I agree. You are right. Let it be maybeEvalCommaExpr (for now), if there is no better suggestion.

djtodoro updated this revision to Diff 189661.Mar 7 2019, 1:00 AM

-add support for the rest of direct mutation cases

lebedev.ri accepted this revision.Mar 7 2019, 1:06 AM

Still LG.
I'm sure you have verified that there is test coverage for all the newly-supported cases.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2019, 7:50 AM