Skip to content

[clang] LazyOffsetPtr: Use native pointer width #111995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[clang] LazyOffsetPtr: Use native pointer width #111995

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions clang/include/clang/AST/ExternalASTSource.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,25 +326,25 @@ 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;
mutable uintptr_t Ptr = 0;

public:
LazyOffsetPtr() = default;
explicit LazyOffsetPtr(T *Ptr) : Ptr(reinterpret_cast<uint64_t>(Ptr)) {}
explicit LazyOffsetPtr(T *Ptr) : Ptr(reinterpret_cast<uintptr_t>(Ptr)) {}

explicit LazyOffsetPtr(uint64_t Offset) : Ptr((Offset << 1) | 0x01) {
assert((Offset << 1 >> 1) == Offset && "Offsets must require < 63 bits");
explicit LazyOffsetPtr(uintptr_t Offset) : Ptr((Offset << 1) | 0x01) {
assert((Offset << 1 >> 1) == Offset && "Offsets must fit in addressable bits");
if (Offset == 0)
Ptr = 0;
}

LazyOffsetPtr &operator=(T *Ptr) {
this->Ptr = reinterpret_cast<uint64_t>(Ptr);
this->Ptr = reinterpret_cast<uintptr_t>(Ptr);
return *this;
}

LazyOffsetPtr &operator=(uint64_t Offset) {
assert((Offset << 1 >> 1) == Offset && "Offsets must require < 63 bits");
LazyOffsetPtr &operator=(uintptr_t Offset) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be correct. The offset is required to be a 64 bit integer while the higher 32 bits are meaningful. Is there any ideas to make it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can the higher 32 bits be meaningful on a system whose pointers are 32 bits in length? Put another way: in what situation is a 64-bit offset to a 32-bit pointer valid?

The original message warns about 63 bits because the last bit is needed to tag the value as not-a-pointer (and as an offset). That doesn't mean values passed here have to be 63 bits in length, just that the (naive) implementation that hardcoded 64-bit size for pointers required the offset to fit in N-1 = 63 bits.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's an abstract offset, not a pointer offset.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we mmap the whole of each module file, so we definitely can't have more than 4GiB of module file data if the host has 32-bit pointers. But Offset here is in bits, and with 31 bits of offset, that only covers 256MiB of module data, which is a plausible and reasonable total file size for .pcm files even when compiling on a 32-bit host. So I think we do want a 64-bit offset here even on a 32-bit system.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. But it is still invalid to reinterpret_cast a uint64_t to a pointer on 32-bit BE systems, and that would still leave that as a segfault.

C++23 draft N4950, §7.6.1.10 number 5:

A value of integral type or enumeration type can be explicitly converted to a pointer. A pointer converted to an integer of sufficient size (if any such exists on the implementation) and back to the same pointer type will have its original value; mappings between pointers and integers are otherwise implementation-defined.

uint64_t is not the sufficient size for a 32-bit BE system. How should we proceed?

  • I suppose a union could work, though it would probably be ugly.
  • Alternatively, there could be two fields (a uint64_t Offset and a uintptr_t Ptr), but that would unconditionally add 8 bytes per LazyOffsetPtr. I'm not completely familiar with how many live LazyOffsetPtr objects might be present in a single invocation of Clang and how that would affect memory usage.
  • If there is a different way, I'd be happy to implement that as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 32-bit PPC issue is presumably solved by just adding an intermediate cast.

Copy link
Collaborator

@jrtc27 jrtc27 Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's not. Please re-read the problem description. Casts in the LazyOffsetPtr implementation aren't going to fix anything. The problem is that getAddressOfPointer pretends that the underlying storage is a pointer, when it's really a uint64_t, and on a 32-bit big-endian system *(void **)&my_uint64 = p and my_uint64 = (uint64_t)(uintptr_t)p store the contents of p in different halves of my_uint64 (and zero the other half in the latter, but that isn't what's causing a problem). If you wanted to do something really disgusting you could implement getAddressOfPointer as return (T **)&Ptr + 1, I suppose. But ideally we wouldn't be doing anything as highly dodgy when it comes to strict aliasing as that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so the existence of a function that returns the address of internal storage is obviously always going to make abstraction very difficult. The perfectly portable way to write this is to use a union and a separate bool, and the whole point of this type is that that's a very inefficient way to store this information; we want it to be more compact, and we're willing to accept some non-portable assumptions to get that. So that's why I think the key question is whether we can change the clients to remove the need for getAddressOfPointer first. Once we do that, the rest of this becomes significantly more trivial.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not, you probably need something like:

diff --git a/clang/include/clang/AST/ExternalASTSource.h b/clang/include/clang/AST/ExternalASTSource.h
index 385c32edbae0..3f0c63d56d67 100644
--- a/clang/include/clang/AST/ExternalASTSource.h
+++ b/clang/include/clang/AST/ExternalASTSource.h
@@ -25,6 +25,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/iterator.h"
 #include "llvm/Support/PointerLikeTypeTraits.h"
+#include <algorithm>
 #include <cassert>
 #include <cstddef>
 #include <cstdint>
@@ -326,29 +327,43 @@ 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 char Data[DataSize] = {};
+
+  template <typename U> U &As() const {
+    if (llvm::sys::IsBigEndianHost)
+      return *reinterpret_cast<U *>(Data + DataSize - sizeof(U));
+    else
+      return *reinterpret_cast<U *>(Data);
+  }
+
+  char &AsLSB() const { return As<char>(); }
+  T *&AsPtr() const { return As<T *>(); }
+  uint64_t &AsU64() const { return As<uint64_t>(); }
 
 public:
   LazyOffsetPtr() = default;
-  explicit LazyOffsetPtr(T *Ptr) : Ptr(reinterpret_cast<uint64_t>(Ptr)) {}
+  explicit LazyOffsetPtr(T *Ptr) : Data() { AsPtr() = 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;
+      AsPtr() = NULL;
+    else
+      AsU64() = (Offset << 1) | 0x01;
   }
 
   LazyOffsetPtr &operator=(T *Ptr) {
-    this->Ptr = reinterpret_cast<uint64_t>(Ptr);
+    AsPtr() = Ptr;
     return *this;
   }
 
   LazyOffsetPtr &operator=(uint64_t Offset) {
     assert((Offset << 1 >> 1) == Offset && "Offsets must require < 63 bits");
     if (Offset == 0)
-      Ptr = 0;
+      AsPtr() = NULL;
     else
-      Ptr = (Offset << 1) | 0x01;
+      AsU64() = (Offset << 1) | 0x01;
 
     return *this;
   }
@@ -356,15 +371,15 @@ public:
   /// 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() || AsPtr() != 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() || AsPtr() != NULL; }
 
   /// Whether this pointer is currently stored as an offset.
-  bool isOffset() const { return Ptr & 0x01; }
+  bool isOffset() const { return AsLSB() & 0x01; }
 
   /// Retrieve the pointer to the AST node that this lazy pointer points to.
   ///
@@ -375,9 +390,9 @@ public:
     if (isOffset()) {
       assert(Source &&
              "Cannot deserialize a lazy pointer without an AST source");
-      Ptr = reinterpret_cast<uint64_t>((Source->*Get)(OffsT(Ptr >> 1)));
+      AsPtr() = (Source->*Get)(OffsT(AsU64() >> 1));
     }
-    return reinterpret_cast<T*>(Ptr);
+    return AsPtr();
   }
 
   /// Retrieve the address of the AST node pointer. Deserializes the pointee if
@@ -385,7 +400,7 @@ public:
   T **getAddressOfPointer(ExternalASTSource *Source) const {
     // Ensure the integer is in pointer form.
     (void)get(Source);
-    return reinterpret_cast<T**>(&Ptr);
+    return &AsPtr();
   }
 };

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we need to preserve at least 64 bits in this structure.

Hmm. We're only really preserving 63 bits here, so that might be a problem.

Sorry for misleading. The current decoding for GlobalDeclID (64 bits) is to split it as {ModuleFileIndex : 32, DeclIndex : 32}. And both ModuleFileIndex and DeclIndex starts from 0 to higher values. So it is safe to remove the highest bit of (ModuleFileIndex) by paying the price that upper bound of the number of modules in a single compilation to be only 2^31, which is a reasonable number.

assert((Offset << 1 >> 1) == Offset && "Offsets must fit in addressable bits");
if (Offset == 0)
Ptr = 0;
else
Expand Down Expand Up @@ -375,7 +375,7 @@ 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)));
Ptr = reinterpret_cast<uintptr_t>((Source->*Get)(OffsT(Ptr >> 1)));
}
return reinterpret_cast<T*>(Ptr);
}
Expand Down
Loading