Page MenuHomePhabricator

[clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups
ClosedPublic

Authored by zinovy.nis on Nov 8 2020, 11:42 AM.

Diff Detail

Event Timeline

zinovy.nis created this revision.Nov 8 2020, 11:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2020, 11:42 AM
zinovy.nis requested review of this revision.Nov 8 2020, 11:42 AM
zinovy.nis added a reviewer: baloghadamsoftware.

Think this has an incorrect name, seems to have something to do with bugprone-redundant-branch-condition

Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
87

Please fix warning.

zinovy.nis retitled this revision from [clang-tidy][bugprone-use-after-mnove] Warn on std::move for consts to [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups.Nov 8 2020, 9:04 PM

Think this has an incorrect name, seems to have something to do with bugprone-redundant-branch-condition

Oops, thanks to autofill. Fixed.

zinovy.nis updated this revision to Diff 303750.Nov 8 2020, 9:11 PM
zinovy.nis edited the summary of this revision. (Show Details)

auto -> const auto.

zinovy.nis marked an inline comment as done.Nov 8 2020, 9:12 PM
aaron.ballman added inline comments.Nov 10 2020, 6:26 AM
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
85–87

Under what circumstances does getCond() return nullptr?

90

You can call ExprWithCleanupsCond->getSubExpr() to do this more cleanly -- but similar question here as above -- what circumstances lead to a null sub expression?

134–135

Same feedback here as above.

zinovy.nis marked 2 inline comments as done.

*iterator -> getSubExpr()

zinovy.nis marked an inline comment as done and 2 inline comments as not done.Nov 10 2020, 10:02 AM
zinovy.nis added inline comments.
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
85–87

getCond() is not null, but it can be ExprWithCleanupsCond leading the original dyn_cast to crash. That what this bug is about.

90

Thanks, fixed to use getSubExpr().

aaron.ballman added inline comments.Nov 10 2020, 10:38 AM
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
85–87

That's what I figured, but you changed dyn_cast<> to be dyn_cast_or_null<> and that seems incorrect -- getCond() doesn't return null so the dyn_cast<> was correct.

zinovy.nis marked 2 inline comments as done.Nov 10 2020, 11:35 AM
zinovy.nis added inline comments.
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
85–87

dyn_cast asserts unless getCond() returns BinaryExpression, right?
But getCond() can return (and it does so in the test case) ExprWithCleanups which is not a subclass of BinaryExpression.
That's why I use _or_null version of dyn_cast.

zinovy.nis marked an inline comment as done.Nov 10 2020, 11:35 AM
aaron.ballman added inline comments.Nov 10 2020, 11:40 AM
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
85–87

dyn_cast asserts unless getCond() returns BinaryExpression, right?

Nope.

cast<> asserts if the cast cannot be completed or if the cast input is null.
dyn_cast<> returns null if the cast cannot be completed and asserts if the cast input is null.
There are _or_null variants of each which will accept a null input without asserting.

dyn_cast_or_null -> dyn_cast

zinovy.nis marked an inline comment as done.Nov 10 2020, 11:43 AM
zinovy.nis added inline comments.
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
85–87

Yes, you are right, I definitely need a vacation :-)
I confused dyn_cast with cast
I've uploaded a fix.

zinovy.nis marked an inline comment as done.Nov 10 2020, 12:17 PM
aaron.ballman added inline comments.Nov 10 2020, 1:10 PM
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
136

The old code used to assert that CondOp was a BinaryOperator but the new code means that CondOp can be null -- should you add an assert in to ensure we have a valid CondOp still or will that assert trigger on some constructs?

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
1077

Can remove the newline here.

Taking a step back, rather than worrying about if its an ExprWithCleanups. Shouldn't we just get the condition removing all implicit nodes.

const Expr* Cond = InnerIf->getCond()->ignoreImplicit();

This has to be simpler and will likely future proof this against any other implicit nodes potentially added in future that can appear between the Condition and the binary operator.

Simplified to use IgnoreImplicit().

zinovy.nis marked 2 inline comments as done.Nov 10 2020, 10:12 PM
zinovy.nis added inline comments.
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
136

Reverted to cast.

zinovy.nis marked an inline comment as done.Nov 10 2020, 10:15 PM

Taking a step back, rather than worrying about if its an ExprWithCleanups. Shouldn't we just get the condition removing all implicit nodes.

const Expr* Cond = InnerIf->getCond()->ignoreImplicit();

This has to be simpler and will likely future proof this against any other implicit nodes potentially added in future that can appear between the Condition and the binary operator.

Nice! Thanks. Changed to use IgnoreImplicit().

njames93 added inline comments.Nov 11 2020, 1:39 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
1092

This line isn't really doing anything. CHECK-FIXES just runs FileCheck on the whole file after clang tidy has applied any fixes. You need to explicitly say what you are expecting it to be.

njames93 added inline comments.Nov 11 2020, 4:16 AM
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
45

Just noticed, as this is ignoring Parenthesis and implicit nodes/casts, maybe we should also be ignoring those when getting the condition in the check
InnerIf->getCond()->IgnoreParenImpCasts()
I reckon if that change isnt made this could fail on

if (IsSet){
  if ((OtherCond && IsSet))
    ;
}

Taking a step back, rather than worrying about if its an ExprWithCleanups. Shouldn't we just get the condition removing all implicit nodes.

const Expr* Cond = InnerIf->getCond()->ignoreImplicit();

This has to be simpler and will likely future proof this against any other implicit nodes potentially added in future that can appear between the Condition and the binary operator.

Good call!

Use IgnoreParenImpCasts

zinovy.nis marked an inline comment as done.Nov 11 2020, 9:56 AM
zinovy.nis marked an inline comment as done.
zinovy.nis added inline comments.
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
45

You are right, thanks! I fixed it.

zinovy.nis marked an inline comment as done.Nov 11 2020, 9:58 AM
aaron.ballman added inline comments.Nov 11 2020, 12:20 PM
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
1092

There's not a whole lot of context for FileCheck to determine if it's been correctly applied or not (same below) -- for instance, won't this pass even if no changes are applied because FileCheck is still going to find isSet in the file?

Fixed fix-it section.

zinovy.nis marked an inline comment as done.Nov 11 2020, 10:10 PM
zinovy.nis added inline comments.
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
1092

Thanks. Fixed.

zinovy.nis marked an inline comment as done.Nov 11 2020, 10:10 PM
aaron.ballman added inline comments.Nov 12 2020, 4:43 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
1092

Maybe it's just early in the morning for me, but... I was expecting the transformation to be:

if (RetT::Test(isSet).Ok() && isSet) {
  if (RetT::Test(isSet).Ok() && isSet) {
  }
}

turns into

if (RetT::Test(isSet).Ok() && isSet) {
}

Why does it remove the && isSet instead? That seems like it's changing the logic here from if (true && false) to if (true).

zinovy.nis added inline comments.Nov 12 2020, 9:09 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
1092

IMO it's correct.
isSet cannot change its value between ifs while RetT::Test(isSet).Ok() can.
So we don't need to re-evaluate isSet and need to re-evaluate RetT::Test(isSet).Ok() only.

That seems like it's changing the logic here from if (true && false) to if (true).

As I understand only the second if is transformed.

aaron.ballman added inline comments.Nov 12 2020, 9:48 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
1092

Does this only trigger as a redundant branch condition if the definition of RetT::Test() is available? Because Test() takes a bool& so it sure seems like isSet could be modified between the branches if the definition isn't found because it's being passed as a non-const reference to Test().

zinovy.nis marked an inline comment as done.Nov 12 2020, 10:03 AM
zinovy.nis added inline comments.
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
1092

if the definition of RetT::Test() is available?

Removing the body from RetT::Test changes nothing.

  1. Turning RetT Test(bool &_isSet) -> RetT Test(bool _isSet) also changes nothing.
zinovy.nis marked an inline comment as done.Nov 12 2020, 10:03 AM
aaron.ballman accepted this revision.Nov 12 2020, 10:33 AM

LGTM!

clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
1092

Given the following four ways of declaring Test():

static RetT Test(bool &_isSet); // #1
static RetT Test(bool _isSet); // #2
static RetT Test(const bool &_isSet); // #3
static RetT Test(bool &_isSet) { return 0; } // #4

I would expect #2 and #3 to behave the same way -- the isSet argument could never be modified by calling Test() and so the second Test(isSet) && isSet is partially redundant and the second if statement can drop the && isSet. (Without dropping the Test(isSet) because the call could still modify some global somewhere, etc.)

I would expect #1 to not modify the second if statement at all because there's no way of knowing whether Test(isSet) && isSet is the same between the first if statement and the second one (because the second call to Test(isSet) may modify isSet in a way the caller can observe).

Ideally, #4 would be a case where we could remove the entire second if statement because we can identify that not only does isSet not get modified despite being passed by non-const reference, we can see that the Test() function doesn't modify any state at all. However, this seems like it would require data flow analysis and so I think it makes sense to treat #4 the same as #1.

That said, I just realized the check isn't being very careful with reference parameters in the first place: https://godbolt.org/z/P1aP3W, so your changes aren't introducing a new problem, they just happened to highlight an existing issue.

This revision is now accepted and ready to land.Nov 12 2020, 10:33 AM

LGTM!

Thanks! I'll see what can be done to deal with it in a separate commit.

zinovy.nis added inline comments.
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
1092

Please look at https://reviews.llvm.org/D91495 - there's a fix for it.