[clang][Interp] Check one-past-the-end pointers in GetPtrField

[tbaeder]

Summary

Rename CheckBaseDerived to something more general and call it in
GetPtrField() as well, so we don't crash later in Pointer::toAPValue().

[tbaeder]

Ping

[aaron.ballman]

LGTM

clang/test/AST/Interp/records.cpp

533–557

Neat! For the tests in this namespace, Clang and ICC agree, GCC and MSVC agree, and users get to cry: https://godbolt.org/z/7EWWrY5z6

[shafik]

clang/test/AST/Interp/records.cpp

533–557

Yeah, unfortunate but I am pretty sure clang is correct on both of those.

[aaron.ballman]

clang/test/AST/Interp/records.cpp

533–557

I think Clang is correct on one of those, but not both of them. I think Clang is correct to reject constexpr const int *pn = &(&b + 1)->n; as that involves dereferencing the one-past-the-end pointer, but I think Clang is wrong to reject constexpr A *a2 = &b + 1; as there's no dereference there.

[shafik]

clang/test/AST/Interp/records.cpp

533–557

@rsmith it looks like you added this diagnostic a while ago see ce1ec5e1f5620 did you mean it to apply to this case? The rules have changed a lot since then so perhaps this was invalid but now is good.

clang/lib/AST/Interp/Interp.h

 /// Checks if a pointer is in range.
 bool CheckRange(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
                 AccessKinds AK);
 
 /// Checks if a field from which a pointer is going to be derived is valid.
 bool CheckRange(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
                 CheckSubobjectKind CSK);
 
-/// Checks if accessing a base or derived record of the given pointer is valid.
-bool CheckBaseDerived(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
-                      CheckSubobjectKind CSK);
+/// Checks if Ptr is a one-past-the-end pointer.
+bool CheckSubobject(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
 
 /// Checks if a pointer points to const storage.
 bool CheckConst(InterpState &S, CodePtr OpPC, const Pointer &Ptr);
 
 /// Checks if a pointer points to a mutable field.
 bool CheckMutable(InterpState &S, CodePtr OpPC, const Pointer &Ptr);
 
 /// Checks if a value can be loaded from a block.

 inline bool GetPtrField(InterpState &S, CodePtr OpPC, uint32_t Off) {
   const Pointer &Ptr = S.Stk.pop<Pointer>();
   if (!CheckNull(S, OpPC, Ptr, CSK_Field))
     return false;
   if (!CheckExtern(S, OpPC, Ptr))
     return false;
   if (!CheckRange(S, OpPC, Ptr, CSK_Field))
     return false;
+  if (!CheckSubobject(S, OpPC, Ptr, CSK_Field))
+    return false;
+
   S.Stk.push<Pointer>(Ptr.atField(Off));
   return true;
 }
 
 inline bool GetPtrThisField(InterpState &S, CodePtr OpPC, uint32_t Off) {
   if (S.checkingPotentialConstantExpression())
     return false;
   const Pointer &This = S.Current->getThis();
if (S.checkingPotentialConstantExpression())
   S.Stk.push<Pointer>(std::move(Field));
   return true;
 }
 
 inline bool GetPtrDerivedPop(InterpState &S, CodePtr OpPC, uint32_t Off) {
   const Pointer &Ptr = S.Stk.pop<Pointer>();
   if (!CheckNull(S, OpPC, Ptr, CSK_Derived))
     return false;
-  if (!CheckBaseDerived(S, OpPC, Ptr, CSK_Derived))
+  if (!CheckSubobject(S, OpPC, Ptr, CSK_Derived))
     return false;
   S.Stk.push<Pointer>(Ptr.atFieldSub(Off));
   return true;
 }
 
 inline bool GetPtrBase(InterpState &S, CodePtr OpPC, uint32_t Off) {
   const Pointer &Ptr = S.Stk.peek<Pointer>();
   if (!CheckNull(S, OpPC, Ptr, CSK_Base))
     return false;
-  if (!CheckBaseDerived(S, OpPC, Ptr, CSK_Base))
+  if (!CheckSubobject(S, OpPC, Ptr, CSK_Base))
     return false;
   S.Stk.push<Pointer>(Ptr.atField(Off));
   return true;
 }
 
 inline bool GetPtrBasePop(InterpState &S, CodePtr OpPC, uint32_t Off) {
   const Pointer &Ptr = S.Stk.pop<Pointer>();
   if (!CheckNull(S, OpPC, Ptr, CSK_Base))
     return false;
-  if (!CheckBaseDerived(S, OpPC, Ptr, CSK_Base))
+  if (!CheckSubobject(S, OpPC, Ptr, CSK_Base))
     return false;
   S.Stk.push<Pointer>(Ptr.atField(Off));
   return true;
 }
 
 inline bool GetPtrThisBase(InterpState &S, CodePtr OpPC, uint32_t Off) {
   if (S.checkingPotentialConstantExpression())
     return false;

clang/lib/AST/Interp/Interp.cpp

bool CheckRange(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
                 CheckSubobjectKind CSK) {
   if (!Ptr.isElementPastEnd())
     return true;
   const SourceInfo &Loc = S.Current->getSource(OpPC);
   S.FFDiag(Loc, diag::note_constexpr_past_end_subobject) << CSK;
   return false;
 }
 
-bool CheckBaseDerived(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
-                      CheckSubobjectKind CSK) {
+bool CheckSubobject(InterpState &S, CodePtr OpPC, const Pointer &Ptr,
   if (!Ptr.isOnePastEnd())
     return true;
 
   const SourceInfo &Loc = S.Current->getSource(OpPC);
   S.FFDiag(Loc, diag::note_constexpr_past_end_subobject) << CSK;
   return false;
 }
 

clang/test/AST/Interp/records.cpp

namespace DeclRefs {
   constexpr B b;
   /// FIXME: The following two lines don't work because we don't get the
   ///   pointers on the LHS correct. They make us run into an assertion
   ///   in CheckEvaluationResult. However, this may just be caused by the
   ///   problems in the previous examples.
   //static_assert(b.a.m == 100, "");
   //static_assert(b.a.f == 100, "");
 }
 
+namespace PointerArith {
+  struct A {};
+  struct B : A { int n; };
+
+  B b = {};
+  constexpr A *a1 = &b;
+  constexpr B *b1 = &b + 1;
+  constexpr B *b2 = &b + 0;
+
+#if 0
+  constexpr A *a2 = &b + 1; // expected-error {{must be initialized by a constant expression}} \
+                            // expected-note {{cannot access base class of pointer past the end of object}} \
+                            // ref-error {{must be initialized by a constant expression}} \
+                            // ref-note {{cannot access base class of pointer past the end of object}}
+
+#endif
+  constexpr const int *pn = &(&b + 1)->n; // expected-error {{must be initialized by a constant expression}} \
+                                          // expected-note {{cannot access field of pointer past the end of object}} \
+                                          // ref-error {{must be initialized by a constant expression}} \
+                                          // ref-note {{cannot access field of pointer past the end of object}}
+
+}
+
 #if __cplusplus >= 202002L
 namespace VirtualCalls {
 namespace Obvious {
 
   class A {
   public:
     constexpr A(){}
     constexpr virtual int foo() {
       return 3;