This is an archive of the discontinued LLVM Phabricator instance.

[clang-repl] Implement code undo
ClosedPublic

Authored by junaire on May 30 2022, 7:40 PM.

Details

Summary

In interactive C++ it is convenient to roll back to a previous state of the
compiler. For example:
clang-repl> int x = 42;
clang-repl> %undo
clang-repl> float x = 24 // not an error

To support this, the patch extends the functionality used to recover from
errors and adds functionality to recover the low-level execution infrastructure.

The current implementation is based on watermarks. It exploits the fact that
at each incremental input the underlying compiler infrastructure is in a valid
state. We can only go N incremental inputs back to a previous valid state. We do
not need and do not do any further dependency tracking.

This patch was co-developed with V. Vassilev, relies on the past work of Purva
Chaudhari in clang-repl and is inspired by the past work on the same feature
in the Cling interpreter.

Co-authored-by: Purva-Chaudhari <purva.chaudhari02@gmail.com>
Co-authored-by: Vassil Vassilev <v.g.vassilev@gmail.com>
Signed-off-by: Jun Zhang <jun@junz.org>

Diff Detail

Event Timeline

junaire created this revision.May 30 2022, 7:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2022, 7:40 PM
junaire requested review of this revision.May 30 2022, 7:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2022, 7:40 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
junaire updated this revision to Diff 433008.May 30 2022, 7:51 PM

Also remove PTU in PTU list

junaire updated this revision to Diff 433009.May 30 2022, 7:56 PM

Improve code a little bit

junaire updated this revision to Diff 433011.May 30 2022, 8:12 PM

Remove a NFC change to make review easier

junaire updated this revision to Diff 433029.May 31 2022, 12:33 AM
  • Support undo N times
  • Add a comment
junaire updated this revision to Diff 433030.May 31 2022, 12:38 AM

Pass PTU by reference as it can't be NULL, this makes our code safer.

junaire updated this revision to Diff 433031.May 31 2022, 12:42 AM

Fix the build

junaire updated this revision to Diff 433038.May 31 2022, 1:36 AM
  • Add prefix for meta commands
  • Add more tests
junaire retitled this revision from [WIP][Interpreter] Implement undo command to [Interpreter][ClangRepl] Implement undo command.May 31 2022, 1:40 AM

I would prefer to merge them into one.

junaire updated this revision to Diff 435779.Jun 9 2022, 8:20 PM
junaire added a subscriber: Purva-Chaudhari.

Merge D126684 into this patch.

Consider D126684 is almost @Purva-Chaudhari 's work, I'll list her as
co-author when I commit this.

junaire updated this revision to Diff 435781.Jun 9 2022, 8:29 PM

Restore PTU in Interpreter::Undo, this should fix the broken tests.

junaire updated this revision to Diff 435782.Jun 9 2022, 8:35 PM

Allow undo command failed in the edge cases like undoed too many times.

v.g.vassilev added inline comments.Jun 9 2022, 9:48 PM
clang/lib/Interpreter/IncrementalParser.cpp
180

We should call Restore here.

clang/unittests/Interpreter/InterpreterTest.cpp
251 ↗(On Diff #435782)

Can we move this as a regular test called code-undo.cpp?

junaire updated this revision to Diff 435826.Jun 10 2022, 1:16 AM
  • split the tests
  • add % to all meta commands
junaire added inline comments.Jun 10 2022, 1:54 AM
clang/lib/Interpreter/IncrementalParser.cpp
180

Though there are many logic duplicates with Restore, I'm not sure if we can refactor it. We have done more here than Restore, and Restore need to pass a PTU, but we only have TranslationDecl here.

v.g.vassilev added inline comments.Jun 12 2022, 4:43 AM
clang/lib/Interpreter/IncrementalParser.cpp
180

I think we can/should be able to construct the PartialTranslationUnit with only a TranslationUnitDecl having TheModule as nullptr.

junaire updated this revision to Diff 436214.Jun 12 2022, 6:40 AM

Address Vassil's comments

junaire edited the summary of this revision. (Show Details)Jun 12 2022, 6:44 AM
junaire added a reviewer: rsmith.
junaire updated this revision to Diff 436219.Jun 12 2022, 7:03 AM

Fix the unintentional format by clang-format

junaire updated this revision to Diff 436229.Jun 12 2022, 8:55 AM

Fix the build

I think we are in a good shape to broaden the list of reviewers.

junaire updated this revision to Diff 438892.Jun 21 2022, 7:27 PM
  • Add unittest
  • Fix a crash when previous input failed to parse

Thanks for working on this. Here are some more comments.

clang/include/clang/Interpreter/Interpreter.h
72

I am not sure if that's the best function. We

72

Please ignore this comment it was superseded by another one and I forgot to delete it.

clang/lib/Interpreter/IncrementalParser.cpp
279

I think we can merge this function with Undo where we can conditionally check if we have a llvm::Module to recover.

clang/lib/Interpreter/Interpreter.cpp
270

I am not sure how feasible is this usecase but in -fsyntax-only mode we can still need undo. That means somebody created an Interpreter instance for frontend-only operations, such as lookups or template instantiations. In that case I think it also would make sense to support the undo operation.

275

In some cases we can have PTU's that do not generate code. For example, define A 1 will not produce a corresponding llvm::Module. I think here we should count the size of the PartialTranslationUnitDecls.

clang/test/Interpreter/code-undo.cpp
7

Can you add here something like int j = 0 and print its value after the undo. This way we will make sure we do not undo too much.

junaire updated this revision to Diff 440027.Jun 25 2022, 9:13 PM

Address @v.g.vassilev 's comments.

junaire marked 3 inline comments as done.Jun 25 2022, 9:15 PM
junaire added inline comments.
clang/lib/Interpreter/IncrementalParser.cpp
279

I'm not sure I understand you correctly, do you mean to merge Interpreter::Restore with Interpreter::Undo? I think IncrementalParser::Restore is used above?

junaire retitled this revision from [Interpreter][ClangRepl] Implement undo command to [Interpreter][clang-repl] Implement undo command.Jun 25 2022, 11:43 PM
junaire updated this revision to Diff 440036.Jun 26 2022, 12:05 AM
  • Rename IncrementalParser::Restore to IncrementalParser::CleanupPTU
  • Drop unused code
junaire retitled this revision from [Interpreter][clang-repl] Implement undo command to [clang-repl] Implement undo command.Jun 26 2022, 12:06 AM
junaire updated this revision to Diff 440038.Jun 26 2022, 12:15 AM
  • Update a comment
  • Add a FIXME back
junaire updated this revision to Diff 440042.Jun 26 2022, 12:30 AM

Update commit message

junaire edited the summary of this revision. (Show Details)Jun 26 2022, 12:31 AM
junaire retitled this revision from [clang-repl] Implement undo command to [clang-repl] Implement code undo.
junaire updated this revision to Diff 440043.Jun 26 2022, 12:39 AM
junaire edited the summary of this revision. (Show Details)

Rename CleanupPTU => CleanUpPTU

junaire updated this revision to Diff 440050.Jun 26 2022, 2:47 AM

Try to fix the failing permerge test.

This revision is now accepted and ready to land.Jun 26 2022, 3:29 AM