Partial fix for 9049: False negative: uninitialized variable with nested ifs#8680
Partial fix for 9049: False negative: uninitialized variable with nested ifs#8680pfultz2 wants to merge 34 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (Token::Match(ftok, "exit|abort")) | ||
| return true; |
There was a problem hiding this comment.
There is also terminate() but it feels like should already be handled via the noreturn handling below (i.e. library configuration).
| const bool inElse = scope->type == ScopeType::eElse; | ||
| const bool inDoWhile = scope->type == ScopeType::eDo; | ||
| const bool inLoop = contains({ScopeType::eDo, ScopeType::eFor, ScopeType::eWhile}, scope->type); | ||
| const bool hasElse = Token::simpleMatch(tok, "} else {"); |
There was a problem hiding this comment.
Since it is grouped with other similar variables this is fine but in a future we should try to reduce the scope of this (i.e. avoid computing them until they are used).
There was a problem hiding this comment.
It was moved here so its not calculated multiple times. Probably could use the memoize function to lazily compute it once, but I think its beyond the scope here because I need to evaluate if its slower or not.
There was a problem hiding this comment.
I was referring to moving them after the if (!condTok) check. But as also mentioned no need to do so.
|
The comments and the description appear to hint at AI assistance. Is that the case? |
|
|
| std::size_t operator()(const std::tuple<const Token*, const Token*, const Token*>& t) const { | ||
| const std::hash<const Token*> h; | ||
| std::size_t seed = h(std::get<0>(t)); | ||
| seed ^= h(std::get<1>(t)) + 0x9e3779b9 + (seed << 6) + (seed >> 2); |
| const std::hash<const Token*> h; | ||
| std::size_t seed = h(std::get<0>(t)); | ||
| seed ^= h(std::get<1>(t)) + 0x9e3779b9 + (seed << 6) + (seed >> 2); | ||
| seed ^= h(std::get<2>(t)) + 0x9e3779b9 + (seed << 6) + (seed >> 2); |
| vfOptions.doConditionExpressionAnalysis = true; | ||
| vfOptions.maxForwardBranches = -1; | ||
| vfOptions.maxForwardConditionForkDepth = 4; | ||
| vfOptions.maxForwardConditionForks = 256; |
| int maxForwardConditionForkDepth = 4; | ||
|
|
||
| /** @brief Maximum total condition-fork continuations in one forward traversal. */ | ||
| int maxForwardConditionForks = 256; |
Ok so I have improved the performance quite a bit: First, it implements a cache for There can still be an explosion of forked paths here and we are using the very slow exhaustive option in CI. I still limit it to a depth of 4 on exhaustive and a limit of 256 forks when forward. Once it reaches the limit it fallbacks to updating in the main analyzer rather than forking. For I still think exhaustive should probably use much higher limits here. I am thinking we should add an @danmar @chrchr-github Any feedback? |
|
Since it's quite a large change, can you run test-my-pr.py? daca shows numerous changes already in this dev cycle. |
Is there instructions somewhere on how to run it? I get this error when I try to do |
|
|
I ran over 50 packages, these are the warning diff I saw: The |
|
Yeah, those seem like recent changes unrelated to this PR. So the script doesn't use current head as the baseline for some reason. |
|
So I ran it over 200 packages and I did see one new message: But this is a true positive. |
4 participants
This fixes the case for this:
Which doesnt use a compund condition
dimensions >= 1 && b)and it also requires complete variables and functions. The compund condition could be handled in the future by forking within the condition, soif(a && b) { ... }can be treated asif(a) { if(b) { ... }}.Here is a summary of the changes: