C++: Add StackVariable class, preferred over LocalScopeVariable#2381
C++: Add StackVariable class, preferred over LocalScopeVariable#2381geoffw0 merged 14 commits intogithub:masterfrom
Conversation
The new `StackVariable` class actually denotes what its name suggests.
This is now a copy of `LocalScopeVariableReachability.qll`, just with `s/LocalScopeVariable/StackVariable/g`. It can be used as a drop-in replacement since the `LocalScopeVariableReachability.qll` library implementation was already restricted to `SemanticStackVariable`.
This library is a drop-in replacement for `LocalScopeVariableReachability`, so no changes are expected.
This means we'll no longer get SSA definitions for thread-local local-scope variables.
Most of the implementation was already in terms of `SemanticStackVariable`, so not much should have changed.
In these files it was possible to remove calls to `isStatic` by switching from `LocalScopeVariable` to `StackVariable`. This changes semantics, hopefully for the better, to treat `thread_local` locals the same as `static` locals.
These changes should not affect semantics since these uses of `LocalScopeVariable` were already constrained to stack variables by their use of SSA or def-use.
This might cause fewer variables to be analysed because not every use of `LocalScopeVariable` was constrained by the def-use library. Hopefully this leads to an improved nullness analysis since it avoids treating `static T *x = nullptr;` the same as `static T *x; x = nullptr;`.
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Obviously this is a lot to review, but the improvements LGTM both in terms of library usability and potential improvements to query results.
I'd like to be reassured about the exact impact of these changes on our queries, and I have a small concern that there may be edge cases in SSA, def-use, reachability etc we haven't thought about. Perhaps a query differences run would be worthwhile?
jbj
left a comment
jbj
left a comment
There was a problem hiding this comment.
Yes, these changes could have all sorts of subtle implications. I'll start a dist compare.
It looks like allowing statics in `Nullness.qll` is fine since it's a "may be null" analysis rather than a "must be null" analysis. This reverts commit f5b9837.
Conflicts:
change-notes/1.24/analysis-cpp.md
cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql
|
I've run dist-compare, and there were no changes. That's always suspicious, so I've tried my very best to convince myself that I invoked dist-compare correctly. I first started https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/669, but one of its sub-jobs failed with a transient error, so I killed it. At that point it had built a pair of ODASAs: I started https://jenkins.internal.semmle.com/job/Changes/job/CPP-Differences/670 with the same parameters except that I put "6934,6935" in the LGTMDists field. This job finished and found no differences. The two ODASA jobs look correct to me: they have a qlSubmoduleShaOverride of d22df24 and 763b18c respectively. |
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Great, I'm happy with this!
2 participants
Fixes #2352.
This is a very extensive fix for a very small bug, but it's the second time we had reports about a FP due to
thread_local, and similar problems might exist in 5-10 other queries. This PR attempts to address them all at once while at the same time reducing our use of the awkardly-namedLocalScopeVariableandLocalScopeVariableReachabilityclasses.