[clang][Interp] Handle undefined functions better

[tbaeder]

Summary

Make sure we add body-less functions to our program as well, and add the actual body later. We then need to compile functions again later once we have the definition with a body.

We do a bunch of work in ByteCodeEmitter::compileFunc() to prepare the values passed to Program::createFunction() even if we don't need them now, that's a possible optimization for a later commit.

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeEmitter.cpp

24–27

hasBody() returns true if any body in the redeclaration chain already has a body, so I'm surprised to see this change -- I don't know if we reset willHaveBody() when we give the function a body, but the logic here seems wrong to me.

clang/lib/AST/Interp/Program.cpp

207–208

Same question here about canonical decl.

clang/lib/AST/Interp/Program.h

96

Are you trying to get the FunctionDecl which represents the function definition, or do you really mean you want the first declaration in the redecl chain?

[tbaeder]

clang/lib/AST/Interp/ByteCodeEmitter.cpp

24–27

I was confused by your question, but it's just about hasBody() vs !hasBody(), right? The old code used a negation and of course that way it makes sense, at least the way I read it. Yes I forgot the negation. :)

clang/lib/AST/Interp/Program.h

96

Whether or not the function has a body is irrelevant here, I think. The Def here is just used for caching. So the first one makes sense, doesn't it?

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeEmitter.cpp

24–27

Heh, you managed to decipher my word salad correctly -- I was tying to figure out if you dropped the ! for a reason. :-D

[aaron.ballman]

LGTM!

clang/lib/AST/Interp/ByteCodeEmitter.cpp

23–27

.... negating the Boolean calculation and applying deMorgan's law did not make that code more clear, did it (assuming I did everything right)? If you agree, then I'm fine with the more complicated form and letting the optimizer make it faster.

clang/lib/AST/Interp/Program.h

96

Hmmm, this may actually be fine. The situation I had in mind is where a redeclaration adds more semantic information to the function declaration that may disqualify it from being a constexpr function due to UB. For example, declare a function with no attribute, then redeclare the function with an attribute accepting an expression argument (like __attribute__((enable_if)) or __attribute__((diagnose_if)) and put the UB in the expression argument.

But I can't devise a test case that hits the problem because any *use* of the constexpr function causes us to claim it's non-constexpr if the function is declared but not defined.

[tbaeder]

clang/lib/AST/Interp/ByteCodeEmitter.cpp

23–27

I can see that going either way. I think your version is a more confusing though because the two body conditions are coupled with the isDefined condition. E.g. HasBody is true if isDefined() && !willHaveBody(), which doesn't make sense to me I think.

I think I just read this code too many times now. How does being defined even relate to the function having a body? Should that code just be HasBody = hasBody() || willHaveBody()?

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeEmitter.cpp

23–27

I can see that going either way. I think your version is a more confusing though because the two body conditions are coupled with the isDefined condition. E.g. HasBody is true if isDefined() && !willHaveBody(), which doesn't make sense to me I think.

I'm glad we both are confused about the same thing!

I think I just read this code too many times now. How does being defined even relate to the function having a body? Should that code just be HasBody = hasBody() || willHaveBody()?

I've not dug back in time to see how we got to that predicate in the first place, but my *hunch* is that this may have to do with implicit function definitions for things like special member functions. That's a case where you can have a function definition without a body. e.g.,

struct S {
  S();
};

S::S() = default;

[tbaeder]

clang/lib/AST/Interp/ByteCodeEmitter.cpp

23–27

Well, any sort of digging usually just gets you to the one commit that introduced all of the new constant interpreter :P

willHaveBody() isn't even relevant here, is it?
I think simply

bool hasBody = FuncDecl->isDefined() || FuncDecl->hasBody();

makes the most sense.

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeEmitter.cpp

23–27

I think it's one or the other?

isDefined searches for a function that's been defined by looking for something in the redecl chain that: is deleted as written, is defaulted, has a body, will have a body, has a body that's skipped, or has an attribute that causes the declaration to be a definition.

By contrast, hasBody searches the redecl chain looking for a declaration that has not been defaulted and has a body or is template late parsed.

So I *think* what you want is FuncDecl->hasBody(FuncDecl) to do the check and get the specific declaration with the body.

[tbaeder]

clang/lib/AST/Interp/ByteCodeEmitter.cpp

23–27

FuncDecl->hasBody(FuncDecl) sounds like it makes sense. None of the variations break any of the existing tests, so I will trust you. :) Thanks

clang/lib/AST/Interp/ByteCodeEmitter.cpp

 
 using namespace clang;
 using namespace clang::interp;
 
 using APSInt = llvm::APSInt;
 using Error = llvm::Error;
 
 Expected<Function *>
 ByteCodeEmitter::compileFunc(const FunctionDecl *FuncDecl) {
-  // Do not try to compile undefined functions.
-  if (!FuncDecl->isDefined(FuncDecl) ||
-      (!FuncDecl->hasBody() && FuncDecl->willHaveBody()))
-    return nullptr;
+  // Function is not defined at all or not yet. We will
+  // create a Function instance but not compile the body. That
+  // will (maybe) happen later.
+  bool HasBody = FuncDecl->hasBody(FuncDecl);
 
   // Set up argument indices.
   unsigned ParamOffset = 0;
   SmallVector<PrimType, 8> ParamTypes;
   llvm::DenseMap<unsigned, Function::ParamDescriptor> ParamDescriptors;
 
   // If the return is not a primitive, a pointer to the storage where the value
   // is initialized in is passed as the first argument.
for (const ParmVarDecl *PD : FuncDecl->parameters()) {
     Descriptor *Desc = P.createDescriptor(PD, Ty);
     ParamDescriptors.insert({ParamOffset, {Ty, Desc}});
     Params.insert({PD, ParamOffset});
     ParamOffset += align(primSize(Ty));
     ParamTypes.push_back(Ty);
   }
 
   // Create a handle over the emitted code.
-  Function *Func =
+  Function *Func = P.getFunction(FuncDecl);
+  if (!Func)
+    Func =
         P.createFunction(FuncDecl, ParamOffset, std::move(ParamTypes),
                          std::move(ParamDescriptors), HasThisPointer, HasRVO);
+  assert(Func);
+  if (!HasBody)
+    return Func;
+
   // Compile the function body.
   if (!FuncDecl->isConstexpr() || !visitFunc(FuncDecl)) {
     // Return a dummy function if compilation failed.
     if (BailLocation)
       return llvm::make_error<ByteCodeGenError>(*BailLocation);
     else {
       Func->setIsFullyCompiled(true);
       return Func;

clang/lib/AST/Interp/ByteCodeExprGen.cpp

 Record *ByteCodeExprGen<Emitter>::getRecord(const RecordDecl *RD) {
   return P.getOrCreateRecord(RD);
 }
 
 template <class Emitter>
 const Function *ByteCodeExprGen<Emitter>::getFunction(const FunctionDecl *FD) {
   assert(FD);
   const Function *Func = P.getFunction(FD);
+  bool IsBeingCompiled = Func && !Func->isFullyCompiled();
+  bool WasNotDefined = Func && !Func->hasBody();
 
-  if (!Func) {
+  if (IsBeingCompiled)
+    return Func;
+
+  if (!Func || WasNotDefined) {
     if (auto R = ByteCodeStmtGen<ByteCodeEmitter>(Ctx, P).compileFunc(FD))
       Func = *R;
     else {
       llvm::consumeError(R.takeError());
       return nullptr;
     }
   }
 

clang/lib/AST/Interp/Context.cpp

 
 Context::Context(ASTContext &Ctx) : Ctx(Ctx), P(new Program(*this)) {}
 
 Context::~Context() {}
 
 bool Context::isPotentialConstantExpr(State &Parent, const FunctionDecl *FD) {
   assert(Stk.empty());
   Function *Func = P->getFunction(FD);
-  if (!Func) {
+  if (!Func || !Func->hasBody()) {
     if (auto R = ByteCodeStmtGen<ByteCodeEmitter>(*this, *P).compileFunc(FD)) {
       Func = *R;
     } else {
       handleAllErrors(R.takeError(), [&Parent](ByteCodeGenError &Err) {
         Parent.FFDiag(Err.getRange().getBegin(),
                       diag::err_experimental_clang_interp_failed)
             << Err.getRange();
       });

clang/lib/AST/Interp/Function.h

public:
   /// Checks if the function is a constructor.
   bool isConstructor() const { return isa<CXXConstructorDecl>(F); }
 
   /// Checks if the function is fully done compiling.
   bool isFullyCompiled() const { return IsFullyCompiled; }
 
   bool hasThisPointer() const { return HasThisPointer; }
 
+  // Checks if the funtion already has a body attached.
+  bool hasBody() const { return HasBody; }
+
   unsigned getNumParams() const { return ParamTypes.size(); }
 
 private:
   /// Construct a function representing an actual function.
   Function(Program &P, const FunctionDecl *F, unsigned ArgSize,
            llvm::SmallVector<PrimType, 8> &&ParamTypes,
            llvm::DenseMap<unsigned, ParamDescriptor> &&Params,
            bool HasThisPointer, bool HasRVO);
 
   /// Sets the code of a function.
   void setCode(unsigned NewFrameSize, std::vector<char> &&NewCode, SourceMap &&NewSrcMap,
                llvm::SmallVector<Scope, 2> &&NewScopes) {
     FrameSize = NewFrameSize;
     Code = std::move(NewCode);
     SrcMap = std::move(NewSrcMap);
     Scopes = std::move(NewScopes);
     IsValid = true;
+    HasBody = true;
   }
 
   void setIsFullyCompiled(bool FC) { IsFullyCompiled = FC; }
 
 private:
   friend class Program;
   friend class ByteCodeEmitter;
 
private:
   bool IsFullyCompiled = false;
   /// Flag indicating if this function takes the this pointer
   /// as the first implicit argument
   bool HasThisPointer = false;
   /// Whether this function has Return Value Optimization, i.e.
   /// the return value is constructed in the caller's stack frame.
   /// This is done for functions that return non-primive values.
   bool HasRVO = false;
+  /// If we've already compiled the function's body.
+  bool HasBody = false;
 
 public:
   /// Dumps the disassembled bytecode to \c llvm::errs().
   void dump() const;
   void dump(llvm::raw_ostream &OS) const;
 };
 
 } // namespace interp
 } // namespace clang
 
 #endif

clang/lib/AST/Interp/Interp.h

 
 /// Checks if a method can be invoked on an object.
 bool CheckInvoke(InterpState &S, CodePtr OpPC, const Pointer &Ptr);
 
 /// Checks if a value can be initialized.
 bool CheckInit(InterpState &S, CodePtr OpPC, const Pointer &Ptr);
 
 /// Checks if a method can be called.
-bool CheckCallable(InterpState &S, CodePtr OpPC, Function *F);
+bool CheckCallable(InterpState &S, CodePtr OpPC, const Function *F);
 
 /// Checks the 'this' pointer.
 bool CheckThis(InterpState &S, CodePtr OpPC, const Pointer &This);
 
 /// Checks if a method is pure virtual.
 bool CheckPure(InterpState &S, CodePtr OpPC, const CXXMethodDecl *MD);
 
 /// Checks if Div/Rem operation on LHS and RHS is valid.

 }
 
 inline bool Call(InterpState &S, CodePtr &PC, const Function *Func) {
   auto NewFrame = std::make_unique<InterpFrame>(S, Func, PC);
   if (Func->hasThisPointer()) {
     if (!CheckInvoke(S, PC, NewFrame->getThis())) {
       return false;
     }
-    // TODO: CheckCallable
   }
 
+  if (!CheckCallable(S, PC, Func))
+    return false;
+
   InterpFrame *FrameBefore = S.Current;
   S.Current = NewFrame.get();
 
   APValue CallResult;
   // Note that we cannot assert(CallResult.hasValue()) here since
   // Ret() above only sets the APValue if the curent frame doesn't
   // have a caller set.
   if (Interpret(S, CallResult)) {

clang/lib/AST/Interp/Interp.cpp

 bool CheckInit(InterpState &S, CodePtr OpPC, const Pointer &Ptr) {
   if (!CheckLive(S, OpPC, Ptr, AK_Assign))
     return false;
   if (!CheckRange(S, OpPC, Ptr, AK_Assign))
     return false;
   return true;
 }
 
-bool CheckCallable(InterpState &S, CodePtr OpPC, Function *F) {
-  const SourceLocation &Loc = S.Current->getLocation(OpPC);
+bool CheckCallable(InterpState &S, CodePtr OpPC, const Function *F) {
 
   if (F->isVirtual()) {
     if (!S.getLangOpts().CPlusPlus20) {
+      const SourceLocation &Loc = S.Current->getLocation(OpPC);
       S.CCEDiag(Loc, diag::note_constexpr_virtual_call);
       return false;
     }
   }
 
   if (!F->isConstexpr()) {
+    const SourceLocation &Loc = S.Current->getLocation(OpPC);
     if (S.getLangOpts().CPlusPlus11) {
       const FunctionDecl *DiagDecl = F->getDecl();
 
       // If this function is not constexpr because it is an inherited
       // non-constexpr constructor, diagnose that directly.
       auto *CD = dyn_cast<CXXConstructorDecl>(DiagDecl);
       if (CD && CD->isInheritingConstructor()) {
         auto *Inherited = CD->getInheritedConstructor().getConstructor();

clang/lib/AST/Interp/Program.h

public:
   llvm::Optional<unsigned> createGlobal(const ValueDecl *VD, const Expr *E);
 
   /// Creates a global from a lifetime-extended temporary.
   llvm::Optional<unsigned> createGlobal(const Expr *E);
 
   /// Creates a new function from a code range.
   template <typename... Ts>
   Function *createFunction(const FunctionDecl *Def, Ts &&... Args) {
+    Def = Def->getCanonicalDecl();
     auto *Func = new Function(*this, Def, std::forward<Ts>(Args)...);
     Funcs.insert({Def, std::unique_ptr<Function>(Func)});
     return Func;
   }
   /// Creates an anonymous function.
   template <typename... Ts>
   Function *createFunction(Ts &&... Args) {
     auto *Func = new Function(*this, std::forward<Ts>(Args)...);

clang/lib/AST/Interp/Program.cpp

llvm::Optional<unsigned> Program::createGlobal(const DeclTy &D, QualType Ty,
   G->block()->invokeCtor();
 
   Globals.push_back(G);
 
   return I;
 }
 
 Function *Program::getFunction(const FunctionDecl *F) {
-  F = F->getDefinition();
+  F = F->getCanonicalDecl();
+  assert(F);
   auto It = Funcs.find(F);
   return It == Funcs.end() ? nullptr : It->second.get();
 }
 
 Record *Program::getOrCreateRecord(const RecordDecl *RD) {
   // Use the actual definition as a key.
   RD = RD->getDefinition();
   if (!RD)

clang/test/AST/Interp/functions.cpp

 // RUN: %clang_cc1 -fexperimental-new-constant-interpreter -verify %s
 // RUN: %clang_cc1 -verify=ref %s
 
-// expected-no-diagnostics
-// ref-no-diagnostics
-
 constexpr void doNothing() {}
 constexpr int gimme5() {
   doNothing();
   return 5;
 }
 static_assert(gimme5() == 5, "");
 
 

 
 template<int N = 5>
 constexpr decltype(N) getNum() {
   return N;
 }
 static_assert(getNum<-2>() == -2, "");
 static_assert(getNum<10>() == 10, "");
 static_assert(getNum() == 5, "");
+
+constexpr int f(); // expected-note {{declared here}} \
+                   // ref-note {{declared here}}
+static_assert(f() == 5, ""); // expected-error {{not an integral constant expression}} \
+                             // expected-note {{undefined function 'f'}} \
+                             // ref-error {{not an integral constant expression}} \
+                             // ref-note {{undefined function 'f'}}
+constexpr int a() {
+  return f();
+}
+constexpr int f() {
+  return 5;
+}
+static_assert(a() == 5, "");