[clang-repl] Add a interpreter-specific overload of operator new for C++

[vgvassilev]

This patch brings back the basic support for C by inserting the required for value printing runtime only when we are in C++ mode. Additionally, it defines a new overload of operator placement new because we can't really forward declare it in a library-agnostic way.

Fixes the issue described in #69072.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Vassil Vassilev (vgvassilev)

Changes

This patch brings back the basic support for C by inserting the required for value printing runtime only when we are in C++ mode. Additionally, it defines a new overload of operator placement new because we can't really forward declare it in a library-agnostic way.

Fixes the issue described in llvm/llvm-project#69072.

---

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

2 Files Affected:

• (modified) clang/lib/Interpreter/Interpreter.cpp (+16-2)
• (modified) clang/test/Interpreter/incremental-mode.cpp (+2-1)

diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index c9fcef5b5b5af1..daceabafe4c938 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -248,7 +248,7 @@ Interpreter::~Interpreter() {
 // can't find the precise resource directory in unittests so we have to hard
 // code them.
 const char *const Runtimes = R"(
-    void* operator new(__SIZE_TYPE__, void* __p) noexcept;
+#ifdef __cplusplus
     void *__clang_Interpreter_SetValueWithAlloc(void*, void*, void*);
     void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*);
     void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*, void*);
@@ -256,15 +256,18 @@ const char *const Runtimes = R"(
     void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*, double);
     void __clang_Interpreter_SetValueNoAlloc(void*, void*, void*, long double);
     void __clang_Interpreter_SetValueNoAlloc(void*,void*,void*,unsigned long long);
+    struct __clang_Interpreter_NewTag{};
+    void* operator new(__SIZE_TYPE__, void* __p, __clang_Interpreter_NewTag) noexcept;
     template <class T, class = T (*)() /*disable for arrays*/>
     void __clang_Interpreter_SetValueCopyArr(T* Src, void* Placement, unsigned long Size) {
       for (auto Idx = 0; Idx < Size; ++Idx)
-        new ((void*)(((T*)Placement) + Idx)) T(Src[Idx]);
+        new ((void*)(((T*)Placement) + Idx), __clang_Interpreter_NewTag()) T(Src[Idx]);
     }
     template <class T, unsigned long N>
     void __clang_Interpreter_SetValueCopyArr(const T (*Src)[N], void* Placement, unsigned long Size) {
       __clang_Interpreter_SetValueCopyArr(Src[0], Placement, Size);
     }
+#endif // __cplusplus
 )";
 
 llvm::Expected<std::unique_ptr<Interpreter>>
@@ -814,3 +817,14 @@ __clang_Interpreter_SetValueNoAlloc(void *This, void *OutVal, void *OpaqueType,
   VRef = Value(static_cast<Interpreter *>(This), OpaqueType);
   VRef.setLongDouble(Val);
 }
+
+// A trampoline to work around the fact that operator placement new cannot
+// really be forward declared due to libc++ and libstdc++ declaration mismatch.
+// FIXME: __clang_Interpreter_NewTag is ODR violation because we get the same
+// definition in the interpreter runtime. We should move it in a runtime header
+// which gets included by the interpreter and here.
+struct __clang_Interpreter_NewTag{};
+void* operator new(__SIZE_TYPE__ __sz, void* __p, __clang_Interpreter_NewTag) noexcept {
+  // Just forward to the standard operator placement new.
+  return operator new(__sz, __p);
+}
diff --git a/clang/test/Interpreter/incremental-mode.cpp b/clang/test/Interpreter/incremental-mode.cpp
index e6350d237ef578..d63cee0dd6d15f 100644
--- a/clang/test/Interpreter/incremental-mode.cpp
+++ b/clang/test/Interpreter/incremental-mode.cpp
@@ -1,3 +1,4 @@
 // RUN: clang-repl -Xcc -E
-// RUN: clang-repl -Xcc -emit-llvm 
+// RUN: clang-repl -Xcc -emit-llvm
+// RUN: clang-repl -Xcc -xc
 // expected-no-diagnostics

[vgvassilev]

This PR should help compiler-research/CppInterOp#173 move forward.

cc: @makslevental, @alexander-penev, @mcbarton

[vgvassilev]

Ping.

[hahnjo]

Looks good (sorry, this fell through)

[vitalybuka]

Looks like after this patch https://lab.llvm.org/buildbot/#/builders/236/builds/8819

[vgvassilev]

If the fix is not trivial please revert it since I am not on my laptop (very late on my end)

[vitalybuka]

@vgvassilev I end up with suppression, please check, when you have time, if this can be done better way, or just remove FIXME.

[vgvassilev]

@vitalybuka, thank you!

I am trying to add a fix but I get unsupported option '-fsanitize=hwaddress' for target 'x86_64-apple-darwin21.6.0'. If you have a compatible platform, do you mind testing:

diff --git a/clang/unittests/Interpreter/InterpreterTest.cpp b/clang/unittests/Interpreter/InterpreterTest.cpp
index d6eb0684ba49..406a4871dff5 100644
--- a/clang/unittests/Interpreter/InterpreterTest.cpp
+++ b/clang/unittests/Interpreter/InterpreterTest.cpp
@@ -34,12 +34,6 @@ using namespace clang;
 #define CLANG_INTERPRETER_NO_SUPPORT_EXEC
 #endif
 
-#if LLVM_ADDRESS_SANITIZER_BUILD || LLVM_HWADDRESS_SANITIZER_BUILD
-#include <sanitizer/lsan_interface.h>
-#else
-extern "C" void __lsan_ignore_object(const void *p) {}
-#endif
-
 int Global = 42;
 // JIT reports symbol not found on Windows without the visibility attribute.
 REPL_EXTERNAL_VISIBILITY int getGlobal() { return Global; }
@@ -317,8 +311,9 @@ TEST(IncrementalProcessing, InstantiateTemplate) {
   auto fn =
       cantFail(Interp->getSymbolAddress(MangledName)).toPtr<TemplateSpecFn>();
   EXPECT_EQ(42, fn(NewA.getPtr()));
-  // FIXME: release the memory.
-  __lsan_ignore_object(NewA.getPtr());
+  // FIXME: Consider providing an option in clang::Value to take ownership of
+  // the memory created from the interpreter.
+  free(NewA.getPtr());
 }
 
 #ifdef CLANG_INTERPRETER_NO_SUPPORT_EXEC

I think that's going to work.

[vitalybuka]

I suspect asan may complain on new/free mismatch

[vitalybuka]

Yes, it fixes the leak but on asan bot it's mismatch

https://lab.llvm.org/buildbot/#/builders/168/builds/18159/steps/10/logs/stdio

[vgvassilev]

Yes, you are right, I've put a fix here #78843. Can you take a look?