[lldb-dev] Signedness of scalars built from APInt(s)

[Zachary Turner via lldb-dev]

I don't think #2 is a correct change.  Just because the sign bit is set
doesn't mean it's signed.  Is the 4-byte value 0x10000000 signed or
unsigned?  It's a trick question, because there's not enough information!
If it was written "int x = 0x10000000" then it's signed (and negative).  If
it was written "unsigned x = 0x10000000" then it's unsigned (and
positive).  What about the 4-byte value 0x1?  Still a trick!  If it was
written "int x = 1" then it's signed (and positive), and if it was written
"unsigned x = 1" then it's unsigned (and positive).

My point is that signedness of the *type* does not necessarly imply
signedness of the value, and vice versa.

APInt is purely a bit-representation and a size, there is no information
whatsoever about whether type *type* is signed.  It doesn't make sense to
say "is this APInt negative?" without additional information.

With APSInt, on the other hand, it does make sense to ask that question.
If you have an APSInt where isSigned() is true, *then* you can use the sign
bit to determine whether it's negative.  And if you have an APSInt where
isSigned() is false, then the "sign bit" is not actually a sign bit at all,
it is just an extra power of 2 for the unsigned value.

This is my understanding of the classes, someone correct me if I'm wrong.

IIUC though, the way to fix this is by using APSInt throughout the class,
and delete all references to APInt.

On Fri, Jan 4, 2019 at 2:58 PM Jonas Devlieghere <jonas at devlieghere.com>
wrote:

> If I understand the situation correctly I think we should do both. I'd
> start by doing (2) to improve the current behavior and add a constructor
> for APSInt. We can then audit the call sites and migrate to APSInt where
> it's obvious that the type is signed. That should match the semantics of
> both classes?
>
> On Fri, Jan 4, 2019 at 2:00 PM Davide Italiano <dccitaliano at gmail.com>
> wrote:
>
>> On Fri, Jan 4, 2019 at 1:57 PM Davide Italiano <dccitaliano at gmail.com>
>> wrote:
>> >
>> > While adding support for 512-bit integers in `Scalar`, I figured I
>> > could add some coverage.
>> >
>> > TEST(ScalarTest, Signedness) {
>> >  auto s1 = Scalar(APInt(32, 12, false /* isSigned */));
>> >  auto s2 = Scalar(APInt(32, 12, true /* isSigned */ ));
>> >  ASSERT_EQ(s1.GetType(), Scalar::e_uint); // fails
>> >  ASSERT_EQ(s2.GetType(), Scalar::e_sint); // pass
>> > }
>> >
>> > The result of `s1.GetType()` is Scalar::e_sint.
>> > This is because an APInt can't distinguish between "int patatino = 12"
>> > and "uint patatino = 12".
>> > The correct class in `llvm` to do that is `APSInt`.
>> >
>>
>> Please note that this is also broken in the case where you have
>> APInt(32 /* bitWidth */, -323);
>> because of the way the constructor is implemented.
>>
>> --
>> Davide
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20190104/830d5835/attachment-0001.html>