User Details
- User Since
- Oct 10 2017, 8:01 AM (285 w, 2 d)
Feb 13 2023
Ping.
@fhahn Could you please take a look, if you have some time? Would be great to hear your thoughts.
Feb 1 2023
Hey guys, thank you for the review so far. But I am not sure if we have a full consensus yet.
@nikic Do you think that the arguments above are reasonable for this change?
Jan 27 2023
Oct 28 2022
Yeah, that means, we actually evaded a cast, am I right?
Oct 26 2022
- Address ymandel's comments
- Address ymandel's comments
Thanks for the assiduous review @ymandel !
Oct 25 2022
It is very good that we can get rid of the ParentMapContext because it was sub-optimal to build that up for all declaration contexts of the translation unit. Now we discover the parent-child relations for ourselves and only for the needed declaration contexts. This is the proper way to go.
PS: I'm not sure how/when we should get rid of the LocAsInteger and
represent this by a SymbolCast.
Maybe @ASDenysPetrov or @martong could help me review this.
I am not sure, if the ExprEngine::VisitCast is the proper place to add these new modifications and call SValBuilder's evalCast. I think it might be better positioned in RegionStoreManager::getBinding. Considering, we already do a cast evaluation for certain kind of memregions there before returning with the stored value. (Actually, this is again a legacy hack, which is needed because we do not emit all SymbolCasts and thus we store the SVals with an improper type).
if (const FieldRegion* FR = dyn_cast<FieldRegion>(R)) return svalBuilder.evalCast(getBindingForField(B, FR), T, QualType{});
Oct 21 2022
Ping
Thanks for the update. Nice Work!
- Rebase to latest llvm/main
- Add comments
- Assumption -> ConditionValue
- Use CRTP
- branchTransfer -> transferBranch
- Make just one simple test case for transferBranch
Oct 19 2022
Oct 18 2022
A similar situation could happen if we reinterpret cast pointers, etc. so the situation is not limited to conflicting function prototypes.
Hmm, seems like the conflicting prototype (i.e. the obsolescent use of zero parameters) is needed to reproduce the assertion failure. That makes me wonder, how does the redecl chain of b looks like? Is void b() chained with void b(int*), or are they represented independently from each other? I guess they form the same redecl chain. Which drives us to the next questions.
When the analyzer reaches the CallExpr b(&buffer) which FunctionDecl does it see? Is it b() or b(int*)? My bet, it sees and works with b(). Could we detect if the arguments of the CallExpr does not match the parameters of the FunctionDecl? And if that is the case, could we iterate through the redecl chain to find an appropriate matching FunctionDecl? That would be b(int*) in this case ... and the original bindArray() should work then.
LGTM
Oct 17 2022
Could you please elaborate? I don't see how to help you with it without seeing more details.
Thanks for patch and the many test cases.
I am happy with the change and with the result. However, this colorSpace related report is a bit concerning. Could you please check if colorSpace at step 1 is evaluated as Undefined? Could that happen? Or that is (should be) Unknown since that is a parameter of a top-level analyzed function? Did you have this report before your change? I think this would worth to be further analyzed, maybe just directing the analyzer to analyze only the QImage::convertedToColorSpace top-level function.
Yes, I agree, this bug-report is hard to follow. Seems like the colorSpace argument from step 1 is flowing through step 10. I wonder if colorSpace at step 1 is evaluated as Undefined? Could that happen? Did you have this report before your change?
Oct 15 2022
Oct 14 2022
- Changed to be more verbose: "arg" -> "argument"
- Fixed "less than" to "greater than"
- Added new tests
- Fixed typos
- Rebase
Oct 6 2022
The two checkers work together and 'apiModeling.StdCLibraryFunctions'
and its 'ModelPOSIX=true' option should be now a dependency of
checker 'alpha.unix.Stream'.
I don't see this case different to the unary expressions. Consider the unary minus for example. Let's say, the symbol a is constrained to [1, 1] and then -a is constrained to [-2, -2]. This is clearly an infeasible state and the analyzer will terminate the path. So, no further call of SValBuilder should happen from that point. That is, it is meaningless to evaluate -(-a) if there is a contradiction in the constraints that are assigned to the sub-symbols of the the symbol tree of -(-a).
Here is a test case that demonstrates this (this passes with latest llvm/main):
// RUN: %clang_analyze_cc1 %s \ // RUN: -analyzer-checker=core \ // RUN: -analyzer-checker=debug.ExprInspection \ // RUN: -analyzer-config eagerly-assume=false \ // RUN: -verify
Oct 3 2022
Thanks for the updates. I am okay with it now. LGTM. But please wait for NoQ's approval. So, this is a gentle ping for you @NoQ :)
Thanks for the update! LGTM.
I like the approach of this patch and I think this is somewhat aligned with @NoQ's ideas about
Thank you! Increasing coverage in tests is always great.
Sep 29 2022
Actually, you already have a logic for checking if there is a contradiction in your D103096 patch, in ConstraintAssignor::updateExistingConstraints:
// Update constraints in the map which bitwidth is equal or lower then // `MinBitWidth`. if (CM) { for (auto &Item : *CM) { // Stop after reaching a bigger bitwidth. if (Item.first > MinBitWidth) break; RangeSet RS = RangeFactory.intersect(Item.second, CastTo(Item.first)); // If the intersection is empty, then the branch is infisible. if (RS.isEmpty()) { State = nullptr; return false; } NewCM = CMF.add(NewCM, Item.first, RS); } }
Suppose we have found the way to handle it. But what if we find a contradiction while simplifying, like (short)(int x) = 0 and (int x) = 1. So that we would already know that it is impossible. What SVal should we return in such case? Undef? Unknown? Original one?
Sep 28 2022
LGTM, thanks!
Sep 27 2022
Yeah okay. I get it now. Thank you for your patience and your time on elaborating the issue.
Assume we have (int)(short)(int x). VisitSymbolCast will try to get the constant recursively in the next order:
- (short)(int x)
- (int x)
And here is my concern. Whether it is correct to get the range for int x and consider it as a correct simplification of (int)(short)(int x). IMO it can't be simplified like that. It should go through the set of conventions and intersections.
Sep 26 2022
We had a discussion about this with @dkrupp . We think that the p = realloc(p, var) construct in itself is an error-prone style of using realloc. This style does not necessarily give birth to erroneous code (see the error-free escaping example in foo above from @steakhal). However, the form p = realloc(p, var) is an indication that the programmer might missed the non-trivial error handling case, and chances are high. Thus, one should use the p2 = realloc(p1, var) form as a best practice. So, this looks good to me, but please consider this a weak approve and wait for someone else's approve who has more confidence in clang-tidy (@whisperity could you please take a look?).
Sep 22 2022
Sep 20 2022
Aligned with the RFC, I am going to dissect this patch into two patches:
Sep 16 2022
LGTM, Thanks!
Looks good to me!
Sep 15 2022
- Rebase
- move Msg into the lambda
Sep 14 2022
Okay, I don't see any problem with this change. LGTM.
Sep 12 2022
Sep 5 2022
LGTM, with nits.
Thanks, you have nice tests with good coverage! LGTM!
Sep 1 2022
LGTM
Aug 31 2022
I am okay with this change, it does give a proper canonical form, which is good. On the other hand, I agree that (on the long term) base regions and offsets would be a better memory model than what we have now with field and element regions.
LGTM!
Aug 30 2022
So I think the most valuable optimizations are low-level optimizations to ImmutableMap. There were a few suggestions on the mailing list to use something more modern than the AVL trees under the hood but I don't think authors found much success with those.
Aug 9 2022
@martong I kept your idea in mind for this warning message and implemented it as a new checker if you don't mind it. If the size of an array is Undefined when it's allocated, this patch emits a warning, and ends the analysis.
Thank you for taking care of this!
Aug 8 2022
Thank you for the update! LGTM!
Aug 4 2022
LGTM
Thank you, nice and assiduous work!
Thanks, looks good with some nits.
I think there's only some very minor style nits which are remained, but that should not block this any further. Let's land it.