Page MenuHomePhabricator

[clang] Introduce while and do..while condition checks
Needs ReviewPublic

Authored by stryku on Apr 11 2022, 11:40 AM.

Details

Reviewers
rtrieu
Summary

There are various checks implemented for condition expression in for loop. I thought that we could have similar in while and do..while loops. Curious what you think.

Diff Detail

Repository
rC Clang

Event Timeline

stryku created this revision.Apr 11 2022, 11:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 11:40 AM
Herald added a subscriber: arphaman. · View Herald Transcript
stryku requested review of this revision.Apr 11 2022, 11:40 AM
stryku retitled this revision from Introduce while and do..while condition checks to [clang] Introduce while and do..while condition checks.Apr 11 2022, 11:45 AM

Cool, this conceptually makes a lot of sense to me! That said, I'm not a competent reviewer in this area anymore, @klimek can you recommend someone?

Great, I can see some builds have failed. Will do my best to fix that tomorrow.

Sorry for the delay of fixing failed builds. Full builds and tests take some time. I'm working on it, promise.
The code itself won't change much, so I think it can be reviewed. Or at least the idea can be judged if it's something desired to have in clang. @lattner or @klimek could you please recommend someone to ask for review?

Well, unfortunately I'm unable to reproduce the failed builds. Everything passes for me locally. @lattner or @klimek, or someone who you think is a good candidate to review this code - could you please try to test it?
Could you please assist me how to proceed with this patch?

rsmith edited reviewers, added: rtrieu; removed: lattner.May 26 2022, 2:09 PM
rsmith added a subscriber: lattner.
rsmith added a subscriber: rsmith.May 26 2022, 2:20 PM
rsmith added inline comments.
clang/lib/Sema/SemaStmt.cpp
1851–1852

Please sink this variable down to the point where we've decided we're going to emit a diagnostic, and switch from PDiag to Diag.

1851–1888
2118
stryku updated this revision to Diff 432797.EditedMay 29 2022, 10:08 AM

Thank you for review! Implemented the changes

stryku marked 3 inline comments as done.May 30 2022, 12:31 PM

I think clang-format is yelling at places where formatting should not be applied, so I'm gonna ignore that.

rtrieu added a comment.Wed, Jun 1, 5:34 PM

I wrote the original for loop warning. I did try to extend it to while loops, but due to the different usage of these types of loops, there was a higher false positive rate for while loops that in for loops. For loops are generally self-contained, so checking only the loop and loop body is a good way to determine if the condition is changed. However, while loops are also used in things like multi-threaded code and the while loop is used as a wait.

void Run() {
  bool Wait = true;
  Execute(&Wait);
  while (Wait) {
    // Do some waiting
  }
}

To lower the false positive rate, I tried to add a bool to Decl's to track if they are possible referenced from other places. To do that, Sema needs to be changed to track that bit. Unfortunately, the Sema change was not accepted, so without a way to lower the false positive rate, I abandoned the warning.

Links to old discussions on mailing lists:
Patch to add warning to while loops
https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20120604/058677.html
Patch to add reference bit to Decl
https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20121112/068241.html

There's a few ways forward if you want to keep working on this warning.

  1. Run your version of the warning over a large codebase to get more recent data on true versus false positives. If the numbers have changed since last time, it makes for a greater argument to have this warning.
  2. Find another method to reduce the false positive rate, specifically, examples as listed above.
  3. Instead of making it a warning, implement it in static analysis as mentioned in the review of the original patch
stryku added a comment.Thu, Jun 2, 1:10 PM

Thank you for such a detailed comment. Makes sense. I need to think how to proceed. Will ping you when I figure something out.