This is an archive of the discontinued LLVM Phabricator instance.

[Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker
ClosedPublic

Authored by ziqingluo-90 on Jun 21 2022, 5:09 PM.

Details

Summary

Fixing the bug that there is missing handling of noreturn attributes for ObjC nodes. If infinite loop checker sees a call to a function with a noreturn attribute, it shall never report a warning.

Diff Detail

Event Timeline

ziqingluo-90 created this revision.Jun 21 2022, 5:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2022, 5:09 PM
ziqingluo-90 requested review of this revision.Jun 21 2022, 5:09 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 21 2022, 5:09 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Can this patch be split in two, it seems strange to be fixing 2 unrelated bugs in one patch.
One fix for the ObjC nodes and another for the patch for the static local variables.

Also please can you run git clang-format over this patch.

clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
164–169

nit:

172–177

Nit: as above

179–184

Nit:

Also sometimes, some of the stmts in children can be null and that's expected, is there any reason we are disregarding that case.

195–205

This can be rewritten as a range for

for (CallGraphNode* GNode : SCC) ...
201
215–218
ziqingluo-90 retitled this revision from [Clang-tidy] Fixing bugs in clang-tidy infinite-loop checker to [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker.
ziqingluo-90 edited the summary of this revision. (Show Details)

Separate the change adding handling of noreturn attributes for ObjC nodes. Have run clang-format.

NoQ added a comment.Jun 23 2022, 9:44 AM

Nice!

There's usually some bureaucracy when creating new matchers, i.e. there should be documentation and unittests covering them.

clang/include/clang/ASTMatchers/ASTMatchers.h
5099

Can we make a polymorphic matcher hasNoReturnAttr that can accept either decl or type?

LegalizeAdulthood requested changes to this revision.Jun 23 2022, 10:40 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:40 AM

adjusted my changes with respect to the recent file structure changes in clang-tidy test

I saw @aaron.ballman 's comment "We typically don't add AST matchers until we have a need for them to be used in-tree (ASTMatchers.h is already really expensive to parse; adding matchers for everything possible with AST nodes would be prohibitively expensive)." in this patch. So I think the matchers for testing noreturn attributes is too specific to be in the ASTMatcher.h. Moving them to where they are used. While the objcMessageCallee is sort of the Obj-C counterpart of the callee matcher, so I think it is general enough to stay.

adding a unit test for the ASTMatcher objcMessageCallee added in ASTMatcher.h

Nice!

There's usually some bureaucracy when creating new matchers, i.e. there should be documentation and unittests covering them.

I have added a unit test and auto-generated document for it.

clang/include/clang/ASTMatchers/ASTMatchers.h
5099

I've moved these two matchers, which seems are specific to the infinite loop checker, to InfiniteLoopCheck.cpp. Maybe they no longer need to be generalized by polymorphism.

NoQ added a comment.Jul 5 2022, 10:16 PM

Sounds great! I have a nitpick but other than that I think this fix is good to go.

clang/include/clang/ASTMatchers/ASTMatchers.h
3832

It looks like documentation traditionally starts with a capital letter and ends with .

njames93 added a subscriber: klimek.Jul 7 2022, 8:07 AM

Sorry to do this again, but could this be split up again, one patch for the new matcher and the tests associated with it, then another for the actual bug fix.
Also cc @klimek as he is the code owner of ASTMatchers

Sorry to do this again, but could this be split up again, one patch for the new matcher and the tests associated with it, then another for the actual bug fix.
Also cc @klimek as he is the code owner of ASTMatchers

Makes sense. I have created a new review request for the ASTMatcher. This patch now depends on it.

NoQ accepted this revision.Jul 10 2022, 4:20 PM

Thanks! Good to go after the matchers patch is committed.

The callee ASTMatcher overloading patch has landed in LLVM repo. I update this patch to use callee for matching objective-C message callee methods.

NoQ accepted this revision.Aug 24 2022, 6:34 PM

Still looks good to me, let's commit!

This revision was not accepted when it landed; it landed in state Needs Review.Aug 26 2022, 12:45 PM
This revision was automatically updated to reflect the committed changes.