[clang][Interp] Check pointers when accessing base class

[tbaeder]

Ping

[aaron.ballman]

LGTM

[aaron.ballman]

clang/test/AST/Interp/records.cpp

509–512

I may have jumped the gun on accepting this, actually. Forming the pointer to &b + 1 is fine, but evaluating it by dereferencing it would be UB. e.g., http://eel.is/c++draft/expr.const#13.3

[cor3ntin]

clang/test/AST/Interp/records.cpp

509–512

We probably want tests that ensure &b+2 is invalid in all cases

[tbaeder]

clang/test/AST/Interp/records.cpp

509–512

It's being evaluated because the base class is accessed, isn't it? if b was of type A*, we would not emit a diagnostic (see line 506 above).

[aaron.ballman]

clang/test/AST/Interp/records.cpp

509–512

I'm not seeing the access to the base class -- we're forming a pointer, not dereferencing it.

https://godbolt.org/z/vxThqczeo

I think this is an existing Clang bug. CC @hubert.reinterpretcast @rsmith for additional opinions.

[tbaeder]

Ping

[tbaeder]

Ping

[tbaeder]

Ping

[tbaeder]

Ping

[tbaeder]

Ping

[cor3ntin]

clang/test/AST/Interp/records.cpp

509–512

I believe this is fine, or at least i can't find any wording that would say it's not.

In particular:

• taking the address of &b is fine
• adding + 1 to it is fine: http://eel.is/c++draft/expr#add-4.2
• casting it to a pointer to base is fine http://eel.is/c++draft/conv.ptr#3
• treating the whole expression as a constant expression is also fine http://eel.is/c++draft/expr.const#13.3

So i do believe GCC, MSVC and ICC are correct here.

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 of the given pointer is valid.
+bool CheckBase(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.
 bool CheckLoad(InterpState &S, CodePtr OpPC, const Pointer &Ptr);
if (S.checkingPotentialConstantExpression())
   S.Stk.push<Pointer>(std::move(Field));
   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 (!CheckBase(S, OpPC, Ptr))
+    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 (!CheckBase(S, OpPC, Ptr))
+    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;
   const Pointer &This = S.Current->getThis();

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 CheckBase(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_Base;
+  return false;
+}
+
 bool CheckConst(InterpState &S, CodePtr OpPC, const Pointer &Ptr) {
   assert(Ptr.isLive() && "Pointer is not live");
   if (!Ptr.isConst())
     return true;
 
   // The This pointer is writable in constructors and destructors,
   // even if isConst() returns true.
   if (const Function *Func = S.Current->getFunction();

clang/test/AST/Interp/records.cpp

namespace DeclRefs {
   /// 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;
+
+  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}}
+}
+
 #if __cplusplus >= 202002L
 namespace VirtualCalls {
 namespace Obvious {
 
   class A {
   public:
     constexpr A(){}
     constexpr virtual int foo() {