Added undo command for CppInterOp #513

kr-2003 · 2025-03-03T11:44:10Z

Description

Added Undo command for Clang-REPL. Reverts the last N operations performed by the interpreter.

Type of change

New feature

Testing

Tests in unittests/CppInterOp/FunctionReflectionTest.cpp.
Demo - https://res.cloudinary.com/abhi9av/video/upload/v1740997292/wqgfbgqefrwpx2o7ko95.mp4

Checklist

I have read the contribution guide recently

anutosh491 · 2025-03-03T11:46:31Z

Hi,

@kr-2003 and I were interested in supporting %undo through xeus-cpp/xeus-cpp-lite but we realize it is not being exposed through cppinterop.

Hence I asked him to add the same here.

anutosh491 · 2025-03-03T11:49:32Z

Weird that we found it being exposed through the libclang inspired C API but not through the standard one.

CppInterOp/lib/Interpreter/CXCppInterOp.cpp

Lines 307 to 313 in 7caf10b

    
           enum CXErrorCode clang_Interpreter_undo(CXInterpreter I, unsigned int N) { 
        
           #ifdef CPPINTEROP_USE_CLING 
        
             return CXError_Failure; 
        
           #else 
        
             return getInterpreter(I)->Undo(N) ? CXError_Failure : CXError_Success; 
        
           #endif // CPPINTEROP_USE_CLING 
        
           }

codecov · 2025-03-03T11:57:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.46%. Comparing base (a22df96) to head (77489d7).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #513      +/-   ##
==========================================
+ Coverage   75.33%   75.46%   +0.13%     
==========================================
  Files           9        9              
  Lines        3620     3628       +8     
==========================================
+ Hits         2727     2738      +11     
+ Misses        893      890       -3

Files with missing lines	Coverage Δ
include/clang/Interpreter/CppInterOp.h	`96.29% <ø> (ø)`
lib/Interpreter/CXCppInterOp.cpp	`47.27% <ø> (ø)`
lib/Interpreter/CppInterOp.cpp	`83.07% <100.00%> (+0.01%)`	⬆️
lib/Interpreter/CppInterOpInterpreter.h	`80.32% <100.00%> (+1.23%)`	⬆️

... and 1 file with indirect coverage changes

Files with missing lines	Coverage Δ
include/clang/Interpreter/CppInterOp.h	`96.29% <ø> (ø)`
lib/Interpreter/CXCppInterOp.cpp	`47.27% <ø> (ø)`
lib/Interpreter/CppInterOp.cpp	`83.07% <100.00%> (+0.01%)`	⬆️
lib/Interpreter/CppInterOpInterpreter.h	`80.32% <100.00%> (+1.23%)`	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

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

github-actions · 2025-03-03T12:02:16Z

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

Vipul-Cariappa

@anutosh491 asked me to look at this.

LGTM! If you can just update the test with the changes I specified.

unittests/CppInterOp/FunctionReflectionTest.cpp

lib/Interpreter/CppInterOp.cpp

kr-2003 · 2025-03-04T07:00:12Z

@vgvassilev I have included both cling and clang-REPL for undo feature. All tests are passing successfully. Is there any other work pending?

anutosh491 · 2025-03-04T08:55:35Z

I was actually hopeful we could have this before/in 1.6.0 (which was released on the same day we started discussing undo in xeus-cpp and lite 😅 . We actually made good progress here as xeus-cpp would just naturally have it and I was able to get undo to quite some extent for lite too)

Can we have it through a patch release (1.6.1) or something ?

vgvassilev · 2025-03-04T17:00:21Z

I was actually hopeful we could have this before/in 1.6.0 (which was released on the same day we started discussing undo in xeus-cpp and lite 😅 . We actually made good progress here as xeus-cpp would just naturally have it and I was able to get undo to quite some extent for lite too)

Can we have it through a patch release (1.6.1) or something ?

We do not foresee a patch version for now. It will wait for the next shipping vehicle.

anutosh491 · 2025-03-04T17:17:56Z

It will wait for the next shipping vehicle.

No issues. Let's move it in if we consider it ready though ?

Also hopefully till the next release, I shall have enough context on how to get this working through lite. I was able to implement it (and if you go through the video carefully you will see the flaw in the video)

Screen.Recording.2025-02-28.at.12.02.54.PM.mp4

Have been discussing why this goes wrong with Sam Clegg who says symbols from subsequent dlopen() operations don't override existing global symbols from previous dlopen() calls. So yeah an error in emscripten is what we think this is !

mcbarton · 2025-03-04T19:06:08Z

lib/Interpreter/CXCppInterOp.cpp

 enum CXErrorCode clang_Interpreter_undo(CXInterpreter I, unsigned int N) {
 #ifdef CPPINTEROP_USE_CLING
-  return CXError_Failure;
+  auto* interp = getInterpreter(I);


If using clang_repl you can return CXError_Failure if it fails. With cling and the c api currently as it stands you cannot. Can you fix this @kr-2003

mcbarton · 2025-03-04T19:07:47Z

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 72.54%. Comparing base (7caf10b) to head (87114a5).

Files with missing lines Patch % Lines
lib/Interpreter/CppInterOpInterpreter.h 66.66% 2 Missing ⚠️
Additional details and impacted files

@kr-2003 can you update the tests so your tests cover the 2 missing lines from the patch?

kr-2003 · 2025-03-05T08:27:09Z

lib/Interpreter/CppInterOp.cpp

+#ifdef CPPINTEROP_USE_CLING
+    auto& I = getInterp();
+    cling::Interpreter::PushTransactionRAII RAII(&I);
+    I.unload(N);


@vgvassilev How do I even check if unload function logs any errors, since its a void function? Should I use try-catch block(but it is disabled in the project) or use system level redirection of the output to stream using pipes(this might be an overkill for capturing)? unload function logs cling::errs(), if it fails to run.

@vgvassilev Could you please guide me on how to proceed here?

You should assume that the operation was successful.

Hmmm, I think that's what is being done here https://github.com/compiler-research/CppInterOp/pull/513/files#diff-fb574ca9d2633ca33be71dfa5c0d9f437cfa98768f0e828d5eb789bfcfcef688R3624 so nothing to address here correct ?

anutosh491

I think this looks okay to me.

Maybe the two lines that are not being covered (see comment below) can be addressed.

#513 (comment)

This demands for a negation case (testing a case where undo fails)
Probably can be done through trying undo(1000) when only few operations have taken place. Let's try something relevant out and then move this in ?

kr-2003 · 2025-03-10T07:07:43Z

@mcbarton Test coverage review has been addressed. Can we take this in now?

aaronj0 · 2025-03-13T13:44:34Z

unittests/CppInterOp/FunctionReflectionTest.cpp

+  Cpp::Process("int x = 5;");
+  cerrs = testing::internal::GetCapturedStderr();
+  EXPECT_STREQ(cerrs.c_str(), "");
+  Cpp::Undo();


I believe unloading this first transaction here is problematic for destructors, and is unsupported by Cling, which can be seen in the error thrown in the logs. We should drop this call to Cpp::Undo()

@kr-2003 can you address this review comment?

i added two more instructions before calling the first Cpp::Undo().

mcbarton · 2025-03-14T17:40:22Z

@kr-2003 can you rebase? Now that we have automated wasm tests we need to check that the new test passes for an Emscripten build.

mcbarton · 2025-03-17T10:51:12Z

@anutosh491 Given you want this PR in can you help @kr-2003 fix the test which fails for the Emscripten case?

anutosh491 · 2025-03-17T10:57:53Z

Perfect. So undo isn't expected to work with clang-repl-lite untill emscripten supports dlclose correctly.

Abhinav only run these tests when emscripten is not defined

So

#ifndef EMSCRIPTEN
  place your tests here
#endif

or if you place this construct inside your test, you can do

TEST(FunctionReflectionTest, UndoTest) {
#ifdef EMSCRIPTEN
  GTEST_SKIP() << "Currently not implemented while running clang-repl in the broswer";
#endif

unittests/CppInterOp/FunctionReflectionTest.cpp

kr-2003 · 2025-03-19T05:37:36Z

@mcbarton
https://github.com/compiler-research/CppInterOp/actions/runs/13938614873/job/39011414822?pr=513#step:8:623
All tests are passing but after that the workflow is exiting with code -1 in windows. Can you help me out here?

mcbarton · 2025-03-19T08:21:39Z

@mcbarton https://github.com/compiler-research/CppInterOp/actions/runs/13938614873/job/39011414822?pr=513#step:8:623 All tests are passing but after that the workflow is exiting with code -1 in windows. Can you help me out here?

@kr-2003 I suspect (but won't know for sure until you skip the section of the test causing this message) that you are getting the error from here https://github.com/compiler-research/CppInterOp/actions/runs/13938614873/job/39011414822?pr=513#step:8:408 . Basically almost every test which causes the interpreter to write anything to the screen in Windows is currently skipped. Its a known issue, which we are hoping will get fixed soon. Whether this PR is acceptable with this test skipped on Windows will not be upto me though. You would need the approval of @vgvassilev or maybe @aaronj0 .

kr-2003 · 2025-03-19T10:43:23Z

@mcbarton https://github.com/compiler-research/CppInterOp/actions/runs/13938614873/job/39011414822?pr=513#step:8:623 All tests are passing but after that the workflow is exiting with code -1 in windows. Can you help me out here?

@kr-2003 I suspect (but won't know for sure until you skip the section of the test causing this message) that you are getting the error from here https://github.com/compiler-research/CppInterOp/actions/runs/13938614873/job/39011414822?pr=513#step:8:408 . Basically almost every test which causes the interpreter to write anything to the screen in Windows is currently skipped. Its a known issue, which we are hoping will get fixed soon. Whether this PR is acceptable with this test skipped on Windows will not be upto me though. You would need the approval of @vgvassilev or maybe @aaronj0 .

So, should I skip these tests for windows? @vgvassilev @aaronj0

kr-2003 · 2025-03-22T04:49:23Z

For now I have skipped these tests for windows.

vgvassilev

Lgtm!

vgvassilev · 2025-03-24T12:42:23Z

Make sure the ci passes and you squash on merge.

anutosh491 · 2025-03-24T12:42:55Z

Make sure the ci passes and you squash on merge.

Absolutely
Yeah 15 commits is a bit too much.

Vipul-Cariappa · 2025-03-25T04:23:56Z

@kr-2003 Congratulations on getting your first PR merged 🚀.

kr-2003 · 2025-03-25T04:49:22Z

@kr-2003 Congratulations on getting your first PR merged 🚀.

Thank you 😊

Vipul-Cariappa approved these changes Mar 3, 2025

View reviewed changes

unittests/CppInterOp/FunctionReflectionTest.cpp Outdated Show resolved Hide resolved

mcbarton reviewed Mar 3, 2025

View reviewed changes

lib/Interpreter/CppInterOp.cpp Show resolved Hide resolved

Vipul-Cariappa requested a review from vgvassilev March 3, 2025 12:10

anutosh491 reviewed Mar 3, 2025

View reviewed changes

lib/Interpreter/CppInterOp.cpp Outdated Show resolved Hide resolved

kr-2003 changed the title ~~Added undo command for Clang-REPL~~ Added undo command for CppInterOp Mar 3, 2025

anutosh491 requested a review from mcbarton March 4, 2025 10:28

mcbarton reviewed Mar 4, 2025

View reviewed changes

kr-2003 commented Mar 5, 2025

View reviewed changes

anutosh491 approved these changes Mar 7, 2025

View reviewed changes

anutosh491 force-pushed the undo-repl branch from 158b55e to c927755 Compare March 10, 2025 05:47

anutosh491 force-pushed the undo-repl branch from c927755 to 8c34a0c Compare March 11, 2025 06:52

aaronj0 reviewed Mar 13, 2025

View reviewed changes

mcbarton force-pushed the undo-repl branch from 8c34a0c to f2dd4c5 Compare March 14, 2025 18:02

mcbarton reviewed Mar 17, 2025

View reviewed changes

unittests/CppInterOp/FunctionReflectionTest.cpp Outdated Show resolved Hide resolved

anutosh491 force-pushed the undo-repl branch from 8a68993 to 7a6ce60 Compare March 18, 2025 05:52

vgvassilev reviewed Mar 18, 2025

View reviewed changes

unittests/CppInterOp/FunctionReflectionTest.cpp Outdated Show resolved Hide resolved

anutosh491 force-pushed the undo-repl branch from dae7773 to ab8adcd Compare March 19, 2025 04:03

kr-2003 and others added 15 commits March 24, 2025 18:06

added undo command for repl

cb99e62

added undo command for repl

bdbf3a5

added check for cling in CppInterOp.cpp

4e6ba5f

added undo test for n > 1

272039f

added unload feature for cling

24e0fe7

clang-format changes

ae95de9

fixed c-api for undo

4a777dc

test coverage

ec2f201

test coverage

0494249

test coverage

2dc971c

skipped undo test for emscripten build

80ceaa4

skipped undo test for emscripten build

c4ac735

using Cpp::Process ret val

f9581c9

removed testing undo on first transac

2ace298

removed test for windows

77489d7

anutosh491 force-pushed the undo-repl branch from f18bb03 to 77489d7 Compare March 24, 2025 12:36

vgvassilev approved these changes Mar 24, 2025

View reviewed changes

anutosh491 merged commit b9256d7 into compiler-research:main Mar 24, 2025
73 checks passed

kr-2003 mentioned this pull request Apr 15, 2025

Add error propagation to interpreter creation logic #553

Merged

4 tasks

Added undo command for CppInterOp #513

Added undo command for CppInterOp #513

Uh oh!

Conversation

kr-2003 commented Mar 3, 2025

Description

Type of change

Testing

Checklist

Uh oh!

anutosh491 commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anutosh491 commented Mar 3, 2025

Uh oh!

codecov bot commented Mar 3, 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 Mar 3, 2025

Uh oh!

Vipul-Cariappa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kr-2003 commented Mar 4, 2025

Uh oh!

anutosh491 commented Mar 4, 2025

Uh oh!

vgvassilev commented Mar 4, 2025

Uh oh!

anutosh491 commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcbarton Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

mcbarton commented Mar 4, 2025

Codecov Report

Uh oh!

kr-2003 Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

kr-2003 Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

vgvassilev Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

anutosh491 Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anutosh491 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kr-2003 commented Mar 10, 2025

Uh oh!

aaronj0 Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

mcbarton Mar 17, 2025

Choose a reason for hiding this comment

Uh oh!

kr-2003 Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

mcbarton commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcbarton commented Mar 17, 2025

Uh oh!

anutosh491 commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anutosh491 commented Mar 3, 2025 •

edited

Loading

codecov bot commented Mar 3, 2025 •

edited

Loading

anutosh491 commented Mar 4, 2025 •

edited

Loading

anutosh491 Mar 7, 2025 •

edited

Loading

anutosh491 left a comment •

edited

Loading

mcbarton commented Mar 14, 2025 •

edited

Loading

anutosh491 commented Mar 17, 2025 •

edited

Loading

kr-2003 commented Mar 19, 2025 •

edited

Loading