[APSInt] Fix bug in APSInt mentioned in https://github.com/llvm/llvm-project/issues/59515

[Peter]

Summary

Also provide a tryExtValue() API like APInt did in D139683

[Peter]

revert the format.

Original APSInt.h doesn't comply with clang-format. Maybe change it in another patch

[RKSimon]

llvm/unittests/ADT/APSIntTest.cpp

88

why can't a i128 -7 be represented as a i64 -7?

[Peter]

add a unit test

[Peter]

llvm/unittests/ADT/APSIntTest.cpp

88

APInt(128, -7) == (uint64)-7, there are 64 leading zeros and thus is a actually a huge positive number.

I added two more tests to make this clear.

[barannikov88]

llvm/include/llvm/ADT/APSInt.h

94

The comment near getMinSignBits says that it is deprecated (and "use getSignificantBits() instead").
There are also isSignedIntN and isIntN, which can be useful:

return isSigned() ? isSignedIntN(64) : isIntN(63);

[Peter]

using another API for isRepresentableByInt64

[RKSimon]

LGTM

[Peter]

diff against formatted version

[tschuett]

llvm/include/llvm/ADT/APSInt.h

104

You don't need the constructor. getExtValue()should work.

[Peter]

llvm/include/llvm/ADT/APSInt.h

104

Yes I do, otherwise the static type of the two branch don't match, as discussed here: https://reviews.llvm.org/D139683

llvm/include/llvm/ADT/APInt.h:1500:36: error: incompatible operand types ('uint64_t' (aka 'unsigned long') and 'const std::nullopt_t')
    return (getActiveBits() <= 64) ? getZExtValue() : std::nullopt;
                                   ^ ~~~~~~~~~~~~~~   ~~~~~~~~~~~~

[tschuett]

Sorry.

[Peter]

rebase to workable version

[Peter]

NP. I didn't know that either until I was asked to do the same in that patch :)

[RKSimon]

LGTM

[Peter]

Reopen to fix bugs caused by incorrect use of getExtValue()

[Peter]

fix a bug caused by incorrect use of getExtValue

[RKSimon]

LGTM

[Peter]

Update clang

[Peter]

change base

[Peter]

After this patch the semantics of getExtValue changed and thus broke clang. @pcc Would you mind take a look if the modification works for clang.

[efriedma]

clang/lib/CodeGen/CGDebugInfo.cpp

5386 ↗

(On Diff #485328)

I think we actually want the existing behavior here. Values embedded in debug info aren't really signed or unsigned; they're interpreted by the debugger based on the type of the value.

Maybe it makes sense to add a new APSInt API for that? Or I guess we could explicitly write out Init.getInt().isSigned() ? Init.getInt().getSExtValue() : Init.getInt().getZExtValue(); if we can't think of a reasonable name for the new API...

[Peter]

[DebugInfo] update clang to match semantics changes in APSInt

[Peter]

clang/lib/CodeGen/CGDebugInfo.cpp

5386 ↗

(On Diff #485328)

I have no problem with this proposed new API, we may even name it uint64_t tryInt64Representaiton. My main concern is that returning a value whose static type is unsigned while semantics can be signed OR unsigned is a bit inconsist.

I will use something like Init.getInt().isSigned() ? Init.getInt().getSExtValue() : Init.getInt().getZExtValue(); for now.

[Peter]

@pcc @efriedma Would you mind review the changes to clang when you are free? If they look good I'll merge them.

[Peter]

use const ref

[efriedma]

LGTM

[vabridgers]

Hello, it appears this patch causes a crash when I analyze this reproducer, with Z3 enabled. In the case shown here, the analyzer finds that 'f' to the call a(f) is uninitialized, and then is attempted to be refuted through SMTConv, leading to the assertion.

If I modify the below code to not use isRepresentableByIn64(), or use 'assert(getMinSignedBits() <= 64 && "Too many bits for int64_t");' instead, I do not see the crash.

clang --analyze -Xclang -analyzer-config -Xclang crosscheck-with-z3=true --target=x86_64-redhat-linux case.c

void a(int);
typedef struct {
  int b;
} c;
c *d;
void e() {
  (void)d->b;
  int f;
  a(f);
}

The assert ...

clang-16: ../include/llvm/ADT/APSInt.h:99: int64_t llvm::APSInt::getExtValue() const: Assertion `isRepresentableByInt64() && "Too many bits for int64_t"' failed.Program received signal SIGABRT, Aborted.

[Peter]

In D140059#4081549, @vabridgers wrote:

Hello, it appears this patch causes a crash when I analyze this reproducer, with Z3 enabled. In the case shown here, the analyzer finds that 'f' to the call a(f) is uninitialized, and then is attempted to be refuted through SMTConv, leading to the assertion.

If I modify the below code to not use isRepresentableByIn64(), or use 'assert(getMinSignedBits() <= 64 && "Too many bits for int64_t");' instead, I do not see the crash.

clang --analyze -Xclang -analyzer-config -Xclang crosscheck-with-z3=true --target=x86_64-redhat-linux case.c

void a(int);
typedef struct {
  int b;
} c;
c *d;
void e() {
  (void)d->b;
  int f;
  a(f);
}

The assert ...

clang-16: ../include/llvm/ADT/APSInt.h:99: int64_t llvm::APSInt::getExtValue() const: Assertion `isRepresentableByInt64() && "Too many bits for int64_t"' failed.Program received signal SIGABRT, Aborted.

Thanks for helping identifying the bug.

This is not a problem in this patch, the semantics of getExtValue changed in this patch as carefully discussed here.
To summarize, we believe its not reasonable to carry out ext for (signed)(int64::Max + 1). Although it's still 64 bit, the returned value would be interpreted as int64::Min instead of a large positive integer. This behavior was allowed before.

To provide a fix for this problem, this patch provides a better way to determine if the extension is possible isRepresentableByInt64, I would recommend replacing if (LLVM_UNLIKELY(Int.getBitWidth() > 64u)) in Z3Solver.cpp:732 with if (LLVM_UNLIKELY(!Int.isRepresentableByInt64())).

[vabridgers]

Thanks @Peter. I tried that, and the crash is resolved. Submitted https://reviews.llvm.org/D142627 for review.

llvm/include/llvm/ADT/APSInt.h

public:
   void setIsSigned(bool Val) { IsUnsigned = !Val; }
 
   /// Append this APSInt to the specified SmallString.
   void toString(SmallVectorImpl<char> &Str, unsigned Radix = 10) const {
     APInt::toString(Str, Radix, isSigned());
   }
   using APInt::toString;
 
+  /// If this int is representable using an int64_t.
+  bool isRepresentableByInt64() const {
+    // For unsigned values with 64 active bits, they technically fit into a
+    // int64_t, but the user may get negative numbers and has to manually cast
+    // them to unsigned. Let's not bet the user has the sanity to do that and
+    // not give them a vague value at the first place.
+    return isSigned() ? isSignedIntN(64) : isIntN(63);
+  }
+
   /// Get the correctly-extended \c int64_t value.
   int64_t getExtValue() const {
-    assert(getMinSignedBits() <= 64 && "Too many bits for int64_t");
+    assert(isRepresentableByInt64() && "Too many bits for int64_t");
     return isSigned() ? getSExtValue() : getZExtValue();
   }
 
+  std::optional<int64_t> tryExtValue() const {
+    return isRepresentableByInt64() ? std::optional<int64_t>(getExtValue())
+                                    : std::nullopt;
+  }
+
   APSInt trunc(uint32_t width) const {
     return APSInt(APInt::trunc(width), IsUnsigned);
   }
 
   APSInt extend(uint32_t width) const {
     if (IsUnsigned)
       return APSInt(zext(width), IsUnsigned);
     else

llvm/unittests/ADT/APSIntTest.cpp

TEST(APSIntTest, getUnsigned) {
   EXPECT_EQ(7u, APSInt::getUnsigned(7).getZExtValue());
   EXPECT_EQ(7, APSInt::getUnsigned(7).getSExtValue());
   EXPECT_TRUE(APSInt::getUnsigned(-7).isUnsigned());
   EXPECT_EQ(64u, APSInt::getUnsigned(-7).getBitWidth());
   EXPECT_EQ(-7, APSInt::getUnsigned(-7).getSExtValue());
   EXPECT_EQ(UINT64_C(0) - 7, APSInt::getUnsigned(-7).getZExtValue());
 }
 
+TEST(APSIntTest, isRepresentableByInt64) {
+  ASSERT_TRUE(APSInt(APInt(3, 7), true).isRepresentableByInt64());
+  ASSERT_TRUE(APSInt(APInt(128, 7), true).isRepresentableByInt64());
+  ASSERT_TRUE(APSInt(APInt(128, 7), false).isRepresentableByInt64());
+  ASSERT_TRUE(APSInt(APInt(64, -1), false).isRepresentableByInt64());
+  ASSERT_FALSE(APSInt(APInt(64, (uint64_t)-1), true).isRepresentableByInt64());
+}
 TEST(APSIntTest, getExtValue) {
   EXPECT_TRUE(APSInt(APInt(3, 7), true).isUnsigned());
   EXPECT_TRUE(APSInt(APInt(3, 7), false).isSigned());
   EXPECT_TRUE(APSInt(APInt(4, 7), true).isUnsigned());
   EXPECT_TRUE(APSInt(APInt(4, 7), false).isSigned());
   EXPECT_TRUE(APSInt(APInt(4, -7), true).isUnsigned());
   EXPECT_TRUE(APSInt(APInt(4, -7), false).isSigned());
   EXPECT_EQ(7, APSInt(APInt(3, 7), true).getExtValue());
   EXPECT_EQ(-1, APSInt(APInt(3, 7), false).getExtValue());
   EXPECT_EQ(7, APSInt(APInt(4, 7), true).getExtValue());
   EXPECT_EQ(7, APSInt(APInt(4, 7), false).getExtValue());
   EXPECT_EQ(9, APSInt(APInt(4, -7), true).getExtValue());
   EXPECT_EQ(-7, APSInt(APInt(4, -7), false).getExtValue());
 }
+TEST(APSIntTest, tryExtValue) {
+  ASSERT_EQ(-7, APSInt(APInt(64, -7), false).tryExtValue().value_or(42));
+  ASSERT_EQ(42, APSInt(APInt(128, -7), false).tryExtValue().value_or(42));
+  ASSERT_EQ(-1,
+            APSInt(APInt::getAllOnes(128), false).tryExtValue().value_or(42));
+  ASSERT_EQ(42, APSInt(APInt(64, -7), true).tryExtValue().value_or(42));
+  ASSERT_EQ(1, APSInt(APInt(128, 1), true).tryExtValue().value_or(42));
+  ASSERT_EQ(42,
+            APSInt(APInt::getAllOnes(128), true).tryExtValue().value_or(42));
+}
 
 TEST(APSIntTest, compareValues) {
   auto U = [](uint64_t V) { return APSInt::getUnsigned(V); };
   auto S = [](int64_t V) { return APSInt::get(V); };
 
   // Bit-width matches and is-signed.
   EXPECT_TRUE(APSInt::compareValues(S(7), S(8)) < 0);
   EXPECT_TRUE(APSInt::compareValues(S(8), S(7)) > 0);