If we have an unknown value do not treat it as known.
Details
Diff Detail
Event Timeline
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.
- The report was too misleading.
- It turns out we have to keep non-nulls.
- Now we do not treat unknown values as known.
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):
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). |
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. |
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. |
Here.