Page MenuHomePhabricator

[Analyzer] Use of BugType in DereferenceChecker (NFC).
ClosedPublic

Authored by balazske on Jul 24 2020, 12:23 AM.

Details

Summary

Use of BuiltinBug is replaced by BugType.
Class BuiltinBug seems to have no benefits and is confusing.

Diff Detail

Event Timeline

balazske created this revision.Jul 24 2020, 12:23 AM
balazske updated this revision to Diff 280385.Jul 24 2020, 3:24 AM

Fixed failed tests because change of bug category from "Logic error" to "Memory error".

martong accepted this revision.Jul 27 2020, 9:07 AM

AFAIK, BuiltinBug is deprecated and we should be using BugType anyway.
LGTM!

This revision is now accepted and ready to land.Jul 27 2020, 9:07 AM
Szelethus accepted this revision.Jul 28 2020, 1:13 AM
Szelethus added reviewers: NoQ, vsavchenko.

Lets make sure that we invite others from the community to participate, here and on other patches as well. Otherwise, LGTM.

vsavchenko added inline comments.Jul 28 2020, 1:24 AM
clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
214

Maybe "is definitely null" here? Otherwise it can be pretty confusing with a double negation.

clang/test/Analysis/Inputs/expected-plists/conditional-path-notes.c.plist
270 ↗(On Diff #280385)

I might've missed some discussions on that matter, so please correct me on that.
In my opinion, null pointer dereference is not a memory error. Null address is not a correct memory and never was, so it is a logic error that a special value is being interpreted as the pointer to something.

balazske added inline comments.Jul 28 2020, 2:43 AM
clang/test/Analysis/Inputs/expected-plists/conditional-path-notes.c.plist
270 ↗(On Diff #280385)

I can not decide which is better, "logic error" can be said for almost every possible bug. Here the problem is at least related to memory handling. The reference of undefined pointer value is "logic error" too (it is known that the value was not initialized) but a memory error (try to access invalid or valid but wrong address). Probably "pointer handling error" is better?
(One value for bug category is not a good approach, it is never possible to classify a bug to exactly one.)

NoQ added a comment.Jul 28 2020, 7:04 PM

BuiltinBug is deprecated and we should be using BugType anyway

Like, i've never heard of BuiltinBug being deprecated but i'm pretty happy that it gets removed :)

clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
214

Also this entire comment should be inside the if-statement, on the same level as the next comment.

clang/test/Analysis/Inputs/expected-plists/conditional-path-notes.c.plist
270 ↗(On Diff #280385)

Maybe let's not change it then, if there are no clear pros or cons? Users already got used to it.

balazske updated this revision to Diff 281631.Jul 29 2020, 9:15 AM

Changed MemoryError back to LogicError.
Improved placement and text of changed comment.

balazske marked 2 inline comments as done.Jul 29 2020, 9:17 AM

The category string seems to be not really important, better not to change it.

vsavchenko accepted this revision.Jul 29 2020, 10:12 AM

Awesome, thanks!

NoQ accepted this revision.Jul 29 2020, 10:13 AM

Thank you!

This revision was automatically updated to reflect the committed changes.