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

Tue Feb 16 12:27:43 PST 2021

v.g.vassilev added a comment.

Hi Stefan,

Thanks a lot for the details you shared. They are really helpful to me.

In D96033#2565708 <https://reviews.llvm.org/D96033#2565708>, @sgraenitz wrote:

> Hi Vassil, thanks for upstreaming this! I think it goes into a good direction.
>
> The last time I looked at the Cling sources downstream, it was based on LLVM release 5.0. The IncrementalJIT class was based on what we call OrcV1 today. OrcV1 is long deprecated and even though it's still in tree today, it will very likely be removed in the upcoming release cycle. So I guess, one of the challenges will be porting the Cling internals to OrcV2 -- a lot has changed, mostly to the better :) Not all of this is relevant for this patch, but maybe it's worth mentioning for upcoming additions.

Cling is currently being upgraded to llvm9 (https://github.com/vgvassilev/cling/tree/upgrade_llvm90). I expect by the end of this year to be upgraded again to llvm12 and the big challenge is making good use of OrcV2. We need to support code removal of the kind:

  [cling] struct Adder { double Add(double a, double b) {return a - b; }; // comes in a "script" file.
  [cling] Adder adder; printf("%f\n", adder.Add(1, 2)); //realize that we have a mistake.
  [cling] .undo 2
  [cling] struct Adder { double Add(double a, double b) {return a + b; }; // comes in a "script" file.
  [cling] Adder adder; printf("%f\n", adder.Add(1, 2));

Some more details can be seen here -- https://blog.llvm.org/posts/2020-11-30-interactive-cpp-with-cling/

In the example above the JIT will need to remove objects from its state (including already created machine code). Until recently that was not entirely possible with the OrcV2. Lang was actively working on this and maybe this now works well.

The other aspect of this is that upon unloading of these pieces of code we need to run the destructors (that's why we need some non-canonical handling of when we run the `atexit` handlers).

I'd very much want to jump on the OrcV2 with this patch but that would cause me a longer term problem as now the initial version of minimal cling (clang-repl) is already substantially different than cling itself. My upstream strategy was to make a minimal patch with design as close as possible to cling. Then depending on the comments we will start evolving both systems in such a way that cling is not left substantially behind as it still has a majority of interactive C++ cases which we care about.

> OrcV2 works with Dylibs, basically symbol namespaces. When you add a module to a Dylib, all its symbols will be added. Respectively, if you want to remove something from a Dylib, you have to remove the symbols (for fine-tuning it you can reach for a Dylib's ResourceTracker). Symbols won't be materialized until you look them up. I guess for incremental compilation you would keep on adding symbols, one increment at a time.

All this sounds super nice and I am eager to start gradually using it.

>   int var1 = 42; int f() { return var1; }
>   int var2 = f();
>
> Let's say these are two inputs. The first line only adds definitions for symbols `var1` and `f()` but won't materialize anything. The second line would have to lookup `f()`, execute it and emit a new definition for `var2`. I never got into Cling deep enough to find out how it works, but I assume it's high-level enough that it won't require large changes. One thing I'd recommend to double-check: if there is a third line that adds a static constructor, will LLJIT only run this one or will it run all previous static ctors again when calling `initialize()`? I assume the former but I wouldn't bet on it.

Would that capture your concern?

  ./bin/clang-repl 
  clang-repl> extern "C" int printf(const char*,...);
  clang-repl> int var1 = 42; int f() { return printf("init_once\n"); }
  clang-repl> int var2 = f();
  init_once
  clang-repl> int var3 = f();
  init_once
  clang-repl> struct S{ S(const char*) {} } s("");
  clang-repl> 

I think the reason we do not rerun the static constructors is this tweak we have in codegen https://github.com/llvm/llvm-project/commit/188ad3ac02d06

> Another aspect is that downstream Cling is based on RuntimeDyld for linking Orc's output object files. I remember RemovableObjectLinkingLayer adding some object file removal code. Upstream OrcV2 grew it's own linker in the meantime. It's called JITLink and gets pulled into LLJIT via `ObjectLinkingLayer`. RuntimeDyld-based linking is still supported with the `RTDyldObjectLinkingLayer`. JITLink is not complete for all platforms yet. Thus, LLJITBuilder defaults to JITLink on macOS and RuntimeDyld otherwise. Chances are that JITLink gets good enough for ELF to enable it by default on Linux (at least x86-64). I guess that's your platform of concern?

We also care about COFF.

> The related question is whether you are aiming for JITLink straight away or staying with RuntimeDyld for now.

I'd prefer to stay closer to cling at least with that initial patch. That'd mean stick to the RuntimeDyld and switch when cling is ready (presumably end of this year).

================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:908

+CodeGenerator *CodeGenAction::getCodeGenerator() const {
+  return BEConsumer->getCodeGenerator();
----------------
sgraenitz wrote:
> v.g.vassilev wrote:
> > @rjmccall, we were wondering if there is a better way to ask CodeGen to start a new module. The current approach seems to be drilling hole in a number of abstraction layers.
> > 
> > In the past we have touched that area a little in https://reviews.llvm.org/D34444 and the answer may be already there but I fail to connect the dots.
> > 
> > Recently, we thought about having a new FrontendAction callback for beginning a new phase when compiling incremental input. We need to keep track of the created objects (needed for error recovery) in our Transaction. We can have a map of `Transaction*` to `llvm::Module*` in CodeGen. The issue is that new JITs take ownership of the `llvm::Module*` which seems to make it impossible to support jitted code removal with that model (cc: @lhames, @rsmith).
> When compiling incrementally, doeas a "new phase" mean that all subsequent code will go into a new module from then on? How will dependencies to previous symbols be handled? Are all symbols external?
> 
> > The issue is that new JITs take ownership of the llvm::Module*
> 
> That's true, but you can still keep a raw pointer to it, which will be valid at least as long as the module wasn't linked. Afterwards it depends on the linker:
> * RuntimeDyld can return ownership of the object's memory range via `NotifyEmittedFunction`
> * JITLink provides the `ReturnObjectBufferFunction` for the same purpose
> 
> > seems to make it impossible to support jitted code removal with that model
> 
> Can you figure out what symbols are affected and remove these? A la: https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/include/llvm/ExecutionEngine/Orc/Core.h#L1020
> 
> I think @anarazel has ported a client with code removal to OrcV2 successfully in the past. Maybe there's something we can learn from it.
> When compiling incrementally, doeas a "new phase" mean that all subsequent code will go into a new module from then on? How will dependencies to previous symbols be handled? Are all symbols external?

There is some discussion on this here https://reviews.llvm.org/D34444#812418

I think the relevant bit is that 'we have just one ever growing TU [...] which we send to the RuntimeDyLD allowing only JIT to resolve symbols from it.  We aid the JIT when resolving symbols with internal linkage by changing all internal linkage to external (We haven't seen issues with that approach)'.

> 
> > The issue is that new JITs take ownership of the llvm::Module*
> 
> That's true, but you can still keep a raw pointer to it, which will be valid at least as long as the module wasn't linked. 

That was my first implementation when I upgraded cling to llvm9 where the `shared_ptr`s went to `unique_ptr`s. This was quite problematic for many of the use cases we support as the JIT is somewhat unpredictable to the high-level API user. 

>Afterwards it depends on the linker:
> * RuntimeDyld can return ownership of the object's memory range via `NotifyEmittedFunction`
> * JITLink provides the `ReturnObjectBufferFunction` for the same purpose
> 

That's exactly what we ended up doing (I would like to thank Lang here who gave a similar advice).

> > seems to make it impossible to support jitted code removal with that model
> 
> Can you figure out what symbols are affected and remove these? A la: https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/include/llvm/ExecutionEngine/Orc/Core.h#L1020
> 
> I think @anarazel has ported a client with code removal to OrcV2 successfully in the past. Maybe there's something we can learn from it.

Indeed. That's not yet on my radar as seemed somewhat distant in time.

================
Comment at: clang/lib/Interpreter/IncrementalExecutor.h:36
+  llvm::Error addModule(std::unique_ptr<llvm::Module> M);
+  llvm::Error runCtors() const;
+};
----------------
sgraenitz wrote:
> v.g.vassilev wrote:
> > teemperor wrote:
> > > 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.
> > The case we have is when there is no JIT -- currently we have such a case in IncrementalProcessingTest I think. Another example, which will show up in future patches, is the registration of atexit handlers. That is, before we `runCtors` we make a pass over the LLVM IR and collect some specific details and (possibly change the IR and then run).
> > 
> > I'd rather keep it separate for now if that's okay.
> > Should we maybe merge runCtors and addModule?
> 
> +1 even though there may be open questions regarding incremental initialization.
> 
> > The case we have is when there is no JIT -- currently we have such a case in IncrementalProcessingTest
> 
> Can you run anything if there is no JIT? I think what you have in `IncrementalProcessing.EmitCXXGlobalInitFunc` is `getGlobalInit(llvm::Module*)`, which checks for symbol names with a specific prefix.
> 
> > before we runCtors we make a pass over the LLVM IR and collect some specific details and (possibly change the IR and then run).
> 
> The idiomatic solution for such modifications would use an IRTransformLayer as in:
> https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/examples/OrcV2Examples/LLJITWithOptimizingIRTransform/LLJITWithOptimizingIRTransform.cpp#L108
> 
> > Another example, which will show up in future patches, is the registration of atexit handlers
> 
> `atexit` handlers as well as global ctors/dtors should be covered by LLJIT PlatformSupport. The LLJITBuilder will inject a GenericLLVMIRPlatformSupport instance by default:
> https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp#L125
> 
> It's not as comprehensive as e.g. the MachO implementation, but should be sufficient for your use-case as you have IR for all your JITed code. (It would NOT work if you cached object files, reloaded them in a subsequent session and wanted to run their ctors.) So, your below call to `initialize()` should do it already.
> 
> > Should we maybe merge runCtors and addModule?
> 
> +1 even though there may be open questions regarding incremental initialization.
> 
> > The case we have is when there is no JIT -- currently we have such a case in IncrementalProcessingTest
> 
> Can you run anything if there is no JIT? I think what you have in `IncrementalProcessing.EmitCXXGlobalInitFunc` is `getGlobalInit(llvm::Module*)`, which checks for symbol names with a specific prefix.

Yes, I'd think such mode is useful for testing but also for other cases where the user is handed a Transaction* and allowed to make some modification before processing the `llvm::Module`

> 
> > before we runCtors we make a pass over the LLVM IR and collect some specific details and (possibly change the IR and then run).
> 
> The idiomatic solution for such modifications would use an IRTransformLayer as in:
> https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/examples/OrcV2Examples/LLJITWithOptimizingIRTransform/LLJITWithOptimizingIRTransform.cpp#L108

That looks very nice. It assumes the JIT is open to the users, here we open only the `llvm::Module` (I am not arguing if that's a good idea in general).

> 
> > Another example, which will show up in future patches, is the registration of atexit handlers
> 
> `atexit` handlers as well as global ctors/dtors should be covered by LLJIT PlatformSupport. The LLJITBuilder will inject a GenericLLVMIRPlatformSupport instance by default:
> https://github.com/llvm/llvm-project/blob/13f4448ae7db1a47/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp#L125

Does that give me control over when the `atexit` handlers are called? Can the interpreter call them at its choice?

> 
> It's not as comprehensive as e.g. the MachO implementation, but should be sufficient for your use-case as you have IR for all your JITed code. (It would NOT work if you cached object files, reloaded them in a subsequent session and wanted to run their ctors.) So, your below call to `initialize()` should do it already.
> 

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

https://reviews.llvm.org/D96033