[clang] [clang-repl] Keep the first llvm::Module empty to avoid invalid memory access. (PR #89031)

[Vassil Vassilev via cfe-commits]

https://github.com/vgvassilev updated https://github.com/llvm/llvm-project/pull/89031

>From 3c9698f033c2349e698a5c98f227bfee8499e5b4 Mon Sep 17 00:00:00 2001
From: Vassil Vassilev <v.g.vassilev at gmail.com>
Date: Fri, 19 Apr 2024 07:51:17 +0000
Subject: [PATCH] [clang-repl] Keep the first llvm::Module empty to avoid
 invalid memory access.

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 llvm/llvm-project#84758.
---
 clang/lib/Interpreter/IncrementalParser.cpp | 25 ++++++++++++++++-----
 clang/lib/Interpreter/IncrementalParser.h   |  5 +++++
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/clang/lib/Interpreter/IncrementalParser.cpp b/clang/lib/Interpreter/IncrementalParser.cpp
index 5eec2a2fd6d1a6..c87972719b66b3 100644
--- a/clang/lib/Interpreter/IncrementalParser.cpp
+++ b/clang/lib/Interpreter/IncrementalParser.cpp
@@ -209,6 +209,10 @@ IncrementalParser::IncrementalParser(Interpreter &Interp,
   if (Err)
     return;
   CI->ExecuteAction(*Act);
+
+  if (getCodeGen())
+    CachedInCodeGenModule = std::move(GenModule());
+
   std::unique_ptr<ASTConsumer> IncrConsumer =
       std::make_unique<IncrementalASTConsumer>(Interp, CI->takeASTConsumer());
   CI->setASTConsumer(std::move(IncrConsumer));
@@ -224,11 +228,8 @@ IncrementalParser::IncrementalParser(Interpreter &Interp,
     return;                     // PTU.takeError();
   }
 
-  if (CodeGenerator *CG = getCodeGen()) {
-    std::unique_ptr<llvm::Module> M(CG->ReleaseModule());
-    CG->StartModule("incr_module_" + std::to_string(PTUs.size()),
-                    M->getContext());
-    PTU->TheModule = std::move(M);
+  if (getCodeGen()) {
+    PTU->TheModule = std::move(GenModule());
     assert(PTU->TheModule && "Failed to create initial PTU");
   }
 }
@@ -364,6 +365,20 @@ IncrementalParser::Parse(llvm::StringRef input) {
 std::unique_ptr<llvm::Module> IncrementalParser::GenModule() {
   static unsigned ID = 0;
   if (CodeGenerator *CG = getCodeGen()) {
+    // 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 created an empty module to make CodeGen happy. We should make
+    // sure it always stays empty.
+    assert((!CachedInCodeGenModule ||
+            (CachedInCodeGenModule->empty() &&
+             CachedInCodeGenModule->global_empty() &&
+             CachedInCodeGenModule->alias_empty() &&
+             CachedInCodeGenModule->ifunc_empty() &&
+             CachedInCodeGenModule->named_metadata_empty())) &&
+           "CodeGen wrote to a readonly module");
     std::unique_ptr<llvm::Module> M(CG->ReleaseModule());
     CG->StartModule("incr_module_" + std::to_string(ID++), M->getContext());
     return M;
diff --git a/clang/lib/Interpreter/IncrementalParser.h b/clang/lib/Interpreter/IncrementalParser.h
index e13b74c7f65948..f63bce50acd3b9 100644
--- a/clang/lib/Interpreter/IncrementalParser.h
+++ b/clang/lib/Interpreter/IncrementalParser.h
@@ -24,6 +24,7 @@
 #include <memory>
 namespace llvm {
 class LLVMContext;
+class Module;
 } // namespace llvm
 
 namespace clang {
@@ -57,6 +58,10 @@ class IncrementalParser {
   /// of code.
   std::list<PartialTranslationUnit> PTUs;
 
+  /// When CodeGen is created the first llvm::Module gets cached in many places
+  /// and we must keep it alive.
+  std::unique_ptr<llvm::Module> CachedInCodeGenModule;
+
   IncrementalParser();
 
 public: