forked from llvm/llvm-project
-
Notifications
You must be signed in to change notification settings - Fork 2
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[analyzer] Clean up apiModeling.llvm.ReturnValue (llvm#91231)
This commit heavily refactors and simplifies the small and trivial checker `apiModeling.llvm.ReturnValue`, which is responsible for modeling the peculiar coding convention that in the LLVM/Clang codebase certain Error() methods always return true. Changes included in this commit: - The call description mode is now specified explicitly (this is not the most significant change, but it was the original reason for touching this checker). - Previously the code provided support for modeling functions that always return `false`; but there was no need for that, so this commit hardcodes that the return value is `true`. - The overcomplicated constraint/state handling logic was simplified. - The separate `checkEndFunction` callback was removed to simplify the code. Admittedly this means that the note tag for the "<method> returns false, breaking the convention" case is placed on the method call instead of the `return` statement; but that case will _never_ appear in practice, so this difference is mostly academical. - The text of the note tags was clarified. - The descriptions in the header comment and Checkers.td were clarified. - Some minor cleanup was applied in the associated test file. This change is very close to NFC because it only affects a hidden `apiModeling.llvm` checker that's only relevant during the analysis of the LLVM/Clang codebase, and even there it doesn't affect the normal behavior of the checker.
- Loading branch information
Showing
3 changed files
with
79 additions
and
145 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,91 +1,91 @@ | ||
| // RUN: %clang_analyze_cc1 \ | ||
| // RUN: -analyzer-checker=core,apiModeling.llvm.ReturnValue \ | ||
| // RUN: -analyzer-output=text -verify=class %s | ||
| // RUN: -analyzer-output=text -verify %s | ||
|
|
||
| struct Foo { int Field; }; | ||
| bool problem(); | ||
| void doSomething(); | ||
|
|
||
| // We predefined the return value of 'MCAsmParser::Error' as true and we cannot | ||
| // take the false-branches which leads to a "garbage value" false positive. | ||
| namespace test_classes { | ||
| // Test the normal case when the implementation of MCAsmParser::Error() (one of | ||
| // the methods modeled by this checker) is opaque. | ||
| namespace test_normal { | ||
| struct MCAsmParser { | ||
| static bool Error(); | ||
| }; | ||
|
|
||
| bool parseFoo(Foo &F) { | ||
| if (problem()) { | ||
| // class-note@-1 {{Assuming the condition is false}} | ||
| // class-note@-2 {{Taking false branch}} | ||
| // expected-note@-1 {{Assuming the condition is false}} | ||
| // expected-note@-2 {{Taking false branch}} | ||
| return MCAsmParser::Error(); | ||
| } | ||
|
|
||
| F.Field = 0; | ||
| // class-note@-1 {{The value 0 is assigned to 'F.Field'}} | ||
| return !MCAsmParser::Error(); | ||
| // class-note@-1 {{'MCAsmParser::Error' returns true}} | ||
| // expected-note@-1 {{The value 0 is assigned to 'F.Field'}} | ||
| return false; | ||
| } | ||
|
|
||
| bool parseFile() { | ||
| Foo F; | ||
| if (parseFoo(F)) { | ||
| // class-note@-1 {{Calling 'parseFoo'}} | ||
| // class-note@-2 {{Returning from 'parseFoo'}} | ||
| // class-note@-3 {{Taking false branch}} | ||
| // expected-note@-1 {{Calling 'parseFoo'}} | ||
| // expected-note@-2 {{Returning from 'parseFoo'}} | ||
| // expected-note@-3 {{Taking false branch}} | ||
| return true; | ||
| } | ||
|
|
||
| // The following expression would produce the false positive report | ||
| // "The left operand of '==' is a garbage value" | ||
| // without the modeling done by apiModeling.llvm.ReturnValue: | ||
| if (F.Field == 0) { | ||
| // class-note@-1 {{Field 'Field' is equal to 0}} | ||
| // class-note@-2 {{Taking true branch}} | ||
|
|
||
| // no-warning: "The left operand of '==' is a garbage value" was here. | ||
| // expected-note@-1 {{Field 'Field' is equal to 0}} | ||
| // expected-note@-2 {{Taking true branch}} | ||
| doSomething(); | ||
| } | ||
|
|
||
| // Trigger a zero division to get path notes: | ||
| (void)(1 / F.Field); | ||
| // class-warning@-1 {{Division by zero}} | ||
| // class-note@-2 {{Division by zero}} | ||
| // expected-warning@-1 {{Division by zero}} | ||
| // expected-note@-2 {{Division by zero}} | ||
| return false; | ||
| } | ||
| } // namespace test_classes | ||
| } // namespace test_normal | ||
|
|
||
|
|
||
| // We predefined 'MCAsmParser::Error' as returning true, but now it returns | ||
| // false, which breaks our invariant. Test the notes. | ||
| // Sanity check for the highly unlikely case where the implementation of the | ||
| // method breaks the convention. | ||
| namespace test_break { | ||
| struct MCAsmParser { | ||
| static bool Error() { | ||
| return false; // class-note {{'MCAsmParser::Error' returns false}} | ||
| return false; | ||
| } | ||
| }; | ||
|
|
||
| bool parseFoo(Foo &F) { | ||
| if (problem()) { | ||
| // class-note@-1 {{Assuming the condition is false}} | ||
| // class-note@-2 {{Taking false branch}} | ||
| // expected-note@-1 {{Assuming the condition is false}} | ||
| // expected-note@-2 {{Taking false branch}} | ||
| return !MCAsmParser::Error(); | ||
| } | ||
|
|
||
| F.Field = 0; | ||
| // class-note@-1 {{The value 0 is assigned to 'F.Field'}} | ||
| // expected-note@-1 {{The value 0 is assigned to 'F.Field'}} | ||
| return MCAsmParser::Error(); | ||
| // class-note@-1 {{Calling 'MCAsmParser::Error'}} | ||
| // class-note@-2 {{Returning from 'MCAsmParser::Error'}} | ||
| // expected-note@-1 {{'MCAsmParser::Error' returned false, breaking the convention that it always returns true}} | ||
| } | ||
|
|
||
| bool parseFile() { | ||
| Foo F; | ||
| if (parseFoo(F)) { | ||
| // class-note@-1 {{Calling 'parseFoo'}} | ||
| // class-note@-2 {{Returning from 'parseFoo'}} | ||
| // class-note@-3 {{Taking false branch}} | ||
| // expected-note@-1 {{Calling 'parseFoo'}} | ||
| // expected-note@-2 {{Returning from 'parseFoo'}} | ||
| // expected-note@-3 {{Taking false branch}} | ||
| return true; | ||
| } | ||
|
|
||
| (void)(1 / F.Field); | ||
| // class-warning@-1 {{Division by zero}} | ||
| // class-note@-2 {{Division by zero}} | ||
| // expected-warning@-1 {{Division by zero}} | ||
| // expected-note@-2 {{Division by zero}} | ||
| return false; | ||
| } | ||
| } // namespace test_classes | ||
| } // namespace test_break |