Add missing check for constant evaluation of a comparison of a pointer
authorRichard Smith <richard@metafoo.co.uk>
Tue, 13 Dec 2022 00:39:14 +0000 (16:39 -0800)
committerRichard Smith <richard@metafoo.co.uk>
Tue, 13 Dec 2022 01:09:26 +0000 (17:09 -0800)
to member naming a weak member to nullptr.

This fixes a miscompile where constant evaluation would incorrectly
determine that a weak member function pointer is never null.

In passing, also improve the diagnostics for constant evaluation of some
nearby cases.

14 files changed:
clang/docs/ReleaseNotes.rst
clang/include/clang/Basic/DiagnosticASTKinds.td
clang/lib/AST/ExprConstant.cpp
clang/test/AST/Interp/cxx20.cpp
clang/test/CXX/drs/dr0xx.cpp
clang/test/CXX/drs/dr16xx.cpp
clang/test/CXX/expr/expr.const/p2-0x.cpp
clang/test/CodeGenCXX/weak-external.cpp
clang/test/Sema/const-eval.c
clang/test/SemaCXX/attr-weak.cpp
clang/test/SemaCXX/builtins.cpp
clang/test/SemaCXX/constant-expression-cxx11.cpp
clang/www/cxx_dr_status.html
clang/www/make_cxx_dr_status

index 53155b0..3aef8c9 100644 (file)
@@ -321,6 +321,9 @@ Bug Fixes
   `Issue 59100 <https://github.com/llvm/llvm-project/issues/59100>`_
 - Fix issue using __attribute__((format)) on non-variadic functions that expect
   more than one formatted argument.
+- Fix bug where constant evaluation treated a pointer to member that points to
+  a weak member as never being null. Such comparisons are now treated as
+  non-constant.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
index 6a2d15e..d4303ef 100644 (file)
@@ -84,7 +84,23 @@ def note_constexpr_pointer_subtraction_not_same_array : Note<
 def note_constexpr_pointer_subtraction_zero_size : Note<
   "subtraction of pointers to type %0 of zero size">;
 def note_constexpr_pointer_comparison_unspecified : Note<
-  "comparison has unspecified value">;
+  "comparison between '%0' and '%1' has unspecified value">;
+def note_constexpr_pointer_constant_comparison : Note<
+  "comparison of numeric address '%0' with pointer '%1' can only be performed "
+  "at runtime">;
+def note_constexpr_literal_comparison : Note<
+  "comparison of addresses of literals has unspecified value">;
+def note_constexpr_pointer_weak_comparison : Note<
+  "comparison against address of weak declaration '%0' can only be performed "
+  "at runtime">;
+def note_constexpr_mem_pointer_weak_comparison : Note<
+  "comparison against pointer to weak member %q0 can only be performed "
+  "at runtime">;
+def note_constexpr_pointer_comparison_past_end : Note<
+  "comparison against pointer '%0' that points past the end of a "
+  "complete object has unspecified value">;
+def note_constexpr_pointer_comparison_zero_sized : Note<
+  "comparison of pointers '%0' and '%1' to unrelated zero-sized objects">;
 def note_constexpr_pointer_comparison_base_classes : Note<
   "comparison of addresses of subobjects of different base classes "
   "has unspecified value">;
index 280b37a..8fd4619 100644 (file)
@@ -2483,6 +2483,7 @@ static bool EvalPointerValueAsBool(const APValue &Value, bool &Result) {
   // A null base expression indicates a null pointer.  These are always
   // evaluatable, and they are false unless the offset is zero.
   if (!Value.getLValueBase()) {
+    // TODO: Should a non-null pointer with an offset of zero evaluate to true?
     Result = !Value.getLValueOffset().isZero();
     return true;
   }
@@ -2495,6 +2496,7 @@ static bool EvalPointerValueAsBool(const APValue &Value, bool &Result) {
 }
 
 static bool HandleConversionToBool(const APValue &Val, bool &Result) {
+  // TODO: This function should produce notes if it fails.
   switch (Val.getKind()) {
   case APValue::None:
   case APValue::Indeterminate:
@@ -2519,6 +2521,9 @@ static bool HandleConversionToBool(const APValue &Val, bool &Result) {
   case APValue::LValue:
     return EvalPointerValueAsBool(Val, Result);
   case APValue::MemberPointer:
+    if (Val.getMemberPointerDecl() && Val.getMemberPointerDecl()->isWeak()) {
+      return false;
+    }
     Result = Val.getMemberPointerDecl();
     return true;
   case APValue::Vector:
@@ -12938,41 +12943,55 @@ EvaluateComparisonBinaryOperator(EvalInfo &Info, const BinaryOperator *E,
     // Reject differing bases from the normal codepath; we special-case
     // comparisons to null.
     if (!HasSameBase(LHSValue, RHSValue)) {
+      auto DiagComparison = [&] (unsigned DiagID, bool Reversed = false) {
+        std::string LHS = LHSValue.toString(Info.Ctx, E->getLHS()->getType());
+        std::string RHS = RHSValue.toString(Info.Ctx, E->getRHS()->getType());
+        Info.FFDiag(E, DiagID)
+            << (Reversed ? RHS : LHS) << (Reversed ? LHS : RHS);
+        return false;
+      };
       // Inequalities and subtractions between unrelated pointers have
       // unspecified or undefined behavior.
-      if (!IsEquality) {
-        Info.FFDiag(E, diag::note_constexpr_pointer_comparison_unspecified);
-        return false;
-      }
+      if (!IsEquality)
+        return DiagComparison(
+            diag::note_constexpr_pointer_comparison_unspecified);
       // A constant address may compare equal to the address of a symbol.
       // The one exception is that address of an object cannot compare equal
       // to a null pointer constant.
+      // TODO: Should we restrict this to actual null pointers, and exclude the
+      // case of zero cast to pointer type?
       if ((!LHSValue.Base && !LHSValue.Offset.isZero()) ||
           (!RHSValue.Base && !RHSValue.Offset.isZero()))
-        return Error(E);
+        return DiagComparison(diag::note_constexpr_pointer_constant_comparison,
+                              !RHSValue.Base);
       // It's implementation-defined whether distinct literals will have
       // distinct addresses. In clang, the result of such a comparison is
       // unspecified, so it is not a constant expression. However, we do know
       // that the address of a literal will be non-null.
       if ((IsLiteralLValue(LHSValue) || IsLiteralLValue(RHSValue)) &&
           LHSValue.Base && RHSValue.Base)
-        return Error(E);
+        return DiagComparison(diag::note_constexpr_literal_comparison);
       // We can't tell whether weak symbols will end up pointing to the same
       // object.
       if (IsWeakLValue(LHSValue) || IsWeakLValue(RHSValue))
-        return Error(E);
+        return DiagComparison(diag::note_constexpr_pointer_weak_comparison,
+                              !IsWeakLValue(LHSValue));
       // We can't compare the address of the start of one object with the
       // past-the-end address of another object, per C++ DR1652.
-      if ((LHSValue.Base && LHSValue.Offset.isZero() &&
-           isOnePastTheEndOfCompleteObject(Info.Ctx, RHSValue)) ||
-          (RHSValue.Base && RHSValue.Offset.isZero() &&
-           isOnePastTheEndOfCompleteObject(Info.Ctx, LHSValue)))
-        return Error(E);
+      if (LHSValue.Base && LHSValue.Offset.isZero() &&
+          isOnePastTheEndOfCompleteObject(Info.Ctx, RHSValue))
+        return DiagComparison(diag::note_constexpr_pointer_comparison_past_end,
+                              true);
+      if (RHSValue.Base && RHSValue.Offset.isZero() &&
+           isOnePastTheEndOfCompleteObject(Info.Ctx, LHSValue))
+        return DiagComparison(diag::note_constexpr_pointer_comparison_past_end,
+                              false);
       // We can't tell whether an object is at the same address as another
       // zero sized object.
       if ((RHSValue.Base && isZeroSized(LHSValue)) ||
           (LHSValue.Base && isZeroSized(RHSValue)))
-        return Error(E);
+        return DiagComparison(
+            diag::note_constexpr_pointer_comparison_zero_sized);
       return Success(CmpResult::Unequal, E);
     }
 
@@ -13076,6 +13095,19 @@ EvaluateComparisonBinaryOperator(EvalInfo &Info, const BinaryOperator *E,
     if (!EvaluateMemberPointer(E->getRHS(), RHSValue, Info) || !LHSOK)
       return false;
 
+    // If either operand is a pointer to a weak function, the comparison is not
+    // constant.
+    if (LHSValue.getDecl() && LHSValue.getDecl()->isWeak()) {
+      Info.FFDiag(E, diag::note_constexpr_mem_pointer_weak_comparison)
+          << LHSValue.getDecl();
+      return true;
+    }
+    if (RHSValue.getDecl() && RHSValue.getDecl()->isWeak()) {
+      Info.FFDiag(E, diag::note_constexpr_mem_pointer_weak_comparison)
+          << RHSValue.getDecl();
+      return true;
+    }
+
     // C++11 [expr.eq]p2:
     //   If both operands are null, they compare equal. Otherwise if only one is
     //   null, they compare unequal.
index 6445abd..38f0584 100644 (file)
@@ -101,6 +101,7 @@ constexpr bool b1 = foo(p1) == foo(p1);
 static_assert(b1);
 
 constexpr bool b2 = foo(p1) == foo(p2); // ref-error {{must be initialized by a constant expression}} \
+                                        // ref-note {{comparison of addresses of literals}} \
                                         // ref-note {{declared here}}
 static_assert(!b2); // ref-error {{not an integral constant expression}} \
                     // ref-note {{not a constant expression}}
@@ -111,6 +112,7 @@ constexpr auto name2() { return "name2"; }
 constexpr auto b3 = name1() == name1();
 static_assert(b3);
 constexpr auto b4 = name1() == name2(); // ref-error {{must be initialized by a constant expression}} \
+                                        // ref-note {{has unspecified value}} \
                                         // ref-note {{declared here}}
 static_assert(!b4); // ref-error {{not an integral constant expression}} \
                     // ref-note {{not a constant expression}}
index e34dc75..6e7c5c5 100644 (file)
@@ -988,10 +988,10 @@ namespace dr70 { // dr70: yes
 // dr72: dup 69
 
 #if __cplusplus >= 201103L
-namespace dr73 { // dr73: no
-  // The resolution to dr73 is unworkable. Consider:
+namespace dr73 { // dr73: sup 1652
   int a, b;
   static_assert(&a + 1 != &b, ""); // expected-error {{not an integral constant expression}}
+  // expected-note@-1 {{comparison against pointer '&a + 1' that points past the end of a complete object}}
 }
 #endif
 
index 83b4447..6f8ced5 100644 (file)
@@ -42,6 +42,12 @@ namespace dr1611 { // dr1611: dup 1658
   C c;
 }
 
+namespace dr1652 { // dr1652: 3.6
+  int a, b;
+  int arr[&a + 1 == &b ? 1 : 2]; // expected-error 2{{variable length array}}
+                                 // expected-note@-1 {{points past the end}}
+}
+
 namespace dr1684 { // dr1684: 3.6
 #if __cplusplus >= 201103L
   struct NonLiteral { // expected-note {{because}}
index 8b10f33..d8b228f 100644 (file)
@@ -499,22 +499,22 @@ namespace UnspecifiedRelations {
   // different objects that are not members of the same array or to different
   // functions, or if only one of them is null, the results of p<q, p>q, p<=q,
   // and p>=q are unspecified.
-  constexpr bool u1 = p < q; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
-  constexpr bool u2 = p > q; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
-  constexpr bool u3 = p <= q; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
-  constexpr bool u4 = p >= q; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
-  constexpr bool u5 = p < (int*)0; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
-  constexpr bool u6 = p <= (int*)0; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
-  constexpr bool u7 = p > (int*)0; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
-  constexpr bool u8 = p >= (int*)0; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
-  constexpr bool u9 = (int*)0 < q; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
-  constexpr bool u10 = (int*)0 <= q; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
-  constexpr bool u11 = (int*)0 > q; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
-  constexpr bool u12 = (int*)0 >= q; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
+  constexpr bool u1 = p < q; // expected-error {{constant expression}} expected-note {{comparison between '&a' and '&b' has unspecified value}}
+  constexpr bool u2 = p > q; // expected-error {{constant expression}} expected-note {{comparison between '&a' and '&b' has unspecified value}}
+  constexpr bool u3 = p <= q; // expected-error {{constant expression}} expected-note {{comparison between '&a' and '&b' has unspecified value}}
+  constexpr bool u4 = p >= q; // expected-error {{constant expression}} expected-note {{comparison between '&a' and '&b' has unspecified value}}
+  constexpr bool u5 = p < (int*)0; // expected-error {{constant expression}} expected-note {{comparison between '&a' and 'nullptr' has unspecified value}}
+  constexpr bool u6 = p <= (int*)0; // expected-error {{constant expression}} expected-note {{comparison between '&a' and 'nullptr' has unspecified value}}
+  constexpr bool u7 = p > (int*)0; // expected-error {{constant expression}} expected-note {{comparison between '&a' and 'nullptr' has unspecified value}}
+  constexpr bool u8 = p >= (int*)0; // expected-error {{constant expression}} expected-note {{comparison between '&a' and 'nullptr' has unspecified value}}
+  constexpr bool u9 = (int*)0 < q; // expected-error {{constant expression}} expected-note {{comparison between 'nullptr' and '&b' has unspecified value}}
+  constexpr bool u10 = (int*)0 <= q; // expected-error {{constant expression}} expected-note {{comparison between 'nullptr' and '&b' has unspecified value}}
+  constexpr bool u11 = (int*)0 > q; // expected-error {{constant expression}} expected-note {{comparison between 'nullptr' and '&b' has unspecified value}}
+  constexpr bool u12 = (int*)0 >= q; // expected-error {{constant expression}} expected-note {{comparison between 'nullptr' and '&b' has unspecified value}}
   void f(), g();
 
   constexpr void (*pf)() = &f, (*pg)() = &g;
-  constexpr bool u13 = pf < pg; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
+  constexpr bool u13 = pf < pg; // expected-error {{constant expression}} expected-note {{comparison between '&f' and '&g' has unspecified value}}
                                 // expected-warning@-1 {{ordered comparison of function pointers}}
   constexpr bool u14 = pf == pg;
 
@@ -566,11 +566,11 @@ namespace UnspecifiedRelations {
   constexpr void *pv = (void*)&s.a;
   constexpr void *qv = (void*)&s.b;
   constexpr bool v1 = null < (int*)0;
-  constexpr bool v2 = null < pv; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
+  constexpr bool v2 = null < pv; // expected-error {{constant expression}} expected-note {{comparison between 'nullptr' and '&s.a' has unspecified value}}
   constexpr bool v3 = null == pv; // ok
   constexpr bool v4 = qv == pv; // ok
   constexpr bool v5 = qv >= pv; // expected-error {{constant expression}} expected-note {{unequal pointers to void}}
-  constexpr bool v6 = qv > null; // expected-error {{constant expression}} expected-note {{comparison has unspecified value}}
+  constexpr bool v6 = qv > null; // expected-error {{constant expression}} expected-note {{comparison between '&s.b' and 'nullptr' has unspecified value}}
   constexpr bool v7 = qv <= (void*)&s.b; // ok
   constexpr bool v8 = qv > (void*)&s.a; // expected-error {{constant expression}} expected-note {{unequal pointers to void}}
 }
index 433fb3c..34af202 100644 (file)
@@ -76,3 +76,23 @@ namespace not_weak_on_first {
       func,
   };
 }
+
+namespace constant_eval {
+  [[gnu::weak]] extern int a;
+  // CHECK-LABEL: define {{.*}} @__cxx_global_var_init
+  // CHECK:     store i8 zext (i1 icmp ne (ptr @_ZN13constant_eval1aE, ptr null) to i8), ptr @_ZN13constant_eval6has_a1E,
+  bool has_a1 = &a;
+  // CHECK-LABEL: define {{.*}} @__cxx_global_var_init
+  // CHECK:     store i8 zext (i1 icmp ne (ptr @_ZN13constant_eval1aE, ptr null) to i8), ptr @_ZN13constant_eval6has_a2E,
+  bool has_a2 = &a != nullptr;
+
+  struct X {
+    [[gnu::weak]] void f();
+  };
+  // CHECK-LABEL: define {{.*}} @__cxx_global_var_init
+  // CHECK:     store i8 zext (i1 icmp ne (i64 ptrtoint (ptr @_ZN13constant_eval1X1fEv to i64), i64 0) to i8), ptr @_ZN13constant_eval6has_f1E,
+  bool has_f1 = &X::f;
+  // CHECK-LABEL: define {{.*}} @__cxx_global_var_init
+  // CHECK:     store i8 zext (i1 icmp ne (i64 ptrtoint (ptr @_ZN13constant_eval1X1fEv to i64), i64 0) to i8), ptr @_ZN13constant_eval6has_f2E,
+  bool has_f2 = &X::f != nullptr;
+}
index 286a58f..bbffc79 100644 (file)
@@ -138,6 +138,7 @@ void PR21945(void) { int i = (({}), 0l); }
 void PR24622(void);
 struct PR24622 {} pr24622;
 EVAL_EXPR(52, &pr24622 == (void *)&PR24622); // expected-error {{not an integer constant expression}}
+                                             // expected-note@-1 {{past the end}}
 
 // We evaluate these by providing 2s' complement semantics in constant
 // expressions, like we do for integers.
index 51deb66..f065bfd 100644 (file)
@@ -38,3 +38,20 @@ template struct Test7<int>;
 class __attribute__((weak)) Test8 {}; // OK
 
 __attribute__((weak)) auto Test9 = Internal(); // expected-error {{weak declaration cannot have internal linkage}}
+
+[[gnu::weak]] void weak_function();
+struct WithWeakMember {
+  [[gnu::weak]] void weak_method();
+  [[gnu::weak]] virtual void virtual_weak_method();
+};
+constexpr bool weak_function_is_non_null = &weak_function != nullptr; // expected-error {{must be initialized by a constant expression}}
+// expected-note@-1 {{comparison against address of weak declaration '&weak_function' can only be performed at runtime}}
+constexpr bool weak_method_is_non_null = &WithWeakMember::weak_method != nullptr; // expected-error {{must be initialized by a constant expression}}
+// expected-note@-1 {{comparison against pointer to weak member 'WithWeakMember::weak_method' can only be performed at runtime}}
+// GCC accepts this and says the result is always non-null. That's consistent
+// with the ABI rules for member pointers, but seems unprincipled, so we do not
+// follow it for now.
+// TODO: Consider warning on such comparisons, as they do not test whether the
+// virtual member function is present.
+constexpr bool virtual_weak_method_is_non_null = &WithWeakMember::virtual_weak_method != nullptr; // expected-error {{must be initialized by a constant expression}}
+// expected-note@-1 {{comparison against pointer to weak member 'WithWeakMember::virtual_weak_method' can only be performed at runtime}}
index 300bc4a..3687acd 100644 (file)
@@ -48,6 +48,7 @@ void a(void) {}
 int n;
 void *p = __builtin_function_start(n);               // expected-error {{argument must be a function}}
 static_assert(__builtin_function_start(a) == a, ""); // expected-error {{static assertion expression is not an integral constant expression}}
+// expected-note@-1 {{comparison of addresses of literals has unspecified value}}
 } // namespace function_start
 
 void no_ms_builtins() {
index 0d2b108..fea45d5 100644 (file)
@@ -359,7 +359,8 @@ struct Str {
 extern char externalvar[];
 constexpr bool constaddress = (void *)externalvar == (void *)0x4000UL; // expected-error {{must be initialized by a constant expression}} expected-note {{reinterpret_cast}}
 constexpr bool litaddress = "foo" == "foo"; // expected-error {{must be initialized by a constant expression}}
-// cxx20_2b-warning@-1 {{comparison between two arrays is deprecated}}
+// expected-note@-1 {{comparison of addresses of literals has unspecified value}}
+// cxx20_2b-warning@-2 {{comparison between two arrays is deprecated}}
 static_assert(0 != "foo", "");
 
 }
@@ -2181,6 +2182,7 @@ namespace PR19010 {
 
 void PR21327(int a, int b) {
   static_assert(&a + 1 != &b, ""); // expected-error {{constant expression}}
+  // expected-note@-1 {{comparison against pointer '&a + 1' that points past the end of a complete object has unspecified value}}
 }
 
 namespace EmptyClass {
@@ -2200,6 +2202,7 @@ namespace PR21786 {
   extern void (*start[])();
   extern void (*end[])();
   static_assert(&start != &end, ""); // expected-error {{constant expression}}
+  // expected-note@-1 {{comparison of pointers '&start' and '&end' to unrelated zero-sized objects}}
   static_assert(&start != nullptr, "");
 
   struct Foo;
index f131df7..e171ef4 100755 (executable)
     <td><a href="https://wg21.link/cwg73">73</a></td>
     <td>TC1</td>
     <td>Pointer equality</td>
-    <td class="none" align="center">No</td>
+    <td class="full" align="center">Superseded by <a href="#1652">1652</a></td>
   </tr>
   <tr id="74">
     <td><a href="https://wg21.link/cwg74">74</a></td>
@@ -9719,7 +9719,7 @@ and <I>POD class</I></td>
     <td><a href="https://wg21.link/cwg1652">1652</a></td>
     <td>CD4</td>
     <td>Object addresses in <TT>constexpr</TT> expressions</td>
-    <td class="none" align="center">Unknown</td>
+    <td class="full" align="center">Clang 3.6</td>
   </tr>
   <tr id="1653">
     <td><a href="https://wg21.link/cwg1653">1653</a></td>
index 1b4fdb6..2059221 100755 (executable)
@@ -15,10 +15,14 @@ class DR:
     return '%s (%s): %s' % (self.issue, self.status, self.title)
 
 def parse(dr):
-  section, issue_link, status, liaison, title = [
-      col.split('>', 1)[1].split('</TD>')[0]
-      for col in dr.split('</TR>', 1)[0].split('<TD')[1:]
-  ]
+  try:
+    section, issue_link, status, liaison, title = [
+        col.split('>', 1)[1].split('</TD>')[0]
+        for col in dr.split('</TR>', 1)[0].split('<TD')[1:]
+    ]
+  except Exception as ex:
+    print(f"Parse error: {ex}\n{dr}", file=sys.stderr)
+    sys.exit(1)
   _, url, issue = issue_link.split('"', 2)
   url = url.strip()
   issue = int(issue.split('>', 1)[1].split('<', 1)[0])