[MRE] successive Declare fails if InstantiateFunctionDefinition fails in MakeFunctionCallable #616

Vipul-Cariappa · 2025-06-06T08:11:10Z

This is a bug report.

If the instantiation of a templated function fails while CodeGen (MakeFunctionCallable), then successive call to Cpp::Declare fails.
But a second call to Cpp::Declare will pass.

I suspect something similar to

#ifdef CPPINTEROP_USE_CLING
    cling::Interpreter::PushTransactionRAII RAII(&I);
#endif

is required?

github-actions · 2025-06-06T08:19:52Z

clang-tidy review says "All clean, LGTM! 👍"

Vipul-Cariappa · 2025-06-06T08:32:19Z

Summary of conversion had over discord:
Declare does not print out any error.
The return code says it failed.
Stray Declare(" ") also fails.

github-actions · 2025-06-06T09:12:04Z

clang-tidy review says "All clean, LGTM! 👍"

codecov · 2025-06-06T09:23:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.18%. Comparing base (28abd17) to head (07e91ab).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #616      +/-   ##
==========================================
+ Coverage   78.00%   78.18%   +0.17%     
==========================================
  Files           9        9              
  Lines        3769     3781      +12     
==========================================
+ Hits         2940     2956      +16     
+ Misses        829      825       -4

Files with missing lines	Coverage Δ
include/CppInterOp/CppInterOp.h	`100.00% <ø> (ø)`
lib/CppInterOp/CXCppInterOp.cpp	`48.86% <ø> (ø)`
lib/CppInterOp/Compatibility.h	`92.37% <100.00%> (+0.40%)`	⬆️
lib/CppInterOp/CppInterOp.cpp	`85.85% <100.00%> (+0.23%)`	⬆️

Files with missing lines	Coverage Δ
include/CppInterOp/CppInterOp.h	`100.00% <ø> (ø)`
lib/CppInterOp/CXCppInterOp.cpp	`48.86% <ø> (ø)`
lib/CppInterOp/Compatibility.h	`92.37% <100.00%> (+0.40%)`	⬆️
lib/CppInterOp/CppInterOp.cpp	`85.85% <100.00%> (+0.23%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2025-06-06T09:25:17Z

clang-tidy review says "All clean, LGTM! 👍"

vgvassilev · 2025-06-06T12:53:56Z

lib/CppInterOp/CppInterOp.cpp

+          GeneratedPTU.takeError(), llvm::errs(),
+          "[MakeFunctionCallable -> InstantiateFunctionDefinition] Failed to "
+          "generate PTU:");
+#endif


Do we want to move this somewhere in InstantiateTemplate and replace S.InstantiateFunctionDefinition with it?

github-actions

clang-tidy made some suggestions

github-actions · 2025-06-10T07:56:23Z

unittests/CppInterOp/FunctionReflectionTest.cpp

+  Cpp::GetClassTemplatedMethods("is_equal", Cpp::GetGlobalScope(), fns);
+  EXPECT_EQ(fns.size(), 1);
+
+  Cpp::TemplateArgInfo args[2] = {{o1}, {o2}};


warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]

Cpp::TemplateArgInfo args[2] = {{o1}, {o2}}; ^

github-actions · 2025-06-10T07:56:23Z

unittests/CppInterOp/FunctionReflectionTest.cpp

+  EXPECT_EQ(fns.size(), 1);
+
+  Cpp::TemplateArgInfo args[2] = {{o1}, {o2}};
+  Cpp::TCppScope_t fn = Cpp::InstantiateTemplate(fns[0], args, 2);


warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]

Cpp::TCppScope_t fn = Cpp::InstantiateTemplate(fns[0], args, 2); ^

github-actions

clang-tidy made some suggestions

github-actions · 2025-06-13T08:42:25Z

lib/CppInterOp/Compatibility.h

@@ -376,6 +380,20 @@ namespace Cpp_utils = Cpp::utils;

 namespace compat {
 using Interpreter = Cpp::Interpreter;
+
+class PushTransactionRAII {


warning: class 'PushTransactionRAII' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

class PushTransactionRAII { ^

The name should be something like SynthesizingCodeRAII or similar that tells the reader that we are producing ast nodes and require the codegen and the jit to know about it...

github-actions · 2025-06-13T08:42:25Z

lib/CppInterOp/CppInterOp.cpp

+    getSema().InstantiateFunctionDefinition(SourceLocation(), FD,
+                                            /*Recursive=*/true,
+                                            /*DefinitionRequired=*/true);
+    (void)Cpp::Interpreter::CompilationResult();


warning: no header providing "Cpp::Interpreter" is directly included [misc-include-cleaner]

lib/CppInterOp/CppInterOp.cpp:12:

+ #include "CppInterOpInterpreter.h"

… in MakeFunctionCallable

…nMatch`

add `InstantiateFunctionDefinition` function

github-actions

clang-tidy made some suggestions

github-actions · 2025-06-16T13:23:23Z

lib/CppInterOp/Compatibility.h

@@ -376,6 +381,20 @@ namespace Cpp_utils = Cpp::utils;

 namespace compat {
 using Interpreter = Cpp::Interpreter;
+
+class SynthesizingCodeRAII {


warning: class 'SynthesizingCodeRAII' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

class SynthesizingCodeRAII { ^

vgvassilev

Should we replace more occurrences of PushTransactionRAII? LGTM!

Vipul-Cariappa · 2025-06-16T15:19:10Z

I have not replaced it in other places because clang does the necessary cleanup, and we need not explicitly compile code for the cleanup to take place. If I replace it in all the occurrences, it would hit performance.

Vipul-Cariappa requested review from vgvassilev and aaronj0 June 6, 2025 08:11

Vipul-Cariappa marked this pull request as draft June 6, 2025 08:20

Vipul-Cariappa changed the title ~~[DO NOT MERGE] [MRE] successive Declare fails if InstantiateFunctionDefinition fails in MakeFunctionCallable~~ [MRE] successive Declare fails if InstantiateFunctionDefinition fails in MakeFunctionCallable Jun 6, 2025

Vipul-Cariappa force-pushed the bug/InstantiateFunctionDefinition branch from c2130c8 to 58d155a Compare June 6, 2025 09:11

vgvassilev reviewed Jun 6, 2025

View reviewed changes

Vipul-Cariappa marked this pull request as ready for review June 10, 2025 07:07

Vipul-Cariappa force-pushed the bug/InstantiateFunctionDefinition branch from 58d155a to 65f2092 Compare June 10, 2025 07:42

github-actions bot reviewed Jun 10, 2025

View reviewed changes

Vipul-Cariappa force-pushed the bug/InstantiateFunctionDefinition branch 2 times, most recently from 04cdcd5 to 35b6780 Compare June 13, 2025 08:32

github-actions bot reviewed Jun 13, 2025

View reviewed changes

Vipul-Cariappa force-pushed the bug/InstantiateFunctionDefinition branch 3 times, most recently from 16ae2e9 to 9f0c48a Compare June 13, 2025 14:31

Vipul-Cariappa added 3 commits June 16, 2025 10:11

[MRE] successive Declare fails if InstantiateFunctionDefinition fails…

7c30584

… in MakeFunctionCallable

fix: by forcing to compile empty code block

8f90c79

fix test to use InstantiateTemplate instead of `BestOverloadFunctio…

d85485c

…nMatch`

Vipul-Cariappa force-pushed the bug/InstantiateFunctionDefinition branch from 9f0c48a to 688fb53 Compare June 16, 2025 08:11

add PushTransactionRAII that works with both clang & cling

07e91ab

add `InstantiateFunctionDefinition` function

Vipul-Cariappa force-pushed the bug/InstantiateFunctionDefinition branch from 688fb53 to 07e91ab Compare June 16, 2025 13:01

github-actions bot reviewed Jun 16, 2025

View reviewed changes

vgvassilev approved these changes Jun 16, 2025

View reviewed changes

Vipul-Cariappa merged commit 49e6f4c into compiler-research:main Jun 16, 2025
50 checks passed

Vipul-Cariappa deleted the bug/InstantiateFunctionDefinition branch June 16, 2025 15:20

[MRE] successive Declare fails if InstantiateFunctionDefinition fails in MakeFunctionCallable #616

[MRE] successive Declare fails if InstantiateFunctionDefinition fails in MakeFunctionCallable #616

Uh oh!

Conversation

Vipul-Cariappa commented Jun 6, 2025

Uh oh!

github-actions bot commented Jun 6, 2025

Uh oh!

Vipul-Cariappa commented Jun 6, 2025

Uh oh!

github-actions bot commented Jun 6, 2025

Uh oh!

codecov bot commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Jun 6, 2025

Uh oh!

vgvassilev Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

vgvassilev Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

vgvassilev left a comment

Choose a reason for hiding this comment

Uh oh!

Vipul-Cariappa commented Jun 16, 2025

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jun 6, 2025 •

edited

Loading