[clang] LazyOffsetPtr: Use native pointer width #111995
Conversation
On big-endian systems, narrow casting will read the higher bits of the value. LazyOffsetPtr's `getAddressOfPointer` returns the address-of `Ptr` which was unconditionally a 64-bit value. On 32-bit big endian systems, reading this value as a 32-bit pointer returns invalid data. Fixes: bc73ef0 ("PR60985: Fix merging of lambda closure types across modules.") Closes: llvm#111993
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: A. Wilcox (awilfox) ChangesOn big-endian systems, narrow casting will read the higher bits of the value. LazyOffsetPtr's Fixes: bc73ef0 ("PR60985: Fix merging of lambda closure types across modules.") Full diff: https://github.com/llvm/llvm-project/pull/111995.diff 1 Files Affected:
diff --git a/clang/include/clang/AST/ExternalASTSource.h b/clang/include/clang/AST/ExternalASTSource.h
index 385c32edbae0fd..f5ce1a916fbee8 100644
--- a/clang/include/clang/AST/ExternalASTSource.h
+++ b/clang/include/clang/AST/ExternalASTSource.h
@@ -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) {
+ assert((Offset << 1 >> 1) == Offset && "Offsets must fit in addressable bits");
if (Offset == 0)
Ptr = 0;
else
@@ -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);
}
|
|
This patch has been tested on 32-bit PowerPC, 64-bit PowerPC, and 64-bit x86_64 (ensuring it doesn't change any behaviour on LE systems) - the entire Clang test suite passed on all three platforms. |
You can test this locally with the following command:git-clang-format --diff efcfa6e711689ada546c323316145ecd749d380a 3c9198421a44f32d00c2ebe4ee0bf5a6ec67e9ac --extensions h -- clang/include/clang/AST/ExternalASTSource.hView the diff from clang-format here.diff --git a/clang/include/clang/AST/ExternalASTSource.h b/clang/include/clang/AST/ExternalASTSource.h
index f5ce1a916f..e146d68759 100644
--- a/clang/include/clang/AST/ExternalASTSource.h
+++ b/clang/include/clang/AST/ExternalASTSource.h
@@ -333,7 +333,8 @@ public:
explicit LazyOffsetPtr(T *Ptr) : Ptr(reinterpret_cast<uintptr_t>(Ptr)) {}
explicit LazyOffsetPtr(uintptr_t Offset) : Ptr((Offset << 1) | 0x01) {
- assert((Offset << 1 >> 1) == Offset && "Offsets must fit in addressable bits");
+ assert((Offset << 1 >> 1) == Offset &&
+ "Offsets must fit in addressable bits");
if (Offset == 0)
Ptr = 0;
}
@@ -344,7 +345,8 @@ public:
}
LazyOffsetPtr &operator=(uintptr_t Offset) {
- assert((Offset << 1 >> 1) == Offset && "Offsets must fit in addressable bits");
+ assert((Offset << 1 >> 1) == Offset &&
+ "Offsets must fit in addressable bits");
if (Offset == 0)
Ptr = 0;
else
|
|
I feel like the messages being highlighted in the CI formatter need work anyway. I was hoping the community would have a suggestion on better wording. However, I will be glad to reformat those lines in that manner if this wording should stay. |
| return *this; | ||
| } | ||
|
|
||
| LazyOffsetPtr &operator=(uint64_t Offset) { | ||
| assert((Offset << 1 >> 1) == Offset && "Offsets must require < 63 bits"); | ||
| LazyOffsetPtr &operator=(uintptr_t Offset) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Well, it's an abstract offset, not a pointer offset.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 Offsetand auintptr_t Ptr), but that would unconditionally add 8 bytes perLazyOffsetPtr. I'm not completely familiar with how many liveLazyOffsetPtrobjects 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.
There was a problem hiding this comment.
The 32-bit PPC issue is presumably solved by just adding an intermediate cast.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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();
}
};
There was a problem hiding this comment.
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.
It is used in |
I can confirm that the patch from this PR fixes the GCC bootstrap on 32-bit PowerPC on Linux for me (GCC pr/target 113341). |
|
Btw, if you submit a fix, it would be great if it could be backported to the 18 and 19 branches. |
I think the 18 branch may already be closed. This is what we are shipping in Adélie for 18: big-endian-32.patch. |
And that in turn is used in (and also in |
@rjmccall @jansvoboda11 for ObjC topics. |
|
The patch from this PR also fixes issue #86460! Have not tested jrtc27's patch yet. |
|
#112806 should address this without narrowing the field to 32 bits. |
As does #111995 (comment), with less code and more generality |
I prefer this solution too. |
That solution still violates the rules of the C++ object model. |
How about: diff --git a/clang/include/clang/AST/ExternalASTSource.h b/clang/include/clang/AST/ExternalASTSource.h
index 385c32edbae0..766ebec2daf0 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,46 @@ 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] = {};
+
+ 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));
+ }
+
+ unsigned char GetLSB() const { return As<unsigned char>(false); }
+ 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 +374,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() || 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 +393,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)));
+ 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 +403,7 @@ public:
T **getAddressOfPointer(ExternalASTSource *Source) const {
// Ensure the integer is in pointer form.
(void)get(Source);
- return reinterpret_cast<T**>(&Ptr);
+ return &GetPtr();
}
};
The std::launder may be unnecessary, I don't understand all the scenarios it's needed. |
Yeah, I think that fixes it. For me that looks a lot less simple than using a union, but it is nice to have only a single implementation. I'd be fine with going in that direction. For CHERI, can we assume that the least-significant byte of the pointer representation is even when the alignment of the type is > 1?
Formally I think it's correct and necessary for the pointer and integer case because an array element and an object for which the array provides storage are not pointer-interconvertible, but it's incorrect for the |
The integer address portion of the capability has the same representation, so will be even. On all little-endian CHERI platforms that exist today this is laid out in memory as you would expect, i.e. with the first byte being the LSB of the address and the last byte being the MSB of the metadata. Big-endian CHERI did historically exist and was a bit weird for various reasons, but will hopefully(?) never return. If it does, whoever wants to tackle that beast can port this code if it's no longer true there by fiddling with the offset calculations. But yes, the short answer is that the code as written should work on Morello and CHERI-RISC-V.
I'll defer to your expert opinion for what exactly to do here. If the launder is needed for the other types but wrong for GetLSB the obvious fix is to just inline a simple specialisation of As, assuming direct access to |
Great, thanks.
I suppose we may as well be pedantically correct here and specialize |
|
|
Closing in favour of #112927 which also fixes these issues. Thank you. |
On big-endian systems, narrow casting will read the higher bits of the value. LazyOffsetPtr's
getAddressOfPointerreturns the address-ofPtrwhich was unconditionally a 64-bit value. On 32-bit big endian systems, reading this value as a 32-bit pointer returns invalid data.Fixes: bc73ef0 ("PR60985: Fix merging of lambda closure types across modules.")
Closes: #111993