[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

Fri Feb 5 04:42:53 PST 2021

teemperor added a reviewer: sgraenitz.
teemperor added a comment.

Another round of comments.

Also a general note that applies to a bunch of places, if you specify the function names in comments when calling a function, you have to add a trailing `=` so that clang-tidy can actually check this for us <https://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html>::

  SomeFunction(/*ArgName*/ 0); // wrong
  SomeFunction(/*ArgName=*/ 0); // right

I think someone who knows more about the JIT should review the JIT-related code (I'm just gonna add Stefan too). And the changes related to the FrontendActions should also be signed off by someone else as I rarely touch that code. Not sure who though.

Beside that I think we're getting there, thanks!

================
Comment at: clang/include/clang/Frontend/FrontendAction.h:238
   /// objects, and run statistics and output file cleanup code.
-  void EndSourceFile();
+  virtual void EndSourceFile();

----------------
I think this and the change above requires updating `WrapperFrontendAction` to also forward these functions to the wrapped action. Otherwise wrapping actions would change their behaviour. See the other virtual functions there.

================
Comment at: clang/include/clang/Interpreter/Transaction.h:1
+//===--- Transaction.h - Incremental Compilation and Execution---*- C++ -*-===//
+//
----------------
v.g.vassilev wrote:
> teemperor wrote:
> > Could this whole file just be part of `IncrementalParser.h` which is the only user ? `clang::Transaction` seems anyway a bit of a generic name, so maybe this could become `clang::IncrementalParser::Transaction` then.
> The intent is to expose the Transaction class and if I move it in the `IncrementalParser` I will have to expose the `IncrementalParser.h`. Would it make sense to move it as part of the Interpreter object? Eg. `clang::Interpreter::Transaction`?
Makes sense. I guess moving this into the Interpreter would create some strange dependencies, so I would say we keep it in its own file. Maybe we could put it into a `interpreter` namespace or give it a more unique name `InterpreterTransaction`?

I also think this should have some documentation.

================
Comment at: clang/lib/Interpreter/IncrementalExecutor.h:36
+  llvm::Error addModule(std::unique_ptr<llvm::Module> M);
+  llvm::Error runCtors() const;
+};
----------------
Should we maybe merge `runCtors` and `addModule`? Not sure if there is a use case for adding a Module but not running Ctors. Also documentation.

================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:63
+    if (CI.hasCodeCompletionConsumer())
+      CompletionConsumer = &CI.getCodeCompletionConsumer();
+
----------------
Can this completion code even be used? It doesn't look like it can (and I'm not sure if using the `CodeCompletionAt` flag is really useful in a REPL as you can only specify it once during startup). IMHO this can be left out until we actually can hook up this into the LineEditor (and we have a way to test this).

================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:69
+    Preprocessor &PP = CI.getPreprocessor();
+    PP.enableIncrementalProcessing();
+    PP.EnterMainSourceFile();
----------------
Could we do that before creating the Sema? Sema's constructor is doing quite a few things and some of them might depend on this setting in the future.

================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:73
+
+  void EndSourceFile() override {
+    if (IsTerminating) {
----------------
I assume this function intercepts all EndSourceFile calls so that the preprocessor is 'stuck' in the main file even if it reaches the end of the user input so far? Might be worth a comment.

================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:74
+  void EndSourceFile() override {
+    if (IsTerminating) {
+      WrapperFrontendAction::EndSourceFile();
----------------
No `{ .. }` 

================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:101
+  // later errors use the default handling behavior instead.
+  llvm::remove_fatal_error_handler();
+}
----------------
I think this should be in ClangRepl.cpp where we actually install the handler (I wouldn't expect that creating an `IncrementalParser` object would touch the global error handler).

================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:136
+    Consumer->HandleTopLevelDecl(DGR);
+  }
+
----------------
Could we test this?

================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:165
+  std::ostringstream SourceName;
+  SourceName << "input_line_" << InputCount++;
+
----------------
I think this is OK for this patch, but if the file is called "input_line_" then Clang would still emit the actual line information (which is 1) and we get `input_line_1:1:9: error: ...` diagnostics. Since D83038 Clang can hide line numbers in diagnostics, so we could selectively turn this setting on when we generate diagnostics for `input_line` files.

(But as said, this is fine for this patch).

================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:189
+  // NewLoc only used for diags.
+  PP.EnterSourceFile(FID, /*DirLookup*/ 0, NewLoc);
+
----------------
I think we could assert that this succeeds.

================
Comment at: clang/lib/Interpreter/IncrementalParser.cpp:195
+
+  if (PP.getLangOpts().DelayedTemplateParsing) {
+    // Microsoft-specific:
----------------
Could we throw in a test that at least covers this code path?

================
Comment at: clang/lib/Interpreter/IncrementalParser.h:38
+  /// Long-lived, incremental parsing action.
+  std::unique_ptr<FrontendAction> Act;
+
----------------
Isn't this always an `IncrementalAction` ? The destructor is blindly casting this, so we might as well use the right type here.

================
Comment at: clang/lib/Interpreter/IncrementalParser.h:50
+  /// Incremental input counter.
+  unsigned InputCount = 0;
+
----------------
Maybe `Counts the number of direct user input lines that have been parsed.` or something like that. That InputCount counts inputs seems like redundant information.

================
Comment at: clang/lib/Interpreter/IncrementalParser.h:61
+  const CompilerInstance *getCI() const { return CI.get(); }
+  llvm::Expected<Transaction&> Parse(llvm::StringRef Input);
+
----------------
Documentation

================
Comment at: clang/lib/Interpreter/Interpreter.cpp:82
+  PCHOps->registerWriter(std::make_unique<ObjectFilePCHContainerWriter>());
+  PCHOps->registerReader(std::make_unique<ObjectFilePCHContainerReader>());
+
----------------
Can you add a FIXME that this should be removed (as I think we keep adding these two lines to all kind of Clang tools)? I assume this is only needed because of Clang errors when you encounter -gmodules produced pcms ?

================
Comment at: clang/lib/Interpreter/Interpreter.cpp:43
+#include "llvm/Support/Host.h"
+//
+
----------------
v.g.vassilev wrote:
> teemperor wrote:
> > I think the comment here/above and some of the empty lines aren't really needed here.
> Yeah, the include section surrounded with `//` is to keep track of the includes required by `IncrementalCompilerBuilder::create`. I am not sure where this code should live... 
> 
> Ideally, I'd like to have it in a place where it can be included by other clients which require compiler instance set up for incremental compilation.
I see. I don't really see a better place for this (I guess this depends on what these other clients actually need such a CompilerInstance for). IMHO just having this in the normal includes is fine for now.

FWIW, I think some of these includes can go:

VerifyDiagnosticConsumer.h -> not used
TextDiagnosticPrinter.h -> not used
ObjectFilePCHContainerOperations.h -> only needed for the mysterious code below
ErrorHandling.h -> not used

================
Comment at: clang/lib/Interpreter/Interpreter.cpp:140
+
+  if (std::find(ClangArgv.begin(), ClangArgv.end(), " -x") == ClangArgv.end()) {
+    // We do C++ by default; append right after argv[0] if no "-x" given
----------------
teemperor wrote:
> `llvm::find(ClangArgv, " -x")`
Sorry, I actually wanted to recommend `llvm::is_contained`, my bad!

================
Comment at: clang/tools/clang-repl/CMakeLists.txt:3
+#  ${LLVM_TARGETS_TO_BUILD}
+#  Option
+  Support
----------------
v.g.vassilev wrote:
> teemperor wrote:
> > Commented out by mistake?
> Does not seem to be required. I will remove it for now...
I meant that shouldn't have been commented out (as I think we don't get the targets symbols from any other library so this fails to link)

================
Comment at: clang/tools/clang-repl/ClangRepl.cpp:59
+    ClangArgs.push_back("-Xclang");
+    ClangArgs.push_back("-emit-llvm-only");
+  }
----------------
This is probably worth a comment why this is needed. Also because it seems important and only gets added when the flags are empty, this probably means that setting*any* Clang arg (`-std=XYZ`) would cause troubles? I think this could just be unconditionally added (as the last flag has the highest priority over whatever the user passed in before).

================
Comment at: clang/unittests/Interpreter/InterpreterTest.cpp:9
+//
+// Unit tests for our Interpreter library.
+//
----------------
`our` -> `Clang's` ?

================
Comment at: clang/unittests/Interpreter/InterpreterTest.cpp:16
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclGroup.h"
----------------
Could be merged with the includes above

================
Comment at: clang/unittests/Interpreter/InterpreterTest.cpp:69
+
+  EXPECT_DEATH((void)Interp->Parse("int var1 = 42;"), "");
+}
----------------
v.g.vassilev wrote:
> teemperor wrote:
> > I think that's usually used for testing asserts but here it's an invalid memory access (which might even work out just if the stars align correctly).
> > 
> > What about either:
> > 1. Shutting down clang-repl cleanly after we hit a diagnostic.
> > 2. Making an assert that we can't codegen a TU that already had any error diagnostic (in which case you can surround it with the following):
> > 
> > ```
> > #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
> > ```
> Would it make sense just to not test that case?
IIUC this would be fixed by just early-exiting from the parse logic when we see that there were previous parsing errors. That would also be easier to test and is better than crashing due to accessing free'd memory.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96033/new/

https://reviews.llvm.org/D96033