[APInt] provide a safe API for zext value and sext value.

[Peter]

Summary

Currently, APInt::getSExtValue and getZExtValue crashes on values with more than 64 bits.
Users may accidently crash the compiler with this setting when the integer may be i128.
As shown in https://github.com/llvm/llvm-project/issues/59316

In this patch we provide a trySExtValue and tryZExtValue to return an Optional, the user
needs to explictly unwrap it and condsier the possibility where there my no value in it.

[lebedev.ri]

llvm/lib/Support/APInt.cpp

506–518 ↗

(On Diff #481476)

These should just check for precondition and call the normal functions

[Peter]

use existing API

[Peter]

llvm/lib/Support/APInt.cpp

506–518 ↗

(On Diff #481476)

You are right, thanks for pointing that out.

[RKSimon]

llvm/include/llvm/ADT/APInt.h

1499

std::optional ?

llvm/lib/Support/APInt.cpp

514 ↗

(On Diff #481482)

inline?

[Peter]

use std::optional and inlining the API

[RKSimon]

llvm/include/llvm/ADT/APInt.h

1522

getSignificantBits - you might need to alter the unit test as 'all bits' representations will safely truncate to -1

[tschuett]

llvm/include/llvm/ADT/APInt.h

1501

std::nullopt?

[RKSimon]

llvm/include/llvm/ADT/APInt.h

1501

+1

[tschuett]

llvm/unittests/ADT/APIntTest.cpp

3136

This is opinion and not review!!! For unit tests, I prefer .has_value() instead of the boolean conversion to make the intent clear.

[Peter]

use std::nullopt, add unit test

[RKSimon]

llvm/include/llvm/ADT/APInt.h

1518

zero -> sign

1522

getActiveBits ->getSignificantBits

[tschuett]

llvm/include/llvm/ADT/APInt.h

1501

std::optional<uint64_t>(std::nullopt). --> std::nullopt .

std::optional<uint64_t>(getZExtValue()) ->. getZExtValue().

[RKSimon]

Also, please can you add APSInt::tryExtValue in this patch as well?

[Peter]

minor changes, adding tryExt to APSInt.

[Peter]

llvm/unittests/ADT/APSIntTest.cpp

94 ↗

(On Diff #482227)

This test is explicitly commented out. It will trigger a bug in getExtValue. Do we want to fix it in this patch or another?

Bug description: There is an inconsistency in getExtValue. When checking if the operation is possible, it is always treated as a signed integer (getMinSignedBits), but when casting, it may be treated as an unsigned.

This inconsistency will crash all unsigned integer > 64 bits, with all high bits set to 1. (e.g. here its u128_max). getExtValue will determine the cast to unsigned int64 is possible(because it's treated as -1) but when casting getZExtValue will find it impossible.

[RKSimon]

Oh - that's annoying! Let's drop the APSInt changes and do them as follow ups.

[Peter]

In D139683#3991450, @RKSimon wrote:

Oh - that's annoying! Let's drop the APSInt changes and do them as follow ups.

Will do

[Peter]

remove APSInt change. will patch it in the future

[RKSimon]

llvm/include/llvm/ADT/APInt.h

1522

Still needs to be converted to getSignificantBits to match getSExtValue

[RKSimon]

I've raised https://github.com/llvm/llvm-project/issues/59515 to cover the APSInt::getExtValue() issue

[Peter]

update signed cast

llvm/include/llvm/ADT/APInt.h

1522

Sry I forgot about it. This patch should be good to go.

[RKSimon]

LGTM with a couple of minors - cheers!

llvm/include/llvm/ADT/APInt.h

1501

Any reason you can't remove the std::optional<uint64_t>() cast?

1522

Any reason you can't remove the std::optional<int64_t>() cast?

[Peter]

llvm/include/llvm/ADT/APInt.h

1501

Err if I remove both cast, the static type of two branches won't match. Seems implicit cast didn't kick in.

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;
                                   ^ ~~~~~~~~~~~~~~   ~~~~~~~~~~~~

Do you know any work arounds?

[Peter]

llvm/include/llvm/ADT/APInt.h

1501

Oh, If I rewrite this as an if-branch that works:

if (getActiveBits() <= 64)
  return getZExtValue();
return std::nullopt;

Which one would you prefer?

[RKSimon]

llvm/include/llvm/ADT/APInt.h

1501

Ah - of course! Keeping as it is is fine - thanks!

llvm/include/llvm/ADT/APInt.h

#endif
   /// uint64_t. Otherwise an assertion will result.
   uint64_t getZExtValue() const {
     if (isSingleWord())
       return U.VAL;
     assert(getActiveBits() <= 64 && "Too many bits for uint64_t");
     return U.pVal[0];
   }
 
+  /// Get zero extended value if possible
+  ///
+  /// This method attempts to return the value of this APInt as a zero extended
+  /// uint64_t. The bitwidth must be <= 64 or the value must fit within a
+  /// uint64_t. Otherwise no value is returned.
+  std::optional<uint64_t> tryZExtValue() const {
+    return (getActiveBits() <= 64) ? std::optional<uint64_t>(getZExtValue())
+                                   : std::optional<uint64_t>();
+  };
+
   /// Get sign extended value
   ///
   /// This method attempts to return the value of this APInt as a sign extended
   /// int64_t. The bit width must be <= 64 or the value must fit within an
   /// int64_t. Otherwise an assertion will result.
   int64_t getSExtValue() const {
     if (isSingleWord())
       return SignExtend64(U.VAL, BitWidth);
     assert(getSignificantBits() <= 64 && "Too many bits for int64_t");
     return int64_t(U.pVal[0]);
   }
 
+  /// Get sign extended value if possible
+  ///
+  /// This method attempts to return the value of this APInt as a zero extended
+  /// int64_t. The bitwidth must be <= 64 or the value must fit within an
+  /// int64_t. Otherwise no value is returned.
+  std::optional<int64_t> trySExtValue() const {
+    return (getActiveBits() <= 64) ? std::optional<int64_t>(getSExtValue())
+                                   : std::optional<int64_t>();
+  };
+
   /// Get bits required for string value.
   ///
   /// This method determines how many bits are required to hold the APInt
   /// equivalent of the string given by \p str.
   static unsigned getBitsNeeded(StringRef str, uint8_t radix);
 
   /// Get the bits that are sufficient to represent the string value. This may
   /// over estimate the amount of bits required, but it does not require

llvm/unittests/ADT/APIntTest.cpp

 
 TEST(APIntTest, DenseMap) {
   DenseMap<APInt, int> Map;
   APInt ZeroWidthInt(0, 0, false);
   Map.insert({ZeroWidthInt, 0});
   Map.find(ZeroWidthInt);
 }
 
+TEST(APIntTest, TryExt) {
+  APInt small(32, 42);
+  APInt large(128, {0xffff, 0xffff});
+  ASSERT_TRUE(small.tryZExtValue());
+  ASSERT_TRUE(small.trySExtValue());
+  ASSERT_FALSE(large.tryZExtValue());
+  ASSERT_FALSE(large.trySExtValue());
+  ASSERT_EQ(small.trySExtValue().value_or(41), 42);
+  ASSERT_EQ(large.trySExtValue().value_or(41), 41);
+}
+
 } // end anonymous namespace