This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fixing a bug raising false alarms on static local variables in the Infinite Loop Checker
ClosedPublic

Authored by ziqingluo-90 on Jun 22 2022, 4:41 PM.

Details

Summary

The Infinite Loop Checker could report a false positive on the example below:

void f() {
  static int i = 0;
  i++;
  while (i < 10)
     f();
}

This patch adds checking for recursive calls in the Infinite Loop Checker when loop condition involves static local variables.

Diff Detail

Event Timeline

ziqingluo-90 created this revision.Jun 22 2022, 4:41 PM
ziqingluo-90 requested review of this revision.Jun 22 2022, 4:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2022, 4:41 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
179

auto could be used, because type is spelled in same statement.

186

Ditto.

220

Ditto.

221

Ditto.

Eugene.Zelenko retitled this revision from [Clang-tidy] Fixing a bug raising false alarms on static local variables in the Infinite Loop Checker to [clang-tidy] Fixing a bug raising false alarms on static local variables in the Infinite Loop Checker .Jun 23 2022, 7:13 AM
gribozavr2 accepted this revision.Jun 23 2022, 7:45 AM
gribozavr2 added a subscriber: gribozavr2.
gribozavr2 added inline comments.
clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
179

Please clang-format before submitting (it looks like this block is indented 4 instead of 2).

204–205
208–211

Canonical => Can is the usual abbreviation, if you want to use one (e.g., see CanQualType)

218–219
230

Cond is not the loop itself.

This revision is now accepted and ready to land.Jun 23 2022, 7:45 AM
LegalizeAdulthood requested changes to this revision.Jun 23 2022, 10:38 AM

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto main:HEAD and:

  • fold your changes into the appropriate subdirs, stripping the module prefix from new files.
  • make the target check-clang-extra to validate your tests
  • make the target docs-clang-tools-html to validate any docs changes
This revision now requires changes to proceed.Jun 23 2022, 10:38 AM

rebased with the latest main:HEAD where clang-tidy file structure has changed

Addressing Dmitri's comments.

gribozavr2 added inline comments.Jun 23 2022, 5:56 PM
clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
204–205

It seems like you missed this one.

NoQ added a subscriber: NoQ.Jun 23 2022, 11:38 PM
NoQ added inline comments.
clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
54

Ok so this whole checker does this procedure involving ExprMutationAnalyzer trying to figure out if the variable can be changed from within the loop. I wonder if our problem of whether the static variable can be changed within the loop is a sub-problem of this problem and therefore could have a general solution inside ExprMutationAnalyzer. If so, other clang-tidy checks that use ExprMutationAnalyzer could immediately benefit from such solution. WDYT?

clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
210

Personally I'm not a fan of using bitwise operators with booleans. It involves implicit casts of bool to int and int to bool and doesn't use the usual short-circuiting logic of || and &&. It also means this code won't be analyzed by clang-tidy as "performing operations on booleans" for things like readability-simplify-boolean-expr.

addressing more comments

trying to be consistent in that auto is used wherever possible

Eugene.Zelenko added inline comments.Jun 24 2022, 2:06 PM
clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
180

Please don't use auto when type is not spelled explicitly or iterator.

207

Ditto.

I missed the part of coding standard about the usage of auto. Now change to conform to it.

ziqingluo-90 added inline comments.Jun 24 2022, 3:22 PM
clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
54

I think this is a good point. I can do it after this patch.

Hi @LegalizeAdulthood, @NoQ, @gribozavr2, @Eugene.Zelenko,

Is there any further issue I should address for this patch?

NoQ accepted this revision.Jul 15 2022, 3:14 PM

Looks great, thanks!

Yes, I think refactoring can be done in a follow-up patch.

Looks great, thanks!

Yes, I think refactoring can be done in a follow-up patch.

Thank you, @NoQ!

Let me politely ping the rest of the commenters: @LegalizeAdulthood, @gribozavr2, @Eugene.Zelenko. Is this patch looking good to you?

NoQ added a comment.Aug 24 2022, 6:33 PM

I think let's commit. @ziqingluo-90 addressed all existing concerns and promises follow-up patches where he'll have a chance to address future concerns as well.

njames93 accepted this revision.Aug 24 2022, 10:54 PM
njames93 added inline comments.
clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
186
214

This will always be false as if it's true, the loop would return.

ziqingluo-90 added inline comments.Aug 25 2022, 12:03 PM
clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
214

Thanks for pointing this out. I will make the change before commit!

This revision was not accepted when it landed; it landed in state Needs Review.Aug 29 2022, 11:21 AM
This revision was automatically updated to reflect the committed changes.