Skip to content

[clang-repl] We do not need to call new in the object allocation. #78843

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 21, 2024

Conversation

vgvassilev
Copy link
Contributor

This test demonstrates template instantiation via the interpreter code. In order to do that we can allocate the object on the stack and extend its lifetime by boxing it into a clang::Value.

That avoids the subtle problem where we call the new operator on an object only known to the interpreter and we cannot destroy it from compiled code since there is not suitable facility in clang::Value yet.

That should resolve the asan issues that was reported in #76218.

This test demonstrates template instantiation via the interpreter code. In order
to do that we can allocate the object on the stack and extend its lifetime by
boxing it into a clang::Value.

That avoids the subtle problem where we call the new operator on an object only
known to the interpreter and we cannot destroy it from compiled code since there
is not suitable facility in clang::Value yet.

That should resolve the asan issues that was reported in llvm#76218.
@vgvassilev vgvassilev requested a review from vitalybuka January 20, 2024 11:41
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Jan 20, 2024
@llvmbot
Copy link
Member

llvmbot commented Jan 20, 2024

@llvm/pr-subscribers-clang

Author: Vassil Vassilev (vgvassilev)

Changes

This test demonstrates template instantiation via the interpreter code. In order to do that we can allocate the object on the stack and extend its lifetime by boxing it into a clang::Value.

That avoids the subtle problem where we call the new operator on an object only known to the interpreter and we cannot destroy it from compiled code since there is not suitable facility in clang::Value yet.

That should resolve the asan issues that was reported in llvm/llvm-project#76218.


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

1 Files Affected:

  • (modified) clang/unittests/Interpreter/InterpreterTest.cpp (+6-9)
diff --git a/clang/unittests/Interpreter/InterpreterTest.cpp b/clang/unittests/Interpreter/InterpreterTest.cpp
index d6eb0684ba49d8..e76c0677db5ead 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; }
@@ -257,7 +251,12 @@ TEST(IncrementalProcessing, FindMangledNameSymbol) {
 static Value AllocateObject(TypeDecl *TD, Interpreter &Interp) {
   std::string Name = TD->getQualifiedNameAsString();
   Value Addr;
-  cantFail(Interp.ParseAndExecute("new " + Name + "()", &Addr));
+  // FIXME: Consider providing an option in clang::Value to take ownership of
+  // the memory created from the interpreter.
+  // cantFail(Interp.ParseAndExecute("new " + Name + "()", &Addr));
+
+  // The lifetime of the temporary is extended by the clang::Value.
+  cantFail(Interp.ParseAndExecute(Name + "()", &Addr));
   return Addr;
 }
 
@@ -317,8 +316,6 @@ 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());
 }
 
 #ifdef CLANG_INTERPRETER_NO_SUPPORT_EXEC

Copy link
Collaborator

@vitalybuka vitalybuka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants