manas added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1397
+ (LHS.To() < 0 && RHS.To() < 0 && Max > 0) ||
+ (LHS.To() < 0 && RHS.To() < 0 && Max > 0)) {
+ return {RangeFactory, Tmin, Tmax};
----------------
vsavchenko wrote:
> This clause is exactly the same as the previous one, it is a mistake.
> And I think we should have a test that could've shown that.
> Also, since you are checking for overflows for both the beginning and the
> end, we should have tests where both overflow.
Understood! I will add tests to check each OR part of these conditionals in
these cases.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1401-1402
+
+ // FIXME: This case in particular is resulting in failed assertion.
+ Range First = Range(Tmin, Max);
+ Range Second = Range(Min, Tmax);
----------------
vsavchenko wrote:
> vsavchenko wrote:
> > manas wrote:
> > > I changed the logic from using getCrucialPoints (which I mentioned in the
> > > e-mail thread) to simple checks to determine overflows using signedness.
> > > But the failed assertions again popped up. And I was unable to pin-point
> > > the issue here. Can someone help me?
> > >
> > > Although, I am thinking of revising the above logic by using bitwise
> > > methods to detect overflows.
> > This is actually one place that I won't expect an assertion failure.
> > Can we get a bit more detail on it? Is it again `From > To` (which will
> > indeed be weird) or something different?
> >
> > NOTE: Asking for help (either here or on StackOverflow) is a lot like
> > filing a bug report, try to put as much information as possible, try to
> > structure this information so it is easy to follow. It's also good to tell
> > people what you tried, instead of saying that you tried something and it
> > didn't work.
> OK, I downloaded your patch and ran the debugger.
>
> It complains about different bit-width for ranges that the analyzer tries to
> intersect. What I checked next: what are those ranges, so I took `begin()`
> from one of them and checked what are `From` and `To` there.
> Here is one of them:
> ```
> (const llvm::APSInt) $8 = {
> llvm::APInt = {
> U = {
> VAL = 22
> pVal = 0x0000000000000016
> }
> BitWidth = 4022311424
> }
> IsUnsigned = true
> }
> ```
> Woah, this `BitWidth` seems ridiculous! What does it tell us? It definitely
> didn't get there as part of any reasonable logic, right? So, what can it be
> instead? Only one answer here - garbage. We have some sort of memory issue
> with integers that we use as part of our ranges! So, let's see what type of
> information `Range` class actually stores:
> ```
> class Range {
> public:
> Range(const llvm::APSInt &From, const llvm::APSInt &To) : Impl(&From, &To) {
> assert(From <= To);
> }
>
> ...
>
> private:
> std::pair<const llvm::APSInt *, const llvm::APSInt *> Impl;
> };
> ```
>
> What we have here is a pair of pointers, and you have:
> ```
> llvm::APSInt Min = LHS.From() + RHS.From();
> llvm::APSInt Max = LHS.To() + RHS.To();
> llvm::APSInt Tmin = ValueFactory.getMinValue(ResultType);
> llvm::APSInt Tmax = ValueFactory.getMaxValue(ResultType);
> ```
> These are ALL stack variables. So, pointer to those point to adequate data
> ONLY while we are inside of our functions. When we call another function,
> these pointers point into some data from some other function in the call
> stack, it doesn't point to anything even remotely resembling `llvm::APSInt`
> and we get `BitWidth = 4022311424`.
>
> OK, we figured out the problem, what about a solution? If you look at other
> implementations, you can notice a couple of interesting things.
> When you do:
> ```
> llvm::APSInt Tmin = ValueFactory.getMinValue(ResultType);
> ```
> other code does something like:
> ```
> const llvm::APSInt &Tmin = ValueFactory.getMinValue(ResultType);
> ```
> Or when you do:
> ```
> Range Second = Range(Min, Tmax);
> ```
> The sibling code does:
> ```
> return {RangeFactory, ValueFactory.getValue(Min), ValueFactory.getValue(Max)};
> ```
>
> It looks like `ValueFactory` is the key here! It actually manages the
> lifetime of those integers for you and, whenever you ask it about values, it
> gives you `llvm::APSInt &` (note the reference part) that will have the
> correct lifetime.
>
> I hope this gives you some insight into how you can debug things like this on
> your own and how you can reason about what you see.
>
> Another piece of advice is to look around, other `VisitBinaryOperator`
> methods have all the information you actually needed. If you don't
> understand why we need `ValueFactory`, - experiment, ask us! It's bad to
> just ignore it.
Got it! I am working on fixing this.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1405-1406
+ RangeSet ResultRangeSet = RangeFactory.getRangeSet(First);
+ RangeSet ResultRangeSet2 = RangeFactory.add(ResultRangeSet, Second);
+ return ResultRangeSet2;
+ }
----------------
vsavchenko wrote:
> Why not just `return RangeFactory.add(ResultRangeSet, Second)`?
>
> NOTE: variables with integers in their names is a big no-no.
Right! I assumed I have to refactor this part so I used this temporarily.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1409-1415
+ if ((LHS.From() > 0 && RHS.From() > 0 && Min < 0) ||
+ (LHS.From() < 0 && RHS.From() < 0 && Min > 0) ||
+ (LHS.To() > 0 && RHS.To() > 0 && Max < 0) ||
+ (LHS.To() < 0 && RHS.To() < 0 && Max > 0)) {
+ // return [Tmin, Tmax]
+ return {RangeFactory, Tmin, Tmax};
+ }
----------------
vsavchenko wrote:
> I thought we talked quite a lot that there is nothing bad with overflows and
> here we have that if ANY overflow happened, we bail out and don't give any
> result.
Understood! Should I replace it with code returning EmptySet()?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103440/new/
https://reviews.llvm.org/D103440
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
