[llvm] 8753917 - Avoid strict aliasing violation on type punning inside llvm::PointerIntPair

[Victor Campos via llvm-commits]

Author: Breno Guimarães
Date: 2023-02-23T17:03:23Z
New Revision: 875391728c11339c8a6cd3338bcaa5ec0ffc2496

URL: https://github.com/llvm/llvm-project/commit/875391728c11339c8a6cd3338bcaa5ec0ffc2496
DIFF: https://github.com/llvm/llvm-project/commit/875391728c11339c8a6cd3338bcaa5ec0ffc2496.diff

LOG: Avoid strict aliasing violation on type punning inside llvm::PointerIntPair

llvm::PointerIntPair has methods that when used together can invoke
undefined behavior by violating strict aliasing.

`getPointer()` uses the underlying storage as it's declared: `intptr_t`
`getAddrOfPointer()` casts the underlying storage as if it was a
`PointerTy`

This violates strict aliasing, so depending on how they are used, it's
possible to have the compiler to optimize the code in unwanted ways.
See the unit test in the patch. We declare a `PointerIntPair` and use
the `getAddrOfPointer` method to fill in the a pointer value.  Then,
when we use `getPointer` the compiler is thrown off, thinking that
`intptr_t` storage could not have possibly be changed, and the check
fails.

Reviewed By: efriedma

Differential Revision: https://reviews.llvm.org/D124571

Added: 
    

Modified: 
    llvm/include/llvm/ADT/PointerIntPair.h
    llvm/unittests/ADT/PointerIntPairTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/ADT/PointerIntPair.h b/llvm/include/llvm/ADT/PointerIntPair.h
index 9278ccdb4788..f73f5bcd6ce0 100644
--- a/llvm/include/llvm/ADT/PointerIntPair.h
+++ b/llvm/include/llvm/ADT/PointerIntPair.h
@@ -19,10 +19,44 @@
 #include "llvm/Support/type_traits.h"
 #include <cassert>
 #include <cstdint>
+#include <cstring>
 #include <limits>
 
 namespace llvm {
 
+namespace detail {
+template <typename Ptr> struct PunnedPointer {
+  static_assert(sizeof(Ptr) == sizeof(intptr_t), "");
+
+  // Asserts that allow us to let the compiler implement the destructor and
+  // copy/move constructors
+  static_assert(std::is_trivially_destructible<Ptr>::value, "");
+  static_assert(std::is_trivially_copy_constructible<Ptr>::value, "");
+  static_assert(std::is_trivially_move_constructible<Ptr>::value, "");
+
+  explicit constexpr PunnedPointer(intptr_t i = 0) { *this = i; }
+
+  constexpr intptr_t asInt() const {
+    intptr_t R = 0;
+    std::memcpy(&R, Data, sizeof(R));
+    return R;
+  }
+
+  constexpr operator intptr_t() const { return asInt(); }
+
+  constexpr PunnedPointer &operator=(intptr_t V) {
+    std::memcpy(Data, &V, sizeof(Data));
+    return *this;
+  }
+
+  Ptr *getPointerAddress() { return reinterpret_cast<Ptr *>(Data); }
+  const Ptr *getPointerAddress() const { return reinterpret_cast<Ptr *>(Data); }
+
+private:
+  alignas(Ptr) unsigned char Data[sizeof(Ptr)];
+};
+} // namespace detail
+
 template <typename T, typename Enable> struct DenseMapInfo;
 template <typename PointerT, unsigned IntBits, typename PtrTraits>
 struct PointerIntPairInfo;
@@ -46,7 +80,7 @@ template <typename PointerTy, unsigned IntBits, typename IntType = unsigned,
 class PointerIntPair {
   // Used by MSVC visualizer and generally helpful for debugging/visualizing.
   using InfoTy = Info;
-  intptr_t Value = 0;
+  detail::PunnedPointer<PointerTy> Value;
 
 public:
   constexpr PointerIntPair() = default;
@@ -86,10 +120,12 @@ class PointerIntPair {
     assert(Value == reinterpret_cast<intptr_t>(getPointer()) &&
            "Can only return the address if IntBits is cleared and "
            "PtrTraits doesn't change the pointer");
-    return reinterpret_cast<PointerTy *>(&Value);
+    return Value.getPointerAddress();
   }
 
-  void *getOpaqueValue() const { return reinterpret_cast<void *>(Value); }
+  void *getOpaqueValue() const {
+    return reinterpret_cast<void *>(Value.asInt());
+  }
 
   void setFromOpaqueValue(void *Val) & {
     Value = reinterpret_cast<intptr_t>(Val);

diff  --git a/llvm/unittests/ADT/PointerIntPairTest.cpp b/llvm/unittests/ADT/PointerIntPairTest.cpp
index b2790050a242..0e1b87c0b314 100644
--- a/llvm/unittests/ADT/PointerIntPairTest.cpp
+++ b/llvm/unittests/ADT/PointerIntPairTest.cpp
@@ -109,4 +109,22 @@ TEST(PointerIntPairTest, ManyUnusedBits) {
                 "trivially copyable");
 }
 
+TEST(PointerIntPairTest, TypePunning) {
+  int I = 0;
+  int *IntPtr = &I;
+
+  int **IntPtrBegin = &IntPtr;
+  int **IntPtrEnd = IntPtrBegin + 1;
+
+  PointerIntPair<int *, 1> Pair;
+  int **PairAddr = Pair.getAddrOfPointer();
+
+  while (IntPtrBegin != IntPtrEnd) {
+    *PairAddr = *IntPtrBegin;
+    ++PairAddr;
+    ++IntPtrBegin;
+  }
+  EXPECT_EQ(Pair.getPointer(), IntPtr);
+}
+
 } // end anonymous namespace