- User Since
- Sep 17 2012, 3:16 AM (505 w, 5 d)
Thu, May 26
Tue, May 24
An alternative approach is to maintain separate counters for back edges and bail if those reach a certain limit. But this global limit is way simpler, so it looks good to me.
Mon, May 23
I prefer references to annotations, but this is also a step in the right direction :l
Fri, May 20
Thu, May 19
Actually, I think in most cases we want to consistent how to merge bool values. So I wonder whether instead of reimplementing the merge operation in this check we should just call a function that does the work. And the same function should be used within the engine to merge states after if statements and so on.
Tue, May 17
While not having tests might be OK, but I'd prefer to introduce at least a couple uses of the new facilities so existing tests cover them.
Fri, May 13
Wed, May 11
Overall looks good to me. I wish some parts would be simpler but it looks like sometimes it is not easy to extend the current code and we might need to do some refactoring at some point.
Fri, May 6
Thanks, this looks good to me!
This approach fixes the worklist for the second phase. Would it be possible to create a wrapper that reverses the order of any worklist instead of committing to one and hardcode that?
Thu, May 5
It looks like all of my comments are resolved now, thanks!
Overall this looks good to me. However, I think this might not use the full potential of the check itself. With the information that the dataflow framework have it could distinguish between potentially unsafe accesses and provably unsafe accesses depending on whether the has_value property is constrained to be false. From the user point of view, it would be nice to emit different warning messages for the above two cases. This can help to gradually introduce this check to a larger codebase and focus on the higher severity diagnostics first.
Most checks in Clang Tidy will run relatively quickly as they usually can do most/all of their work in a single traversal. I wonder whether flow sensitive checks will prove to be a bit slower. I think adding slower checks to Tidy is fine, but it would be nice to properly set the expectations to the user (e.g. if an IDE is running Tidy in the background it might want to opt out from flow sensitive checks if they turn out to be slow). In case the performance is good, I think it should be fine as is. Otherwise I wonder if we want to add something like a tag to mark flow sensitive checks and give an option to turn them off.
Thanks for the clarifications!
Wed, May 4
The code looks good to me too. I was also wondering what sort of check will need this info.
Overall looks good to me. I am curious what will the strategy be to properly support construction. Do you plan to introduce a customization point to Env.createValue to give checks/models a way to set properties up? Or do you have something else in mind?
Tue, May 3
Fri, Apr 29
Apr 27 2022
Apr 25 2022
Nice! Did you do some measurements? Does this improve the performance or decrease the memory consumption?
Apr 22 2022
Apr 20 2022
Apr 19 2022
Apr 14 2022
> Make CTU a two phase analysis
Apr 13 2022
Apr 12 2022
Yeah, this is a hard problem in general. This looks like a sensible workaround for the short term, but I'm looking forward to a better solution. I'm a bit worried that the memory model will need some upgrades to properly solve this problem.
Apr 7 2022
Apr 5 2022
The changes look good to me but please wait at least one more reviewer before committing.
Adding some reviewers
Apr 4 2022
Apr 1 2022
Speaking of builtins, it might be great to add tests for __builtin_unreachable, __builtin_trap, __builtin_debugtrap. The CFG might already have the right shape so we might not need to add any code to support them. But it would be nice to know :) Maybe we could even add a comment to VisitCallExpr why those wouldn't need explicit support.
Mar 31 2022
Mar 29 2022
Wow. This did take some iterations and I feel like I just added to the confusion at some point :D But the latest iteration looks much simpler and I'm confident it is right this time. Thanks!
Mar 25 2022
Mar 23 2022
Mar 21 2022
Thanks! Did you have a chance whether this makes a difference in real world scenarios? I'm mostly curious because I do not have a good mental model of how the matchers are implemented, specifically what optimizations are in place, so I don't really know how much of an impact can caching make :)
Are smart pointers special? I would expect to see similar problems with containers (or even a nested optional). I wonder whether an allowlist instead of a denylist approach is better here. E.g., instead of disabling the modeling for smart pointers, we could enable it for cases that we actually support (or alternatively, we could add a confidence value to the unsafe access). Usually, these checks are pretty robust when we deal with objects on the stack of the analyzed function (locals, parameters), but it is really hard to reason about objects from the outside (e.g., when a reference to an object is acquired from a container or smart pointer) unless we have explicit modeling for the APIs. The confidence approach might be useful as we are unlikely to cover all the custom smart pointers the users have.
Mar 18 2022
Mar 17 2022
Mar 16 2022
Mar 15 2022
The change itself looks good. But out of curiosity, could you give me an example when we do not want to use the builtin transfer functions?
Mar 14 2022
Mar 11 2022
Mar 10 2022
Mar 8 2022
Mar 7 2022
When pre-initializing fields in the environment, the code assumed that all fields of a struct would be initialized