Szelethus accepted this revision. Szelethus marked 2 inline comments as done. Szelethus added a comment.
LGTM! Mind that I just published D76917 <https://reviews.llvm.org/D76917>, you can, if you prefer, rebase on top of that. Also, a test case for `delete`ing a `ZERO_SIZE_PTR` valued pointer might be nice :) If you prefer, feel free to commit without another round of reviews. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:396 + /// yes was the value obtained or not. + mutable Optional<Optional<int>> KernelZeroSizePtrValue; + ---------------- balazske wrote: > martong wrote: > > balazske wrote: > > > martong wrote: > > > > Which one is referred to the lazy initialization? The inner or the > > > > outer? > > > > These questions actually made me to come up with a more explanatory > > > > construct here: > > > > Could we do something like this? > > > > ``` > > > > using LazyInitialized = Optional<int>; > > > > mutable Optional<LazyInitialized> KernelZeroSizePtrValue; // Or > > > > Lazy<Optional<...>> > > > > ``` > > > Probably use a `std::unique_ptr<Optional<int>>` instead (like at the bug > > > types, they could be optional too)? > > > If there is a code like > > > ``` > > > bool IsSomethingInitialized; > > > int Something; > > > ``` > > > that looks as a clear case to use an optional (or unique_ptr)? And if > > > yes, is a reason for not using this construct if `int` is replaced by > > > `Optional<int>`? > > Now I see that the lazy initialization is represented by the outer optional. > > So IMHO a using template could be the best to describe cleanly this > > construct: > > ``` > > template <class T> > > using LazyInitialized = Optional<T>; > > mutable LazyInitialized<Optional<int>> KernelZeroSizePtrValue; > > ``` > > > With this I would wait for opinion of somebody else. What @martong is saying is indeed nice, but the comments explain whats happening as well -- I'll leave it up to you how you want to commit this. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1687 + if (ArgValKnown) { + if (!KernelZeroSizePtrValue) + KernelZeroSizePtrValue = ---------------- balazske wrote: > Szelethus wrote: > > balazske wrote: > > > Szelethus wrote: > > > > Szelethus wrote: > > > > > martong wrote: > > > > > > balazske wrote: > > > > > > > martong wrote: > > > > > > > > martong wrote: > > > > > > > > > This is a bit confusing for me. Perhaps alternatively we > > > > > > > > > could have a free function `isInitialized(KernelZero...)` > > > > > > > > > instead. Or maybe having a separate bool variable to indicate > > > > > > > > > whether it was initialized could be cleaner? > > > > > > > > Another idea: Adding a helper struct to contain the bool > > > > > > > > `initialized`? E.g. (draft): > > > > > > > > ``` > > > > > > > > struct LazyOptional { > > > > > > > > bool initialized = false; > > > > > > > > Opt<int> value; > > > > > > > > Opt& get(); > > > > > > > > void set(const Opt&); > > > > > > > > }; > > > > > > > > ``` > > > > > > > It may be OK to have a function `lazyInitKernelZeroSizePtrValue`? > > > > > > I don't insist, given we have a better described type for > > > > > > `KernelZeroSizePtrValue` (e.g. having a `using` `template`) > > > > > `AnalysisManager` has access to the `Preprocessor`, and it is also > > > > > responsible for the construction of the `CheckerManager` object. This > > > > > would make moving such code to the checker registry function! I'll > > > > > gladly take this issue off your hand and patch it in once > > > > > rG2aac0c47aed8d1eff7ab6d858173c532b881d948 settles :) > > > > Just pushed rG4dc8472942ce60f29de9587b9451cc3d49df7abf. It it settles > > > > (no buildbots complain), you could rebase and the lazy initialization > > > > could be a thing of the past! :) > > > This makes it possible to have a generic one-time initialization function > > > in the checker (`CheckerBase`)? The functionality is needed in more than > > > one checker, at least for the bug types it can be used in almost every > > > checker (that has bugtypes). > > I'm not sure I understand what you mean -- do you want to add > > `CheckerManager` to `CheckerBase`'s constructor? > > > > In any case, I remember reading something around `CheckerBase`'s or > > `BugType`'s implementation the way the checker receives its name is a bit > > tricky and not a part of the construction. > Initialization in register function does not work: The `MacroInfo` is not > found in the preprocessor. Probably the code is not parsed normally at that > time. For now only the lazy initialization is working. > Initialization in register function does not work: The MacroInfo is not found > in the preprocessor. Probably the code is not parsed normally at that time. > For now only the lazy initialization is working. Well that is super annoying. I don't object to the lazy initialization then. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1692-1694 + // If there is a macro called ZERO_SIZE_PTR, it could be a kernel source code + // and this value indicates a special value used for a zero-sized memory + // block. It is a constant value that is allowed to be freed. ---------------- Just a bit of word smithing :) If the macro ZERO_SIZE_PTR is defined, this could be a kernel source code. In that case, the ZERO_SIZE_PTR defines indicates a special value used for a zero-sized memory block which is allowed to be freed, despite not being a null pointer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76830/new/ https://reviews.llvm.org/D76830 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
