[IR] Avoid UB in SymbolTableListTraits
#139096
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced May 9, 2025
dtcxzyw
force-pushed
the
perf/safe_container_of
branch
from
6374456 to
11dfefb
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
dtcxzyw
force-pushed
the
perf/safe_container_of
branch
from
11dfefb to
0d79165
Compare
This was referenced May 29, 2025
|
@llvm/pr-subscribers-llvm-ir Author: Yingwei Zheng (dtcxzyw) ChangesThis patch fixes the "dereferencing null" UB. Unfortunately, C++ doesn't provide an inverse operation for Full diff: https://github.com/llvm/llvm-project/pull/139096.diff 4 Files Affected:
diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h
index 9ee0bacb5c258..10617db09fde6 100644
--- a/llvm/include/llvm/IR/BasicBlock.h
+++ b/llvm/include/llvm/IR/BasicBlock.h
@@ -546,6 +546,10 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
return &BasicBlock::InstList;
}
+ static size_t getSublistOffset(Instruction *) {
+ return offsetof(BasicBlock, InstList);
+ }
+
/// Dedicated function for splicing debug-info: when we have an empty
/// splice (i.e. zero instructions), the caller may still intend any
/// debug-info in between the two "positions" to be spliced.
diff --git a/llvm/include/llvm/IR/Function.h b/llvm/include/llvm/IR/Function.h
index 6d4a53da7ff22..63100568d07e4 100644
--- a/llvm/include/llvm/IR/Function.h
+++ b/llvm/include/llvm/IR/Function.h
@@ -811,6 +811,10 @@ class LLVM_ABI Function : public GlobalObject, public ilist_node<Function> {
return &Function::BasicBlocks;
}
+ static size_t getSublistOffset(BasicBlock *) {
+ return offsetof(Function, BasicBlocks);
+ }
+
public:
const BasicBlock &getEntryBlock() const { return front(); }
BasicBlock &getEntryBlock() { return front(); }
diff --git a/llvm/include/llvm/IR/Module.h b/llvm/include/llvm/IR/Module.h
index 53d1005333ee1..298ccab3bfae1 100644
--- a/llvm/include/llvm/IR/Module.h
+++ b/llvm/include/llvm/IR/Module.h
@@ -609,6 +609,9 @@ class LLVM_ABI Module {
static GlobalListType Module::*getSublistAccess(GlobalVariable*) {
return &Module::GlobalList;
}
+ static size_t getSublistOffset(GlobalVariable *) {
+ return offsetof(Module, GlobalList);
+ }
friend class llvm::SymbolTableListTraits<llvm::GlobalVariable>;
public:
@@ -619,6 +622,9 @@ class LLVM_ABI Module {
static FunctionListType Module::*getSublistAccess(Function*) {
return &Module::FunctionList;
}
+ static size_t getSublistOffset(Function *) {
+ return offsetof(Module, FunctionList);
+ }
/// Detach \p Alias from the list but don't delete it.
void removeAlias(GlobalAlias *Alias) { AliasList.remove(Alias); }
@@ -658,6 +664,9 @@ class LLVM_ABI Module {
static AliasListType Module::*getSublistAccess(GlobalAlias*) {
return &Module::AliasList;
}
+ static size_t getSublistOffset(GlobalAlias *) {
+ return offsetof(Module, AliasList);
+ }
friend class llvm::SymbolTableListTraits<llvm::GlobalAlias>;
/// Get the Module's list of ifuncs (constant).
@@ -668,6 +677,9 @@ class LLVM_ABI Module {
static IFuncListType Module::*getSublistAccess(GlobalIFunc*) {
return &Module::IFuncList;
}
+ static size_t getSublistOffset(GlobalIFunc *) {
+ return offsetof(Module, IFuncList);
+ }
friend class llvm::SymbolTableListTraits<llvm::GlobalIFunc>;
/// Get the Module's list of named metadata (constant).
diff --git a/llvm/include/llvm/IR/SymbolTableListTraits.h b/llvm/include/llvm/IR/SymbolTableListTraits.h
index fcf6f0fb15280..456833fff62ce 100644
--- a/llvm/include/llvm/IR/SymbolTableListTraits.h
+++ b/llvm/include/llvm/IR/SymbolTableListTraits.h
@@ -77,10 +77,8 @@ class SymbolTableListTraits : public ilist_alloc_traits<ValueSubClass> {
/// getListOwner - Return the object that owns this list. If this is a list
/// of instructions, it returns the BasicBlock that owns them.
ItemParentClass *getListOwner() {
- size_t Offset = reinterpret_cast<size_t>(
- &((ItemParentClass *)nullptr->*ItemParentClass::getSublistAccess(
- static_cast<ValueSubClass *>(
- nullptr))));
+ size_t Offset = ItemParentClass::getSublistOffset(
+ static_cast<ValueSubClass *>(nullptr));
ListTy *Anchor = static_cast<ListTy *>(this);
return reinterpret_cast<ItemParentClass*>(reinterpret_cast<char*>(Anchor)-
Offset);
|
efriedma-quic
approved these changes
May 29, 2025
| static_cast<ValueSubClass *>( | ||
| nullptr)))); | ||
| size_t Offset = ItemParentClass::getSublistOffset( | ||
| static_cast<ValueSubClass *>(nullptr)); | ||
| ListTy *Anchor = static_cast<ListTy *>(this); | ||
| return reinterpret_cast<ItemParentClass*>(reinterpret_cast<char*>(Anchor)- |
There was a problem hiding this comment.
I'm not sure C++ semantics actually allow this arithmetic, strictly speaking; you might need to go though uintptr_t. But I don't want to try to address that in this patch.
nikic
approved these changes
May 29, 2025
|
FYI, getting a bunch of these warnings when building LLVM: |
|
Clang builds are heavily warning about this change: https://en.cppreference.com/w/cpp/types/offsetof.html says
|
I noticed this issue when drafting this patch. But we don't have a better solution :( |
|
Ok, but spamming compilation warnings for almost every TU isn't a solution either. If it's not easy to fix, I would propose to revert both PRs for the time being. |
dtcxzyw
added a commit
that referenced
this pull request
May 30, 2025
Reverts #139096 due to invalid uses of `offsetof` on non-standard-layout types.
|
@dtcxzyw I am no expert in what you are trying to do here, but I wonder if disabling specific warnings with |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
May 30, 2025
Reverts llvm/llvm-project#139096 due to invalid uses of `offsetof` on non-standard-layout types.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/29466 Here is the relevant piece of the build log for the reference |
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
This patch fixes the "dereferencing null" UB. Unfortunately, C++ doesn't provide an inverse operation for `p->*pmf`. See also llvm#130952.
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
Reverts llvm#139096 due to invalid uses of `offsetof` on non-standard-layout types.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch fixes the "dereferencing null" UB. Unfortunately, C++ doesn't provide an inverse operation for
p->*pmf.See also #130952.