[clang] Make LazyOffsetPtr more portable

[jrtc27]

LazyOffsetPtr currently relies on uint64_t being able to store a pointer
and, unless sizeof(uint64_t) == sizeof(void *), little endianness, since
getAddressOfPointer reinterprets the memory as a pointer. This also
doesn't properly respect the C++ object model.

As removing getAddressOfPointer would have wide-reaching implications,
improve the implementation to account for these problems by using
placement new and a suitably sized-and-aligned buffer, "right"-aligning
the objects on big-endian platforms so the LSBs are in the same place
for use as the discriminator.

Fixes: bc73ef0
Fixes: #111993

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jessica Clarke (jrtc27)

Changes

LazyOffsetPtr currently relies on uint64_t being able to store a pointer
and, unless sizeof(uint64_t) == sizeof(void *), little endianness, since
getAddressOfPointer reinterprets the memory as a pointer. This also
doesn't properly respect the C++ object model.

As removing getAddressOfPointer would have wide-reaching implications,
improve the implementation to account for these problems by using
placement new and a suitably sized-and-aligned buffer, "right"-aligning
the objects on big-endian platforms so the LSBs are in the same place
for use as the discriminator.

Fixes: bc73ef0
Fixes: #111993

---

Full diff: https://github.com/llvm/llvm-project/pull/112927.diff

1 Files Affected:

• (modified) clang/include/clang/AST/ExternalASTSource.h (+35-13)

diff --git a/clang/include/clang/AST/ExternalASTSource.h b/clang/include/clang/AST/ExternalASTSource.h
index 385c32edbae0fd..caf37144d5eb73 100644
--- a/clang/include/clang/AST/ExternalASTSource.h
+++ b/clang/include/clang/AST/ExternalASTSource.h
@@ -25,10 +25,12 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/iterator.h"
 #include "llvm/Support/PointerLikeTypeTraits.h"
+#include <algorithm>
 #include <cassert>
 #include <cstddef>
 #include <cstdint>
 #include <iterator>
+#include <new>
 #include <optional>
 #include <utility>
 
@@ -326,29 +328,49 @@ struct LazyOffsetPtr {
   ///
   /// If the low bit is clear, a pointer to the AST node. If the low
   /// bit is set, the upper 63 bits are the offset.
-  mutable uint64_t Ptr = 0;
+  static constexpr size_t DataSize = std::max(sizeof(uint64_t), sizeof(T *));
+  alignas(uint64_t) alignas(T *) mutable unsigned char Data[DataSize] = {};
+
+  unsigned char GetLSB() const {
+    return Data[llvm::sys::IsBigEndianHost ? DataSize - 1 : 0];
+  }
+
+  template <typename U> U &As(bool New) const {
+    unsigned char *Obj =
+        Data + (llvm::sys::IsBigEndianHost ? DataSize - sizeof(U) : 0);
+    if (New)
+      return *new (Obj) U;
+    return *std::launder(reinterpret_cast<U *>(Obj));
+  }
+
+  T *&GetPtr() const { return As<T *>(false); }
+  uint64_t &GetU64() const { return As<uint64_t>(false); }
+  void SetPtr(T *Ptr) const { As<T *>(true) = Ptr; }
+  void SetU64(uint64_t U64) const { As<uint64_t>(true) = U64; }
 
 public:
   LazyOffsetPtr() = default;
-  explicit LazyOffsetPtr(T *Ptr) : Ptr(reinterpret_cast<uint64_t>(Ptr)) {}
+  explicit LazyOffsetPtr(T *Ptr) : Data() { SetPtr(Ptr); }
 
-  explicit LazyOffsetPtr(uint64_t Offset) : Ptr((Offset << 1) | 0x01) {
+  explicit LazyOffsetPtr(uint64_t Offset) : Data() {
     assert((Offset << 1 >> 1) == Offset && "Offsets must require < 63 bits");
     if (Offset == 0)
-      Ptr = 0;
+      SetPtr(NULL);
+    else
+      SetU64((Offset << 1) | 0x01);
   }
 
   LazyOffsetPtr &operator=(T *Ptr) {
-    this->Ptr = reinterpret_cast<uint64_t>(Ptr);
+    SetPtr(Ptr);
     return *this;
   }
 
   LazyOffsetPtr &operator=(uint64_t Offset) {
     assert((Offset << 1 >> 1) == Offset && "Offsets must require < 63 bits");
     if (Offset == 0)
-      Ptr = 0;
+      SetPtr(NULL);
     else
-      Ptr = (Offset << 1) | 0x01;
+      SetU64((Offset << 1) | 0x01);
 
     return *this;
   }
@@ -356,15 +378,15 @@ struct LazyOffsetPtr {
   /// Whether this pointer is non-NULL.
   ///
   /// This operation does not require the AST node to be deserialized.
-  explicit operator bool() const { return Ptr != 0; }
+  explicit operator bool() const { return isOffset() || GetPtr() != NULL; }
 
   /// Whether this pointer is non-NULL.
   ///
   /// This operation does not require the AST node to be deserialized.
-  bool isValid() const { return Ptr != 0; }
+  bool isValid() const { return isOffset() || GetPtr() != NULL; }
 
   /// Whether this pointer is currently stored as an offset.
-  bool isOffset() const { return Ptr & 0x01; }
+  bool isOffset() const { return GetLSB() & 0x01; }
 
   /// Retrieve the pointer to the AST node that this lazy pointer points to.
   ///
@@ -375,9 +397,9 @@ struct LazyOffsetPtr {
     if (isOffset()) {
       assert(Source &&
              "Cannot deserialize a lazy pointer without an AST source");
-      Ptr = reinterpret_cast<uint64_t>((Source->*Get)(OffsT(Ptr >> 1)));
+      SetPtr((Source->*Get)(OffsT(GetU64() >> 1)));
     }
-    return reinterpret_cast<T*>(Ptr);
+    return GetPtr();
   }
 
   /// Retrieve the address of the AST node pointer. Deserializes the pointee if
@@ -385,7 +407,7 @@ struct LazyOffsetPtr {
   T **getAddressOfPointer(ExternalASTSource *Source) const {
     // Ensure the integer is in pointer form.
     (void)get(Source);
-    return reinterpret_cast<T**>(&Ptr);
+    return &GetPtr();
   }
 };
 

[zygoloid]

One style nit, otherwise looks good, thanks!

[jrtc27]

One style nit, otherwise looks good, thanks!

I'm not sure "good" is quite the right word given the horrors this interface is hiding, but thanks for the review!

[jrtc27]

@awilfox Could you please verify if this fixes the bug for you?

[ernsteiswuerfel]

I can confirm it fixes issue #86460 for me just as awilfox original PR.

[awilfox]

I'll run a full test on the ppc32 builder now and report back. Thanks for this work.

[awilfox]

Yes, this passes the full Clang test suite on 32-bit PowerPC. Good to go.

[jrtc27]

Thanks both.

[thesamesam]

I appreciate it's not exactly a straightforward change given the discussion it provoked, but any chance you'd be okay with a backport to 19? Clang isn't really usable on ppc32 without this. We could pull it in just for ppc32 downstream but we generally dislike doing conditional patching. WDYT?

cc @mgorny

[glaubitz]

I appreciate it's not exactly a straightforward change given the discussion it provoked, but any chance you'd be okay with a backport to 19? Clang isn't really usable on ppc32 without this. We could pull it in just for ppc32 downstream but we generally dislike doing conditional patching. WDYT?

cc @mgorny

I agree. It would be great if this fix could be backported.

[mgorny]

Doesn't it change ABI, though?

[zygoloid]

Doesn't it change ABI, though?

Only on platforms where it didn't work at all before.