Page MenuHomePhabricator

[analyzer] trackExpressionValue(): Handle unknown values better
Needs ReviewPublic

Authored by Charusso on Jun 6 2019, 1:33 PM.

Details

Summary

If we have an unknown value do not treat it as known.

Diff Detail

Event Timeline

Charusso created this revision.Jun 6 2019, 1:33 PM

Suppressed example:

NoQ added a comment.Jun 6 2019, 8:47 PM

Ah, that positive!

No, i don't think this is a valid way to suppress it. I'll tease you a bit more though, and only give a hint: the null value that we're null-dereferencing was in fact assumed to be null in the top frame, not within any inline function. Therefore the whole "Returning pointer..." note is completely misleading: it is irrelevant what value we return from getSuperRegion(), we might have as well evaluated it conservatively, the report would still have been emitted.

Charusso updated this revision to Diff 203876.Jun 10 2019, 12:50 PM
Charusso retitled this revision from [analyzer] ReturnVisitor: Handle non-null ReturnStmts to [analyzer] ReturnVisitor: Handle unknown ReturnStmts better.
Charusso edited the summary of this revision. (Show Details)
  • The report was too misleading.
  • It turns out we have to keep non-nulls.
  • Now we do not treat unknown values as known.
In D62978#1533558, @NoQ wrote:

Ah, that positive!

Positive == true positive, not false positive, I got it.

No, i don't think this is a valid way to suppress it.

Bought me, they are worth to report. The misleading reports made me think I have to suppress them.

Example (difference starts at note 33, at line 410):


NoQ added inline comments.Jun 10 2019, 6:39 PM
clang/test/Analysis/diagnostics/find_last_store.c
9–10

This remaining note is also unnecessary. You can safely stop tracking the value at e || .... In particular, ReturnVisitor is not at fault.

That said, when we renamed trackNullOrUndefValue to trackExpressionValue, we kinda assumed that it's not really important whether the value is null/undef or not - the function simply tracks the value. This change would break this invariant, causing null values to be handled in a special manner. I recommend adding another flag argument (similar to EnableNullFPSuppression) that would allow the caller to tell whether it's interested in the null or in the "whole" value (defaulting to the latter).

Charusso updated this revision to Diff 204113.Jun 11 2019, 11:01 AM
Charusso retitled this revision from [analyzer] ReturnVisitor: Handle unknown ReturnStmts better to [analyzer] trackExpressionValue(): Handle unknown values better.
Charusso edited the summary of this revision. (Show Details)
Charusso marked 8 inline comments as done.Jun 11 2019, 11:09 AM
Charusso added inline comments.
clang/test/Analysis/diagnostics/deref-track-symbolic-region.c
14

Here.

clang/test/Analysis/diagnostics/find_last_store.c
9–10

That is a great idea! I tried my best but after a while I give up because it is already too complex, so I just removed them. It turns out it makes sense to remove those notes, see inline.

clang/test/Analysis/diagnostics/macro-null-return-suppression.cpp
53

Here.

clang/test/Analysis/loop-widening-notes.cpp
12

Here.

clang/test/Analysis/uninit-const.cpp
59

Here we mismatch the parameter.

NoQ added a comment.Jun 11 2019, 6:26 PM

This problem is fairly complicated. We clearly need both behaviors (sometimes track until the definition, sometimes track until the collapse-to-null), and it's not clear to me right now when exactly do we need each of them. This is also not a high priority for GSoC because neither there are a lot of warnings of this kind (~15 or so) nor they're actually that false. I suggest taking this more slowly and delay this patch until we actually understand what is the right thing to do here.

clang/test/Analysis/null-deref-path-notes.cpp
20

This note was pretty good. It's not very clear but we should definitely keep it.

clang/test/Analysis/uninit-vals.m
405

These notes are also nice, we should keep them.

Szelethus set the repository for this revision to rC Clang.Jun 16 2019, 8:44 AM