User Details
- User Since
- Sep 3 2015, 9:16 AM (293 w, 5 d)
Today
Thu, Apr 8
I'm catching up and these changes look great.
Which is fairly shocking and mind blowing that we've organically developed two independent implementations of casting, one for RegionStore and one for everything else.
Looks great now, thanks!
Looks great now!
This looks amazing, thanks a lot.
Wed, Apr 7
Tue, Apr 6
Here's a reduced repro - a file that has different behavior before and after the patch (sorry, not perfectly reduced, my creduce is broken again):
// RUN: clang --analyze %s typedef Oid; typedef Pointer; typedef text; errstart(int elevel, filename, lineno, funcname, domain); typedef Datum; typedef struct { Oid elemtype; } ArrayType;
Great, thanks!~
Looks great and I'm also curious about nested initializers.
Looks great now!
I mean, the extent of an ElementRegion is the size of a single element. The reason why our intrinsic isn't doing what you expect is because we represent the pointer with offset as ElementRegion regardless of whether operator [] was used. On the other hand, an SVal doesn't ever represent a region at all, it only points to its first byte, regardless of the structure of MemRegion inside it; for that reason clang_analyzer_getExtent() is impossible to implement correctly in our current model.
More tests are great!
Mon, Apr 5
It does make sense to split these in two, but I'm not so sure about evalCall.
Thu, Apr 1
What about clang_analyzer_getExtent(&x[2]) then?
This will be obsoleted by D69726 because they are going to be the same symbol to begin with.
I don't see any further bugs so I think this is good to go!
Mon, Mar 29
LGTM! Thanks for formatting ^.^
Thu, Mar 25
I think this is a good targeted fix for that problem we've discussed, demonstrated by tests.
Mar 20 2021
No-no, TrackControlDependencyCondBRVisitor's purpose is completely different. It tracks symbols that didn't necessarily participate in the report but participated in conditional statements that lexically surround the report. It's used for explaining how did we got ourselves into a given lexical spot but it doesn't explain why is this a problem.
Mar 19 2021
Aha ok, seems reasonable, thx!
Mar 17 2021
By tracking the call-expression you're basically tracking the raw pointer value because that's what operators * and -> return. Of course operator * returns an lvalue reference rather than a pointer but we don't make a difference when it comes to SVal representation.
Great, thanks!!
Mar 16 2021
LGTM!
That's what i like to see!
LGTM! Do any of the high-level comments at the beginning of the file need to be updated to reflect this change?
Mar 12 2021
Hmm this probably even deserves a warning? Like, the mutex goes out of scope but we don't know whether we've successfully destroyed it? Even if we perform the check later, we can't do anything about it anymore? (not sure how bad it is in practice)
Mar 10 2021
Actually many cases don't need to know an exact original type.
I can see that it adds many visitors to the BugReport passed to it.
You're testing it with the help of an alpha checker. Is this checker doing something special that we don't do normally? In particular, both array bound checkers that we have are very likely to be re-done from scratch if they are ever to be completed. A lot of decisions in them are questionable. So i'm worried that this test will become stale in that process. Maybe a unittest would be more appropriate so that to guarantee the specific functionality without relying on implementation details of the specific checker(?)
Mar 9 2021
Ok then, let's try to land it! 🤞
why does this not work?
I still have concerns about these functions - isNull, isNonNull etc. They do indeed provide a quick test for nullness that doesn't involve constructing a new program state. This test, however, will never be as precise as constructing a new state. It used to be when RangeConstraintManager boiled down to simple range operations but now that it grew much larger (eg., the newly added support for symbolic == and != that involves tracking equivalence classes), there's really no way to tell whether a state is going to be feasible without running the whole machine and observing its emergent behavior, which is what assume() does. On the other hand i still don't see any indication that assume() is noticeably expensive. So i'm really worried that this is a premature optimization that sacrifices correctness for nothing. So i'm in favor of phasing out these functions entirely and converting all code to always use assume(). This may occasionally involve untangling unwanted recursions but i hope that all recursive-ish operations can be isolated within the constraint manager itself (and possibly in checker's evalAssume() which we can hopefully guard against with runtime assertions).
Looks all clear.
Mar 3 2021
So, like, I think it's a start. You introduced a single source of truth for casting SVals and it's good. But i'm worried about our API surface.
tiny patch
I'm actually shocked that we provide such option at all. And that it's the first option in the list in scan-build --help. And that I haven't noticed it until now despite that. How did you discover the crash? Are people actually using it?
CERT rules are typically very vague and don't map nicely into specific static analysis algorithms. A lot of CERT rules flag valid code as well as bugs. I want you to explain what *exactly* does the checker checks (say, in the form of a state machine, i.e. what sequences of events in the program does it find) and whether flagged code is expected to be always invalid.
That's definitely an improvement for our API surface. I think this is good but like @steakhal said I recommend running on a large codebase looking for potential regressions because this code is (still) very much spaghetti and hard to reason about.
Mar 2 2021
In some cases BugReport.isInteresting(InnerPointerVal.getAsSymbol()) would yield us exactly what we want. But if there's no symbol we have no choice but to interact with the trackExpressionValue facility and this may turn out to be pretty challenging.
Feb 26 2021
The warning should be emitted but it shouldn't have a note at P.get() telling the user that an inner pointer was obtained.
Feb 25 2021
I suspect you're adding too many notes. The note needs to not be there if the *raw* pointer is not tracked. Eg., I suspect that your patch would add a note in the following case in which it shouldn't be there because the raw pointer value doesn't participate in the report despite smart pointer region being interesting:
std::unique_ptr<A> P; A *a = P.get(); // shound't emit a note here P->foo();
It's important to not emit redundant notes because users typically take these checker-specific notes as an indication that this information is an essential piece of evidence of the bug in their program. In this example they'd believe that the analyzer has figured out that the smart pointer is null by looking at what happens to the raw pointer value. So they may become very confused if this isn't the case.
Feb 15 2021
We doing it this way now? Thanks! o.o
Feb 12 2021
Thx!~
Feb 11 2021
but it is planned that the checker takes use of the planned new attributes to determine if a function should be checked
Feb 9 2021
Ok no problem!
Feb 8 2021
Aha, yup, makes sense!
Feb 6 2021
Feb 5 2021
We have a bad track record of finishing, but forgetting to enable GSoC projects -- did the same happen to SmartPtrChecker or that still needs refinement?
Jan 29 2021
Jan 28 2021
What I want to avoid is having a static analyzer-specific suppression attribute, and a different one for clang-tidy, and a third one for the frontend.
Jan 27 2021
Unforget to git add tests.
I excluded remarks for now with an assertion. Other tests added :)
Address review comments!
Jan 25 2021
Thanks!!
Jan 24 2021
This patch shoots the messenger but someone still needs to conduct a proper investigation. The assertion is losing a lot more of its bug-finding power than necessary to uncover the current cornercase; i really hope to preserve it instead.
Jan 22 2021
I added those for consistency but i think your effort is worth it so let's ditch them.
Jan 12 2021
Add a few comments here and there.
Jan 11 2021
this would be a great flow-sensitive check for the clang static analyzer
Yup, i'm sorry. I even saw this failure locally but was like "this test doesn't look familiar, probably someone else broke it" which i wouldn't have said if i looked at it for more than 1 second as it's strikingly obvious. My bad.
Jan 7 2021
My code seems to think that a variable print_count has the value INT_MAX for some reason
Jan 6 2021
Drop broken attributes. Fix naming.
Handle conflicts across redeclarations! Fix typos and documentation formatting.
Jan 5 2021
This patch adds a new core checker. I wouldn't be comfortable enabling it by default without a much more careful evaluation (than a single lit test). In particular, i'm worried about false positives due to constraint solving issues and our incorrect cast representation - it sounds like both of these may disproportionally affect this checker. I also still wonder about "intentional" use of signed overflows; it's likely that some developers would prefer to silence the check. At least we should have a rough idea about how loud it is.
Dec 24 2020
Amazing, thank you. I'm happy with the analysis and i have nothing more to say really :)
Dec 23 2020
Add an error for conflicting attributes!
Dec 22 2020
I think you've found a very nice and compact 50% solution to the problem. I didn't think of this while i was looking for a proper fix. Very nice.