Page MenuHomePhabricator

[analyzer] Fix accidentally skipping the call during inlined defensive check suppression.
ClosedPublic

Authored by NoQ on Sep 23 2019, 2:22 PM.

Details

Summary

Got myself distracted with a tiny fix for the inlined defensive check suppression.

When bugreporter::trackExpressionValue() is invoked on a DeclRefExpr, it tries to do most of its computations over the node in which this DeclRefExpr is computed, rather than on the error node (or whatever node is stuffed into it). I'm quite confused about the idea behind it and i highly doubt that it actually works correctly, but one reason why we can't simply use the error node may be that the binding to that variable might have already disappeared from the state by the time the bug is found.

This time i noticed that for the inlined defensive checks visitor the DeclRefExpr node is in fact sometimes too early: the call in which the inlined defensive check has happened might have not been entered yet.

Therefore i change the visitor to be fine with tracking dead symbols (which it is totally capable of - the collapse point for the symbol is still well-defined), and fire it up directly on the error node. I still use "LVState" to find out which value should we be tracking, so there shouldn't be any problems with accidentally loading an ill-formed value from a dead variable.

I hope it's one tiny piece of understanding that'll bring us to a better architecture of this code.

Diff Detail

Event Timeline

NoQ created this revision.Sep 23 2019, 2:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2019, 2:23 PM

Hmm, so before this patch, we just used LVNode everywhere and ignored InputNode. It may not have made much sense, but its still not as confusing as using both if the creation of LVNode is unnecessary overall. Could we just remove it?

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1917–1927

When bugreporter::trackExpressionValue() is invoked on a DeclRefExpr, it tries to do most of its computations over the node in which this DeclRefExpr is computed, rather than on the error node (or whatever node is stuffed into it). I'm quite confused about the idea behind it and i highly doubt that it actually works correctly, but one reason why we can't simply use the error node may be that the binding to that variable might have already disappeared from the state by the time the bug is found.

So, its possible that this function, and its uses in trackExpressionValue is completely unnecessary?

2012–2014

Too late in terms of too close to the root of the ExplodedGraph, or the other way around? This is a bitconfusing since visitors are inspecting the graph in reverse.

NoQ marked an inline comment as done.Sep 23 2019, 5:40 PM

Yeah, i think we should avoid such peeking and instead try to do everything in one pass. I.e., if we need to peek at the node above us, just make a visitor that delays the decision until it has precisely the information it needs. I guess i'll be slooowly moving in this direction.

Also every time i see special handling of DeclRefExprs i get pretty worried. It usually means that this code almost never works.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
1917–1927

Of course it's necessary! Why would you ever write a system of mutually recursive visitors and then never implement an ad-hoc visitor within one of these visitors that allows you to quadratically visit nodes while you visit nodes, before actually visiting them in the real visitor? (/sarcasm)

Szelethus accepted this revision.Sep 24 2019, 1:59 AM

If you could change that, it would be awesome! But since this revision has its own side effect, let's commit as-is. LGTM!

This revision is now accepted and ready to land.Sep 24 2019, 1:59 AM
Charusso accepted this revision.Oct 3 2019, 4:26 AM

[...] one reason why we can't simply use the error node may be that the binding to that variable might have already disappeared from the state by the time the bug is found.

Yes, that is pretty interesting. I have found error nodes which after the purge program point, so when we arbitrarily pick the last node to prevent false alarms we could pick the last node which has more information, 2-3 nodes earlier on the path. I am not sure whether this new approach would be useful here, other than that I like the idea.

This revision was automatically updated to reflect the committed changes.