Skip to content

[clang][bytecode] Fix comparing the addresses of union members #133852

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.

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

Merged
merged 1 commit into from
Apr 1, 2025

Conversation

tbaederr
Copy link
Contributor

@tbaederr tbaederr commented Apr 1, 2025

Union members get the same address, so we can't just use Pointer::getByteOffset().

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 1, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 1, 2025

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Union members get the same address, so we can't just use Pointer::getByteOffset().


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

4 Files Affected:

  • (modified) clang/lib/AST/ByteCode/Interp.h (+10-1)
  • (modified) clang/lib/AST/ByteCode/Pointer.cpp (+31)
  • (modified) clang/lib/AST/ByteCode/Pointer.h (+3-1)
  • (modified) clang/test/AST/ByteCode/unions.cpp (+38)
diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h
index 938077a9f10ae..6fe1d4b1f95ae 100644
--- a/clang/lib/AST/ByteCode/Interp.h
+++ b/clang/lib/AST/ByteCode/Interp.h
@@ -1070,9 +1070,18 @@ inline bool CmpHelperEQ<Pointer>(InterpState &S, CodePtr OpPC, CompareFn Fn) {
   }
 
   if (Pointer::hasSameBase(LHS, RHS)) {
+    if (LHS.inUnion() && RHS.inUnion()) {
+      // If the pointers point into a union, things are a little more
+      // complicated since the offset we save in interp::Pointer can't be used
+      // to compare the pointers directly.
+      size_t A = LHS.computeOffsetForComparison();
+      size_t B = RHS.computeOffsetForComparison();
+      S.Stk.push<BoolT>(BoolT::from(Fn(Compare(A, B))));
+      return true;
+    }
+
     unsigned VL = LHS.getByteOffset();
     unsigned VR = RHS.getByteOffset();
-
     // In our Pointer class, a pointer to an array and a pointer to the first
     // element in the same array are NOT equal. They have the same Base value,
     // but a different Offset. This is a pretty rare case, so we fix this here
diff --git a/clang/lib/AST/ByteCode/Pointer.cpp b/clang/lib/AST/ByteCode/Pointer.cpp
index 79b47c26992ae..7ccefd6aeb9e1 100644
--- a/clang/lib/AST/ByteCode/Pointer.cpp
+++ b/clang/lib/AST/ByteCode/Pointer.cpp
@@ -339,6 +339,37 @@ void Pointer::print(llvm::raw_ostream &OS) const {
   }
 }
 
+/// Compute an integer that can be used to compare this pointer to
+/// another one.
+size_t Pointer::computeOffsetForComparison() const {
+  if (!isBlockPointer())
+    return Offset;
+
+  size_t Result = 0;
+  Pointer P = *this;
+  while (!P.isRoot()) {
+    if (P.isArrayRoot()) {
+      P = P.getBase();
+      continue;
+    }
+    if (P.isArrayElement()) {
+      Result += (P.getIndex() * P.elemSize());
+      P = P.getArray();
+    }
+
+    if (const Record *R = P.getBase().getRecord(); R && R->isUnion()) {
+      // Direct child of a union - all have offset 0.
+      P = P.getBase();
+      continue;
+    }
+
+    Result += P.getInlineDesc()->Offset;
+    P = P.getBase();
+  }
+
+  return Result;
+}
+
 std::string Pointer::toDiagnosticString(const ASTContext &Ctx) const {
   if (isZero())
     return "nullptr";
diff --git a/clang/lib/AST/ByteCode/Pointer.h b/clang/lib/AST/ByteCode/Pointer.h
index fd33ee9955f55..988237d39fff4 100644
--- a/clang/lib/AST/ByteCode/Pointer.h
+++ b/clang/lib/AST/ByteCode/Pointer.h
@@ -417,7 +417,7 @@ class Pointer {
     return false;
   }
   bool inUnion() const {
-    if (isBlockPointer())
+    if (isBlockPointer() && asBlockPointer().Base >= sizeof(InlineDescriptor))
       return getInlineDesc()->InUnion;
     return false;
   };
@@ -727,6 +727,8 @@ class Pointer {
   /// Prints the pointer.
   void print(llvm::raw_ostream &OS) const;
 
+  size_t computeOffsetForComparison() const;
+
 private:
   friend class Block;
   friend class DeadBlock;
diff --git a/clang/test/AST/ByteCode/unions.cpp b/clang/test/AST/ByteCode/unions.cpp
index 66b8389606b85..3911a2b2f7dde 100644
--- a/clang/test/AST/ByteCode/unions.cpp
+++ b/clang/test/AST/ByteCode/unions.cpp
@@ -600,3 +600,41 @@ namespace MoveOrAssignOp {
   static_assert(foo());
 }
 #endif
+
+namespace AddressComparison {
+  union {
+    int a;
+    int c;
+  } U;
+  static_assert(__builtin_addressof(U.a) == (void*)__builtin_addressof(U.c));
+  static_assert(&U.a == &U.c);
+
+
+  struct {
+    union {
+      struct {
+        int a;
+        int b;
+      } a;
+      struct {
+        int b;
+        int a;
+      }b;
+    } u;
+    int b;
+  } S;
+
+  static_assert(&S.u.a.a == &S.u.b.b);
+  static_assert(&S.u.a.b != &S.u.b.b);
+  static_assert(&S.u.a.b == &S.u.b.b); // both-error {{failed}}
+
+
+  union {
+    int a[2];
+    int b[2];
+  } U2;
+
+  static_assert(&U2.a[0] == &U2.b[0]);
+  static_assert(&U2.a[0] != &U2.b[1]);
+  static_assert(&U2.a[0] == &U2.b[1]); // both-error {{failed}}
+}

Union members get the same address, so we can't just use
`Pointer::getByteOffset()`.
@tbaederr tbaederr force-pushed the union-member-address branch from fa0d897 to 2bad7af Compare April 1, 2025 05:48
@tbaederr tbaederr merged commit 7267dbf into llvm:main Apr 1, 2025
11 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Apr 1, 2025

LLVM Buildbot has detected a new failure on builder lldb-x86_64-debian running on lldb-x86_64-debian while building clang at step 6 "test".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/19266

Here is the relevant piece of the build log for the reference
Step 6 (test) failure: build (failure)
...
PASS: lldb-api :: tools/lldb-dap/attach/TestDAP_attachByPortNum.py (146 of 2804)
PASS: lldb-api :: functionalities/thread/concurrent_events/TestConcurrentSignalDelayWatch.py (147 of 2804)
PASS: lldb-api :: api/command-return-object/TestSBCommandReturnObject.py (148 of 2804)
PASS: lldb-api :: types/TestIntegerTypeExpr.py (149 of 2804)
PASS: lldb-api :: python_api/module_section/TestModuleAndSection.py (150 of 2804)
PASS: lldb-api :: functionalities/inferior-crashing/TestInferiorCrashingStep.py (151 of 2804)
PASS: lldb-api :: functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/TestDataFormatterLibcxxStringSimulator.py (152 of 2804)
PASS: lldb-api :: linux/thread/create_during_instruction_step/TestCreateDuringInstructionStep.py (153 of 2804)
PASS: lldb-api :: lang/cpp/static_members/TestCPPStaticMembers.py (154 of 2804)
PASS: lldb-api :: functionalities/data-formatter/data-formatter-stl/generic/bitset/TestDataFormatterGenericBitset.py (155 of 2804)
FAIL: lldb-api :: lang/cpp/unique-types4/TestUniqueTypes4.py (156 of 2804)
******************** TEST 'lldb-api :: lang/cpp/unique-types4/TestUniqueTypes4.py' FAILED ********************
Script:
--
/usr/bin/python3 /home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/dotest.py -u CXXFLAGS -u CFLAGS --env LLVM_LIBS_DIR=/home/worker/2.0.1/lldb-x86_64-debian/build/./lib --env LLVM_INCLUDE_DIR=/home/worker/2.0.1/lldb-x86_64-debian/build/include --env LLVM_TOOLS_DIR=/home/worker/2.0.1/lldb-x86_64-debian/build/./bin --arch x86_64 --build-dir /home/worker/2.0.1/lldb-x86_64-debian/build/lldb-test-build.noindex --lldb-module-cache-dir /home/worker/2.0.1/lldb-x86_64-debian/build/lldb-test-build.noindex/module-cache-lldb/lldb-api --clang-module-cache-dir /home/worker/2.0.1/lldb-x86_64-debian/build/lldb-test-build.noindex/module-cache-clang/lldb-api --executable /home/worker/2.0.1/lldb-x86_64-debian/build/./bin/lldb --compiler /home/worker/2.0.1/lldb-x86_64-debian/build/./bin/clang --dsymutil /home/worker/2.0.1/lldb-x86_64-debian/build/./bin/dsymutil --make /usr/bin/gmake --llvm-tools-dir /home/worker/2.0.1/lldb-x86_64-debian/build/./bin --lldb-obj-root /home/worker/2.0.1/lldb-x86_64-debian/build/tools/lldb --lldb-libs-dir /home/worker/2.0.1/lldb-x86_64-debian/build/./lib -t /home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/lang/cpp/unique-types4 -p TestUniqueTypes4.py
--
Exit Code: -11

Command Output (stdout):
--
lldb version 21.0.0git (https://github.com/llvm/llvm-project.git revision 7267dbfe1032f5ebd698403848fab4bbfcbe0b19)
  clang revision 7267dbfe1032f5ebd698403848fab4bbfcbe0b19
  llvm revision 7267dbfe1032f5ebd698403848fab4bbfcbe0b19
Skipping the following test categories: ['libc++', 'dsym', 'gmodules', 'debugserver', 'objc']

--
Command Output (stderr):
--
Change dir to: /home/worker/2.0.1/lldb-x86_64-debian/llvm-project/lldb/test/API/lang/cpp/unique-types4
UNSUPPORTED: LLDB (/home/worker/2.0.1/lldb-x86_64-debian/build/bin/clang-x86_64) :: test_no_simple_template_names_dsym (TestUniqueTypes4.UniqueTypesTestCase4.test_no_simple_template_names_dsym) (test case does not fall in any category of interest for this run) 
runCmd: settings clear -all

output: 

runCmd: settings set symbols.enable-external-lookup false

output: 

runCmd: settings set target.inherit-tcc true

output: 

runCmd: settings set target.disable-aslr false

output: 

runCmd: settings set target.detach-on-error false

output: 

Ankur-0429 pushed a commit to Ankur-0429/llvm-project that referenced this pull request Apr 2, 2025
…133852)

Union members get the same address, so we can't just use
`Pointer::getByteOffset()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants