fix: successive Declare fails if InstantiateFunctionDefinition fails in MakeFunctionCallable (#616)

* [MRE] successive Declare fails if InstantiateFunctionDefinition fails in MakeFunctionCallable

* fix: by forcing to compile an empty code block

* fix test to use `InstantiateTemplate` instead of `BestOverloadFunctionMatch`

* add `SynthesizingCodeRAII` that works with both clang & cling
add `InstantiateFunctionDefinition` function

Author: Vipul-Cariappa

include/CppInterOp/CppInterOp.h

@@ -743,11 +743,12 @@ struct TemplateArgInfo {
 ///           in the \c TemplateArgInfo struct
 ///\param[in] template_args_size - Size of the vector of template arguments
 ///           passed as \c template_args
+///\param[in] instantiate_body - also instantiate/define the body
 ///
 ///\returns Instantiated templated class/function/variable pointer
 CPPINTEROP_API TCppScope_t
 InstantiateTemplate(TCppScope_t tmpl, const TemplateArgInfo* template_args,
-                    size_t template_args_size);
+                    size_t template_args_size, bool instantiate_body = false);
 
 /// Sets the class template instantiation arguments of \c templ_instance.
 ///

lib/CppInterOp/CXCppInterOp.cpp

@@ -571,7 +571,8 @@ bool clang_existsFunctionTemplate(const char* name, CXScope parent) {
 namespace Cpp {
 TCppScope_t InstantiateTemplate(compat::Interpreter& I, TCppScope_t tmpl,
                                 const TemplateArgInfo* template_args,
-                                size_t template_args_size);
+                                size_t template_args_size,
+                                bool instantiate_body = false);
 } // namespace Cpp
 
 CXScope clang_instantiateTemplate(CXScope tmpl,

lib/CppInterOp/Compatibility.h

@@ -99,6 +99,11 @@ namespace compat {
 
 using Interpreter = cling::Interpreter;
 
+class SynthesizingCodeRAII : public Interpreter::PushTransactionRAII {
+public:
+  SynthesizingCodeRAII(Interpreter* i) : Interpreter::PushTransactionRAII(i) {}
+};
+
 inline void maybeMangleDeclName(const clang::GlobalDecl& GD,
                                 std::string& mangledName) {
   cling::utils::Analyze::maybeMangleDeclName(GD, mangledName);
@@ -376,6 +381,20 @@ namespace Cpp_utils = Cpp::utils;
 
 namespace compat {
 using Interpreter = Cpp::Interpreter;
+
+class SynthesizingCodeRAII {
+private:
+  Interpreter* m_Interpreter;
+
+public:
+  SynthesizingCodeRAII(Interpreter* i) : m_Interpreter(i) {}
+  ~SynthesizingCodeRAII() {
+    auto GeneratedPTU = m_Interpreter->Parse("");
+    if (!GeneratedPTU)
+      llvm::logAllUnhandledErrors(GeneratedPTU.takeError(), llvm::errs(),
+                                  "Failed to generate PTU:");
+  }
+};
 }
 
 #endif // CPPINTEROP_USE_REPL

lib/CppInterOp/CppInterOp.cpp

@@ -33,6 +33,7 @@
 #include "clang/Basic/Specifiers.h"
 #include "clang/Basic/Version.h"
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/Interpreter/Interpreter.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Overload.h"
 #include "clang/Sema/Ownership.h"
@@ -222,6 +223,15 @@ void EnableDebugOutput(bool value /* =true*/) { llvm::DebugFlag = value; }
 
 bool IsDebugOutputEnabled() { return llvm::DebugFlag; }
 
+static void InstantiateFunctionDefinition(Decl* D) {
+  compat::SynthesizingCodeRAII RAII(&getInterp());
+  if (auto* FD = llvm::dyn_cast_or_null<FunctionDecl>(D)) {
+    getSema().InstantiateFunctionDefinition(SourceLocation(), FD,
+                                            /*Recursive=*/true,
+                                            /*DefinitionRequired=*/true);
+  }
+}
+
 bool IsAggregate(TCppScope_t scope) {
   Decl* D = static_cast<Decl*>(scope);
 
@@ -923,11 +933,7 @@ TCppType_t GetFunctionReturnType(TCppFunction_t func) {
       }
 
       if (needInstantiation) {
-#ifdef CPPINTEROP_USE_CLING
-        cling::Interpreter::PushTransactionRAII RAII(&getInterp());
-#endif
-        getSema().InstantiateFunctionDefinition(SourceLocation(), FD, true,
-                                                true);
+        InstantiateFunctionDefinition(FD);
       }
       Type = FD->getReturnType();
     }
@@ -2491,14 +2497,8 @@ int get_wrapper_code(compat::Interpreter& I, const FunctionDecl* FD,
   }
   if (needInstantiation) {
     clang::FunctionDecl* FDmod = const_cast<clang::FunctionDecl*>(FD);
-    clang::Sema& S = I.getCI()->getSema();
-    // Could trigger deserialization of decls.
-#ifdef CPPINTEROP_USE_CLING
-    cling::Interpreter::PushTransactionRAII RAII(&I);
-#endif
-    S.InstantiateFunctionDefinition(SourceLocation(), FDmod,
-                                    /*Recursive=*/true,
-                                    /*DefinitionRequired=*/true);
+    InstantiateFunctionDefinition(FDmod);
+
     if (!FD->isDefined(Definition)) {
       llvm::errs() << "TClingCallFunc::make_wrapper"
                    << ":"
@@ -3303,7 +3303,8 @@ std::string ObjToString(const char* type, void* obj) {
 }
 
 static Decl* InstantiateTemplate(TemplateDecl* TemplateD,
-                                 TemplateArgumentListInfo& TLI, Sema& S) {
+                                 TemplateArgumentListInfo& TLI, Sema& S,
+                                 bool instantiate_body) {
   // This is not right but we don't have a lot of options to choose from as a
   // template instantiation requires a valid source location.
   SourceLocation fakeLoc = GetValidSLoc(S);
@@ -3317,6 +3318,8 @@ static Decl* InstantiateTemplate(TemplateDecl* TemplateD,
       // FIXME: Diagnose what happened.
       (void)Result;
     }
+    if (instantiate_body)
+      InstantiateFunctionDefinition(Specialization);
     return Specialization;
   }
 
@@ -3348,19 +3351,21 @@ static Decl* InstantiateTemplate(TemplateDecl* TemplateD,
 }
 
 Decl* InstantiateTemplate(TemplateDecl* TemplateD,
-                          ArrayRef<TemplateArgument> TemplateArgs, Sema& S) {
+                          ArrayRef<TemplateArgument> TemplateArgs, Sema& S,
+                          bool instantiate_body) {
   // Create a list of template arguments.
   TemplateArgumentListInfo TLI{};
   for (auto TA : TemplateArgs)
     TLI.addArgument(
         S.getTrivialTemplateArgumentLoc(TA, QualType(), SourceLocation()));
 
-  return InstantiateTemplate(TemplateD, TLI, S);
+  return InstantiateTemplate(TemplateD, TLI, S, instantiate_body);
 }
 
 TCppScope_t InstantiateTemplate(compat::Interpreter& I, TCppScope_t tmpl,
                                 const TemplateArgInfo* template_args,
-                                size_t template_args_size) {
+                                size_t template_args_size,
+                                bool instantiate_body) {
   auto& S = I.getSema();
   auto& C = S.getASTContext();
 
@@ -3385,14 +3390,15 @@ TCppScope_t InstantiateTemplate(compat::Interpreter& I, TCppScope_t tmpl,
 #ifdef CPPINTEROP_USE_CLING
   cling::Interpreter::PushTransactionRAII RAII(&I);
 #endif
-  return InstantiateTemplate(TmplD, TemplateArgs, S);
+  return InstantiateTemplate(TmplD, TemplateArgs, S, instantiate_body);
 }
 
 TCppScope_t InstantiateTemplate(TCppScope_t tmpl,
                                 const TemplateArgInfo* template_args,
-                                size_t template_args_size) {
+                                size_t template_args_size,
+                                bool instantiate_body) {
   return InstantiateTemplate(getInterp(), tmpl, template_args,
-                             template_args_size);
+                             template_args_size, instantiate_body);
 }
 
 void GetClassTemplateInstantiationArgs(TCppScope_t templ_instance,

unittests/CppInterOp/FunctionReflectionTest.cpp

@@ -905,7 +905,7 @@ TEST(FunctionReflectionTest, InstantiateVariadicFunction) {
 
   // instantiate VariadicFnExtended
   auto Instance2 =
-      Cpp::InstantiateTemplate(Decls[2], args2.data(), args2.size());
+      Cpp::InstantiateTemplate(Decls[2], args2.data(), args2.size(), true);
   EXPECT_TRUE(Cpp::IsTemplatedFunction(Instance2));
 
   FunctionDecl* FD2 = cast<FunctionDecl>((Decl*)Instance2);
@@ -2193,3 +2193,38 @@ TEST(FunctionReflectionTest, UndoTest) {
 #endif
 #endif
 }
+
+TEST(FunctionReflectionTest, FailingTest1) {
+#ifdef _WIN32
+  GTEST_SKIP() << "Disabled on Windows. Needs fixing.";
+#endif
+#ifdef EMSCRIPTEN
+  GTEST_SKIP() << "Test fails for Emscipten builds";
+#endif
+  Cpp::CreateInterpreter();
+  EXPECT_FALSE(Cpp::Declare(R"(
+    class WithOutEqualOp1 {};
+    class WithOutEqualOp2 {};
+
+    WithOutEqualOp1 o1;
+    WithOutEqualOp2 o2;
+
+    template<class C1, class C2>
+    bool is_equal(const C1& c1, const C2& c2) { return (bool)(c1 == c2); }
+  )"));
+
+  Cpp::TCppType_t o1 = Cpp::GetTypeFromScope(Cpp::GetNamed("o1"));
+  Cpp::TCppType_t o2 = Cpp::GetTypeFromScope(Cpp::GetNamed("o2"));
+  std::vector<Cpp::TCppFunction_t> fns;
+  Cpp::GetClassTemplatedMethods("is_equal", Cpp::GetGlobalScope(), fns);
+  EXPECT_EQ(fns.size(), 1);
+
+  Cpp::TemplateArgInfo args[2] = {{o1}, {o2}};
+  Cpp::TCppScope_t fn = Cpp::InstantiateTemplate(fns[0], args, 2);
+  EXPECT_TRUE(fn);
+
+  Cpp::JitCall jit_call = Cpp::MakeFunctionCallable(fn);
+  EXPECT_EQ(jit_call.getKind(), Cpp::JitCall::kUnknown); // expected to fail
+  EXPECT_FALSE(Cpp::Declare("int x = 1;"));
+  EXPECT_FALSE(Cpp::Declare("int y = x;"));
+}