[clang] [clang][bytecode] Diagnose comparing pointers to fields... (PR #137159)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

<details>
<summary>Changes</summary>

... with different access specifiers.

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


4 Files Affected:

- (modified) clang/lib/AST/ByteCode/Interp.h (+13) 
- (modified) clang/lib/AST/ByteCode/Pointer.cpp (+42) 
- (modified) clang/lib/AST/ByteCode/Pointer.h (+8-1) 
- (modified) clang/test/AST/ByteCode/records.cpp (+23) 


``````````diff
diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h
index 99b032bee9e3d..0a52a64240c04 100644
--- a/clang/lib/AST/ByteCode/Interp.h
+++ b/clang/lib/AST/ByteCode/Interp.h
@@ -1007,6 +1007,19 @@ inline bool CmpHelper<Pointer>(InterpState &S, CodePtr OpPC, CompareFn Fn) {
     return false;
   }
 
+  // Diagnose comparisons between fields with different access specifiers.
+  if (std::optional<std::pair<Pointer, Pointer>> Split =
+          Pointer::computeSplitPoint(LHS, RHS)) {
+    const FieldDecl *LF = Split->first.getField();
+    const FieldDecl *RF = Split->second.getField();
+    if (LF && RF && !LF->getParent()->isUnion() &&
+        LF->getAccess() != RF->getAccess()) {
+      S.CCEDiag(S.Current->getSource(OpPC),
+                diag::note_constexpr_pointer_comparison_differing_access)
+          << LF << LF->getAccess() << RF << RF->getAccess() << LF->getParent();
+    }
+  }
+
   unsigned VL = LHS.getByteOffset();
   unsigned VR = RHS.getByteOffset();
   S.Stk.push<BoolT>(BoolT::from(Fn(Compare(VL, VR))));
diff --git a/clang/lib/AST/ByteCode/Pointer.cpp b/clang/lib/AST/ByteCode/Pointer.cpp
index 059503cae3561..8adfd7a1f98d8 100644
--- a/clang/lib/AST/ByteCode/Pointer.cpp
+++ b/clang/lib/AST/ByteCode/Pointer.cpp
@@ -571,6 +571,48 @@ bool Pointer::pointsToLiteral() const {
   return E && !isa<MaterializeTemporaryExpr, StringLiteral>(E);
 }
 
+std::optional<std::pair<Pointer, Pointer>>
+Pointer::computeSplitPoint(const Pointer &A, const Pointer &B) {
+  if (!A.isBlockPointer() || !B.isBlockPointer())
+    return std::nullopt;
+
+  if (A.asBlockPointer().Pointee != B.asBlockPointer().Pointee)
+    return std::nullopt;
+  if (A.isRoot() && B.isRoot())
+    return std::nullopt;
+
+  if (A == B)
+    return std::make_pair(A, B);
+
+  auto getBase = [](const Pointer &P) -> Pointer {
+    if (P.isArrayElement())
+      return P.getArray();
+    return P.getBase();
+  };
+
+  Pointer IterA = A;
+  Pointer IterB = B;
+  Pointer CurA = IterA;
+  Pointer CurB = IterB;
+  for (;;) {
+    if (IterA.asBlockPointer().Base > IterB.asBlockPointer().Base) {
+      CurA = IterA;
+      IterA = getBase(IterA);
+    } else {
+      CurB = IterB;
+      IterB = getBase(IterB);
+    }
+
+    if (IterA == IterB)
+      return std::make_pair(CurA, CurB);
+
+    if (IterA.isRoot() && IterB.isRoot())
+      return std::nullopt;
+  }
+
+  llvm_unreachable("The loop above should've returned.");
+}
+
 std::optional<APValue> Pointer::toRValue(const Context &Ctx,
                                          QualType ResultType) const {
   const ASTContext &ASTCtx = Ctx.getASTContext();
diff --git a/clang/lib/AST/ByteCode/Pointer.h b/clang/lib/AST/ByteCode/Pointer.h
index 8ede706f2736f..e168154a55f58 100644
--- a/clang/lib/AST/ByteCode/Pointer.h
+++ b/clang/lib/AST/ByteCode/Pointer.h
@@ -492,7 +492,11 @@ class Pointer {
     return ElemDesc ? ElemDesc->ElemRecord : nullptr;
   }
   /// Returns the field information.
-  const FieldDecl *getField() const { return getFieldDesc()->asFieldDecl(); }
+  const FieldDecl *getField() const {
+    if (const Descriptor *FD = getFieldDesc())
+      return FD->asFieldDecl();
+    return nullptr;
+  }
 
   /// Checks if the storage is extern.
   bool isExtern() const {
@@ -724,6 +728,9 @@ class Pointer {
   /// Checks if both given pointers point to the same block.
   static bool pointToSameBlock(const Pointer &A, const Pointer &B);
 
+  static std::optional<std::pair<Pointer, Pointer>>
+  computeSplitPoint(const Pointer &A, const Pointer &B);
+
   /// Whether this points to a block that's been created for a "literal lvalue",
   /// i.e. a non-MaterializeTemporaryExpr Expr.
   bool pointsToLiteral() const;
diff --git a/clang/test/AST/ByteCode/records.cpp b/clang/test/AST/ByteCode/records.cpp
index da851785323a5..b4059f009b887 100644
--- a/clang/test/AST/ByteCode/records.cpp
+++ b/clang/test/AST/ByteCode/records.cpp
@@ -1787,3 +1787,26 @@ namespace IntegralBaseCast {
 
   static_assert(f() == 0, "");
 }
+
+namespace AccessMismatch {
+  struct A {
+  public:
+    constexpr A() : a(0), b(0) {}
+    int a;
+    constexpr bool cmp() const { return &a < &b; } // both-note {{comparison of address of fields 'a' and 'b' of 'A' with differing access specifiers (public vs private) has unspecified value}}
+  private:
+    int b;
+  };
+  static_assert(A().cmp(), ""); // both-error {{constant expression}} \
+                                // both-note {{in call}}
+
+  class B {
+  public:
+    A a;
+    constexpr bool cmp() const { return &a.a < &b.a; } // both-note {{comparison of address of fields 'a' and 'b' of 'B' with differing access specifiers (public vs protected) has unspecified value}}
+  protected:
+    A b;
+  };
+  static_assert(B().cmp(), ""); // both-error {{constant expression}} \
+                                // both-note {{in call}}
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/137159