[clang-repl] Keep the first llvm::Module empty to avoid invalid memory access.

[vgvassilev]

Clang's CodeGen is designed to work with a single llvm::Module. In many cases
for convenience various CodeGen parts have a reference to the llvm::Module
(TheModule or Module) which does not change when a new module is pushed.
However, the execution engine wants to take ownership of the module which does
not map well to CodeGen's design. To work this around we clone the module and
pass it down.

With some effort it is possible to teach CodeGen to ask the CodeGenModule for
its current module and that would have an overall positive impact on CodeGen
improving the encapsulation of various parts but that's not resilient to future
regression.

This patch takes a more conservative approach and keeps the first llvm::Module
empty intentionally and does not pass it to the Jit. That's also not bullet
proof because we have to guarantee that CodeGen does not write on the
blueprint. However, we have inserted some assertions to catch accidental
additions to that canary module.

This change will fixes a long-standing invalid memory access reported by
valgrind when we enable the TBAA optimization passes. It also unblock progress
on #84758.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Vassil Vassilev (vgvassilev)

Changes

Clang's CodeGen is designed to work with a single llvm::Module. In many cases for convenience various CodeGen parts have a reference to the llvm::Module (TheModule or Module) which does not change when a new module is pushed. However, the execution engine wants to take ownership of the module which does not map well to CodeGen's design. To work this around we clone the module and pass it down.

With some effort it is possible to teach CodeGen to ask the CodeGenModule for its current module and that would have an overall positive impact on CodeGen improving the encapsulation of various parts but that's not resilient to future regression.

This patch takes a more conservative approach and clones the llvm::Module before passing it to the Jit. That's also not bullet proof because we have to guarantee that CodeGen does not write on the blueprint. At that stage that seems more consistent to what clang-repl already does to map each partial translation unit to a new Module.

This change will fixes a long-standing invalid memory access reported by valgrind when we enable the TBAA optimization passes. It also unblock progress on llvm/llvm-project#84758.

---

Full diff: https://github.com/llvm/llvm-project/pull/89031.diff

1 Files Affected:

• (modified) clang/lib/Interpreter/IncrementalExecutor.cpp (+10-1)

diff --git a/clang/lib/Interpreter/IncrementalExecutor.cpp b/clang/lib/Interpreter/IncrementalExecutor.cpp
index 6f036107c14a9c..e87f43f077f379 100644
--- a/clang/lib/Interpreter/IncrementalExecutor.cpp
+++ b/clang/lib/Interpreter/IncrementalExecutor.cpp
@@ -28,6 +28,7 @@
 #include "llvm/IR/Module.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/TargetSelect.h"
+#include "llvm/Transforms/Utils/Cloning.h"
 
 // Force linking some of the runtimes that helps attaching to a debugger.
 LLVM_ATTRIBUTE_USED void linkComponents() {
@@ -73,7 +74,15 @@ llvm::Error IncrementalExecutor::addModule(PartialTranslationUnit &PTU) {
       Jit->getMainJITDylib().createResourceTracker();
   ResourceTrackers[&PTU] = RT;
 
-  return Jit->addIRModule(RT, {std::move(PTU.TheModule), TSCtx});
+  // Clang's CodeGen is designed to work with a single llvm::Module. In many
+  // cases for convenience various CodeGen parts have a reference to the
+  // llvm::Module (TheModule or Module) which does not change when a new module
+  // is pushed. However, the execution engine wants to take ownership of the
+  // module which does not map well to CodeGen's design. To work this around
+  // we clone the module and pass it down.
+  std::unique_ptr<llvm::Module> ModuleClone = llvm::CloneModule(*PTU.TheModule);
+
+  return Jit->addIRModule(RT, {std::move(ModuleClone), TSCtx});
 }
 
 llvm::Error IncrementalExecutor::removeModule(PartialTranslationUnit &PTU) {

[vgvassilev]

Even if we are attaching a resource tracker (RT) the Jit decides to destroy the ThreadSafeModule.

[vgvassilev]

After an offline discussion with @lhames, we have a more simplified approach which should consume less memory. Now we just keep the first llvm::Module empty and make sure it's used only for read-only purposes such as computing the llvm::DataLayout out of it in CodeGen.

[hahnjo]

Just to confirm my understanding of the change: this is an independent deduplication cleanup, right?

[vgvassilev]

Yes, independent clean up.

[hahnjo]

I believe this comment needs an update?

[vgvassilev]

Fixed.

[lhames]

If there's a trivial reproducer for the original issue it may be worth adding it as a regression test.

[vgvassilev]

If there's a trivial reproducer for the original issue it may be worth adding it as a regression test.

It is manifested in #84758 which has a test. That PR will unblock the other one which will make the test more explicit by changing the valgrind invalid memory access into a proper crash.

[vgvassilev]

Looks like the windows builders have been dead since a day or two...