ASDenysPetrov added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:358
+ assert(!isEmpty());
+ return begin()->From().isUnsigned();
+}
----------------
martong wrote:
> Probably it is unrelated to this patch, but
> Could it happen that `(++begin())->From().isUnsigned()` gives a different
> signedness? Or we had a separate assertion when we added the second `Range`?
> The same question applies to the below two functions as well. Seems like in
> `unite` we don't have such validity check...
>Or we had a separate assertion when we added the second Range?
I didn't find any assertion while adding.
Moreover, I'm not sure if there can happen `APSInts` of different types in a
single `Range`. We only have `assert(From < To)` in a ctor.
>Seems like in `unite` we don't have such validity check...
Not only `unite` doesn't have such, but all the rest don't as well. We rely on
a fact that the caller garantees the validity of RangeSet by using
`AdjustmentType`.
But, yes, it's worth to make some checks.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:687
+
+ if (IsConversion && (!IsPromotion || !What.isUnsigned()))
+ return makePersistent(convertTo(What, Ty));
----------------
martong wrote:
> Could you please explain why do we need the `|| !What.isUnsigned()` part of
> the condition?
>
> This would make much more sense to me:
> ```
> if (IsConversion && !IsPromotion)
> return makePersistent(convertTo(What, Ty));
> ```
Look. Here we handle 2 cases:
- `IsConversion && !IsPromotion`. In this case we handle changing a sign with
same bitwidth: char -> uchar, uint -> int. Here we convert //negatives //to
//positives //and //positives which is out of range// to //negatives//. We use
`convertTo` function for that.
- `IsConversion && IsPromotion && !What.isUnsigned()`. In this case we handle
changing a sign from signeds to unsigneds with higher bitwidth: char -> uint,
int-> uint64. The point is that we also need convert //negatives //to
//positives //and use `convertTo` function as well. For example, we don't need
such a convertion when converting //unsigned //to //signed with higher
bitwidth//, because all the values of //unsigned //is valid for the such
//signed//.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:690
+
+ // Promotion from unsigneds to signeds/unsigneds left.
+
----------------
martong wrote:
> martong wrote:
> > I think it would be more consistent with the other operations (truncate and
> > convert) if we had this logic in its own separate function: `promoteTo`.
> > And this function should be called only if `isPromotion` is set (?)
> This comment is confusing, since we can get here also if
> `isConversion` is false and
> `isPromotion` is false
Nothing confusing is here.
We have 7 main cases:
NOOP: **u -> u, s -> s**
Conversion: **u -> s, s -> u**
Truncation: **U-> u, S -> s**
Promotion: `u->U`, `s -> S`
Truncation + Conversion: **U -> s, S -> u**
Promotion + Conversion: **s -> U**, `u -> S`
As you can see, we've handled all the **bolds** above this line .
So only promotions from //unsigneds// left. That means, `isPromotion` never
should be `false` here. We could add an assertion here if you want.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:690-711
+ // Promotion from unsigneds to signeds/unsigneds left.
+
+ using llvm::APInt;
+ using llvm::APSInt;
+ ContainerType Result;
+ // We definitely know the size of the result set.
+ Result.reserve(What.size());
----------------
ASDenysPetrov wrote:
> martong wrote:
> > martong wrote:
> > > I think it would be more consistent with the other operations (truncate
> > > and convert) if we had this logic in its own separate function:
> > > `promoteTo`. And this function should be called only if `isPromotion` is
> > > set (?)
> > This comment is confusing, since we can get here also if
> > `isConversion` is false and
> > `isPromotion` is false
> Nothing confusing is here.
> We have 7 main cases:
> NOOP: **u -> u, s -> s**
> Conversion: **u -> s, s -> u**
> Truncation: **U-> u, S -> s**
> Promotion: `u->U`, `s -> S`
> Truncation + Conversion: **U -> s, S -> u**
> Promotion + Conversion: **s -> U**, `u -> S`
>
> As you can see, we've handled all the **bolds** above this line .
> So only promotions from //unsigneds// left. That means, `isPromotion` never
> should be `false` here. We could add an assertion here if you want.
That makes sense. I'll do.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:792
+ // unite -> uchar(-2, 1)
+ auto CastRange = [Ty, &VF = ValueFactory](const Range &R) -> Bounds {
+ // Get bounds of the given range.
----------------
martong wrote:
> Why not `-> Range`?
Because `Range` requires `from < to` in its contract. But I need to store the
values after conversions. You can see a check `if (NewBounds.first >
NewBounds.second)` on line #812.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:827
+ if (is_signed_v<F>) {
+ this->template checkCastTo<F, T>({{MIN, MIN}, {MAX, MAX}}, {{MAX, MIN}});
+ this->template checkCastTo<F, T>({{MID, MID}, {MAX, MAX}}, {{MAX, MID}});
----------------
martong wrote:
> I am getting lost. Why don't you check against `ToMIN` and `ToMAX` here?
> Could you explain e.g. with int16->int8?
>
> It is confusing that at many places you test against `MIN`, `MAX`, `A`, ...
> and the conversion is happening automatically by the `checkCastTo` template.
> Would it be more explicit to use everywhere `ToMIN`, `ToA`, `ToB`, ... and
> check against them?
We can't use ToMIN, ToMAX, ... everywhere. That would be incorrect:
**int16(-32768, 32767)** -> **int8(-128, 127)**, aka `(MIN, MAX) -> (ToMIN,
ToMAX) // OK`.
**int16(-32768, -32768)** -> **int8(-128, -128)**, aka `(MIN, MIN) -> (ToMIN,
ToMIN) // NOK`.
**int16(-32768,-32768)** -> **int8(0, 0)**, aka `(MIN, MIN) -> ((int8)MIN,
(int8)MIN) // OK`.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:874-879
+ constexpr auto FromA = TV::FromA;
+ constexpr auto ToA = TV::ToA;
+ constexpr auto FromB = TV::FromB;
+ constexpr auto ToB = TV::ToB;
+ this->template checkCastTo<F, T>({{FromA, ToA}, {FromB, ToB}},
+ {{FromA, ToA}});
----------------
martong wrote:
> Could you please elaborate what do we test here?
E.g.
```
RangeA U RangeB
(0000'1111'0000'0100, 0000'1111'0000'0111)U(1111'0000'0000'0100,
1111'0000'0000'0111)
truncates to
RangeC U RangeC
( 0000'0100, 0000'0111)U( 0000'0100,
0000'0111)
As you can see, we got two equal truncated ranges.
RangeC
Then unite them to (0000'0100, 0000'0111).
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103094/new/
https://reviews.llvm.org/D103094
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
