-
Notifications
You must be signed in to change notification settings - Fork 60
Rely on the placement new version provided by clang-repl. #912
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -132,6 +132,21 @@ | |
| #if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN) | ||
| struct __clang_Interpreter_NewTag { | ||
| } __ci_newtag; | ||
|
|
||
| // Local forwarder for `operator new(size_t, void*, | ||
| // __clang_Interpreter_NewTag)`. clang-repl's Runtimes string declares it (LLVM | ||
| // 18+, llvm/llvm-project@1566f1ffc6b5) but the definition lives in | ||
| // libclangInterpreter -- embedders that dlopen libclangCppInterOp with | ||
| // RTLD_LOCAL (cppyy, the DispatchTests binary) have no path to that | ||
| // symbol. Registered against the mangled name via DefineAbsoluteSymbol | ||
| // at interpreter creation; see DispatchSmokeTest.TaggedPlacementNewResolvable. | ||
| namespace { | ||
| void* CppInterOpPlacementNew(std::size_t, void* __p, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: invalid case style for parameter '__p' [readability-identifier-naming] void* CppInterOpPlacementNew(std::size_t, void* __p,
^this fix will not be applied because it overlaps with another fix |
||
| __clang_Interpreter_NewTag) noexcept { | ||
| return __p; | ||
| } | ||
| } // namespace | ||
|
|
||
| #if CLANG_VERSION_MAJOR > 21 | ||
| extern "C" void* __clang_Interpreter_SetValueWithAlloc(void* This, void* OutVal, | ||
| void* OpaqueType); | ||
|
|
@@ -3034,6 +3049,34 @@ | |
| callbuf << ")"; | ||
| } | ||
|
|
||
| // Tag appended inside `::new (ptr<tag>) T(...)` when emitting a scalar | ||
| // placement new in a JitCall wrapper. | ||
| // | ||
| // clang-repl's Runtimes string declares the scalar tagged overload | ||
| // `operator new(size_t, void*, __clang_Interpreter_NewTag)` (introduced | ||
| // in llvm/llvm-project@1566f1ffc6b5, LLVM 18), so the spelling | ||
| // `::new (p, __ci_newtag) T(...)` binds without the user's TU having | ||
| // `#include <new>` in scope. Array placement in | ||
| // `make_narg_ctor_with_return` is implemented as a loop of scalar tagged | ||
| // placements for the same reason. | ||
| // | ||
| // Cling has no such tag; its runtime makes `<new>` available by default, | ||
| // so the empty tag suffices there (plain scalar placement new). | ||
| // | ||
| // Every placement-new emission uses the unary `::` form so name lookup | ||
| // skips any class-scope `operator new`. Per [class.free]/2, if the | ||
| // allocated type is a class with a class-scope `operator new`, global | ||
| // scope is not consulted as a fallback -- overload resolution against | ||
| // the class's single-arg allocator then fails and the whole wrapper | ||
| // refuses to compile (observed on cppyy's test14_new_overloader). | ||
| inline const char* PlacementTag() { | ||
| #ifdef CPPINTEROP_USE_CLING | ||
| return ""; | ||
| #else | ||
| return ", __ci_newtag"; | ||
| #endif | ||
| } | ||
|
|
||
| void make_narg_ctor_with_return(const FunctionDecl* FD, const unsigned N, | ||
| const std::string& class_name, | ||
| std::ostringstream& buf, int indent_level) { | ||
|
|
@@ -3057,44 +3100,65 @@ | |
| indent(callbuf, indent_level); | ||
| const auto* CD = dyn_cast<CXXConstructorDecl>(FD); | ||
|
|
||
| // Activate this block only if array new is possible | ||
| // if (nary) { | ||
| // (*(ClassName**)ret) = (obj) ? new (*(ClassName**)ret) ClassName[nary] | ||
| // : new ClassName[nary]; | ||
| // } | ||
| // else { | ||
| // Array branch. The is_arena side emits a loop of scalar placement | ||
| // calls rather than `new (p) T[n]`. Two alternatives were considered | ||
| // and rejected: | ||
| // | ||
| // (a) Forward-declare a tagged | ||
| // `operator new[](size_t, void*, __clang_Interpreter_NewTag)` | ||
| // and emit `new (p, __ci_newtag) T[n]`. Cheapest in per-wrapper | ||
| // emission, but clang does not recognise a custom-signature | ||
| // array allocator as the standard placement form and inserts | ||
| // an array cookie for types with non-trivial destructors | ||
| // (Itanium C++ ABI §2.7), breaking the | ||
| // `Cpp::Construct(scope, arena, n) == arena` contract. | ||
| // | ||
| // (b) Forward-declare the STANDARD-signature | ||
| // `operator new[](size_t, void*)`. Clang would signature-match | ||
| // this as the placement form (no cookie), but the declaration | ||
| // is not portably replicable: libstdc++, libc++, and the MSVC | ||
| // STL decorate it with different noexcept macros, calling | ||
| // conventions, and `[[nodiscard]]` attributes. A | ||
| // user-supplied `#include <new>` after interpreter creation | ||
| // would clash with our declaration and crash the parse. | ||
| // | ||
| // The loop binds against the already-declared scalar tagged | ||
| // placement operator (PlacementTag()), adds only O(6) lines per | ||
| // wrapper, and works on cling too (`<new>` is pre-included there, | ||
| // so the empty tag is equivalent to plain scalar placement new). | ||
| if (CD->isDefaultConstructor()) { | ||
| callbuf << "if (nary > 1) {\n"; | ||
| indent(callbuf, indent_level); | ||
| callbuf << "(*(" << class_name << "**)ret) = "; | ||
| callbuf << "(is_arena) ? new (*(" << class_name << "**)ret) "; | ||
| make_narg_ctor(FD, N, typedefbuf, callbuf, class_name, indent_level, | ||
| true); | ||
|
|
||
| callbuf << ": new "; | ||
| // | ||
| // Write the actual expression. | ||
| // | ||
| callbuf << "if (is_arena)\n"; | ||
| indent(callbuf, indent_level + 1); | ||
| callbuf << "for (unsigned long __i = 0; __i < nary; ++__i)\n"; | ||
| indent(callbuf, indent_level + 2); | ||
| callbuf << "::new ((void*)(*(" << class_name << "**)ret + __i)" | ||
| << PlacementTag() << ") " << class_name << "();\n"; | ||
| indent(callbuf, indent_level); | ||
| callbuf << "else (*(" << class_name << "**)ret) = new "; | ||
| make_narg_ctor(FD, N, typedefbuf, callbuf, class_name, indent_level, | ||
| true); | ||
| // | ||
| // End the new expression statement. | ||
| // | ||
| callbuf << ";\n"; | ||
| indent(callbuf, indent_level); | ||
| callbuf << "}\n"; | ||
| callbuf << "else {\n"; | ||
| } | ||
|
|
||
| // Standard branch: | ||
| // (*(ClassName**)ret) = (obj) ? new (*(ClassName**)ret) ClassName(args...) | ||
| // : new ClassName(args...); | ||
| // Standard (scalar) branch: | ||
| // (*(ClassName**)ret) = (is_arena) | ||
| // ? (::new (*(ClassName**)ret[, __ci_newtag]) ClassName(args...)) | ||
| // : new ClassName(args...); | ||
| // Parens around `::new` are required: clang mis-parses `? ::new` | ||
| // inside a conditional expression and reports `expected expression` | ||
| // at the `:`. | ||
| indent(callbuf, indent_level); | ||
| callbuf << "(*(" << class_name << "**)ret) = "; | ||
| callbuf << "(is_arena) ? new (*(" << class_name << "**)ret) "; | ||
| callbuf << "(is_arena) ? (::new (*(" << class_name << "**)ret" | ||
| << PlacementTag() << ") "; | ||
| make_narg_ctor(FD, N, typedefbuf, callbuf, class_name, indent_level); | ||
|
|
||
| callbuf << ": new "; | ||
| callbuf << ") : new "; | ||
| // | ||
| // Write the actual expression. | ||
| // | ||
|
|
@@ -3182,7 +3246,8 @@ | |
| // Write the placement part of the placement new. | ||
| // | ||
| indent(callbuf, indent_level); | ||
| callbuf << "new (ret) "; | ||
| // See PlacementTag for the rationale of the tag and the `::`. | ||
| callbuf << "::new (ret" << PlacementTag() << ") "; | ||
| // | ||
| // Write the type part of the placement new. | ||
| // | ||
|
|
@@ -4168,6 +4233,38 @@ | |
| #if !defined(CPPINTEROP_USE_CLING) && !defined(EMSCRIPTEN) | ||
| DefineAbsoluteSymbol(*I, "__ci_newtag", | ||
| reinterpret_cast<uint64_t>(&__ci_newtag)); | ||
|
|
||
| // Register our forwarding definition of the tagged placement | ||
| // `operator new(size_t, void*, __clang_Interpreter_NewTag)`. Look up | ||
| // the Decl introduced by clang-repl's Runtimes string, compute its | ||
| // mangled name, and point it at CppInterOpPlacementNew above. This | ||
| // shields embedders that do not expose libclangInterpreter's symbols | ||
| // (e.g. cppyy, or the DispatchTests binary which dlopens us with | ||
| // RTLD_LOCAL) from `Symbols not found: | ||
| // [ _ZnwmPv26__clang_Interpreter_NewTag ]` at JIT link time. The | ||
| // regression is caught by DispatchSmokeTest.PlacementConstructTaggedNew. | ||
| { | ||
| Sema& S = I->getSema(); | ||
| ASTContext& Ctx = S.getASTContext(); | ||
| DeclarationName DN = Ctx.DeclarationNames.getCXXOperatorName(OO_New); | ||
| LookupResult R(S, DN, SourceLocation(), Sema::LookupOrdinaryName); | ||
| S.LookupQualifiedName(R, Ctx.getTranslationUnitDecl()); | ||
| for (auto* D : R) { | ||
| auto* FD = dyn_cast<FunctionDecl>(D); | ||
| if (!FD || FD->getNumParams() != 3) | ||
| continue; | ||
| QualType LastTy = FD->getParamDecl(2)->getType(); | ||
| const auto* RD = LastTy->getAsCXXRecordDecl(); | ||
| if (!RD || RD->getName() != "__clang_Interpreter_NewTag") | ||
| continue; | ||
| std::string mangled; | ||
| compat::maybeMangleDeclName(GlobalDecl(FD), mangled); | ||
| DefineAbsoluteSymbol(*I, mangled.c_str(), | ||
| reinterpret_cast<uint64_t>(&CppInterOpPlacementNew)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast] ^ |
||
| break; | ||
| } | ||
| } | ||
|
|
||
| // llvm >= 21 has this defined as a C symbol that does not require mangling | ||
| #if CLANG_VERSION_MAJOR >= 21 | ||
| DefineAbsoluteSymbol( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,7 +103,7 @@ TEST(DispatchSmokeTest, TemplateInstantiation) { | |
| // --- Construct / Destruct --- | ||
|
|
||
| TEST(DispatchSmokeTest, ConstructDestruct) { | ||
| Cpp::CreateInterpreter({"-include", "new"}); | ||
| Cpp::CreateInterpreter({}); | ||
| Cpp::Declare("struct DispObj { int x = 7; };"); | ||
|
|
||
| auto* scope = Cpp::GetNamed("DispObj"); | ||
|
|
@@ -114,6 +114,59 @@ TEST(DispatchSmokeTest, ConstructDestruct) { | |
| Cpp::Destruct(obj, scope, /*withFree=*/true); | ||
| } | ||
|
|
||
| // End-to-end guard: after the JitCall wrapper is switched to emit | ||
| // `, __ci_newtag` in scalar placement-new expressions, `Cpp::Construct` | ||
| // on a user-supplied arena must land the object at the provided address | ||
| // (no array cookie, no extra indirection). TaggedPlacementNewResolvable | ||
| // above already pins the JIT-link side; this test pins the wrapper | ||
| // emission side. Fires if a future change drops the tag, emits a | ||
| // custom-signature array allocator that inserts an Itanium ABI cookie | ||
| // (Itanium C++ ABI S2.7), or otherwise violates `Construct(s,a) == a`. | ||
| TEST(DispatchSmokeTest, PlacementConstructTaggedNew) { | ||
| Cpp::CreateInterpreter({}); | ||
| Cpp::Declare("struct DispPlace { int x = 42; };"); | ||
|
|
||
| auto* scope = Cpp::GetNamed("DispPlace"); | ||
| ASSERT_NE(scope, nullptr); | ||
|
|
||
| void* arena = Cpp::Allocate(scope); | ||
| ASSERT_NE(arena, nullptr); | ||
|
|
||
| EXPECT_EQ(Cpp::Construct(scope, arena), arena); | ||
| EXPECT_EQ(*reinterpret_cast<int*>(arena), 42); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast] EXPECT_EQ(*reinterpret_cast<int*>(arena), 42);
^ |
||
|
|
||
| Cpp::Destruct(arena, scope, /*withFree=*/false, 0); | ||
| Cpp::Deallocate(scope, arena); | ||
| } | ||
|
|
||
| // Regression guard for the tagged placement-new JIT-link path. clang-repl's | ||
| // Runtimes string declares `operator new(size_t, void*, | ||
| // __clang_Interpreter_NewTag)`, and the definition lives in | ||
| // libclangInterpreter. This binary loads libclangCppInterOp via | ||
| // dlopen(RTLD_LOCAL) and does not link it directly, so the definition is | ||
| // NOT reachable through the process symbol table. The only resolution | ||
| // path is the DefineAbsoluteSymbol registration CppInterOp performs at | ||
| // interpreter creation; if that registration is lost (or its name is | ||
| // interned without the platform's global prefix), JIT link fails here | ||
| // with `Symbols not found: [ _ZnwmPv26__clang_Interpreter_NewTag ]`. | ||
| // The test drives the lookup directly via user-level code rather than | ||
| // through a JitCall wrapper so it fires whether or not the wrapper | ||
| // emitter has been switched to emit the tagged form. | ||
| TEST(DispatchSmokeTest, TaggedPlacementNewResolvable) { | ||
| #ifdef CPPINTEROP_USE_CLING | ||
| GTEST_SKIP() << "Cling does not use the __ci_newtag overload."; | ||
| #endif | ||
| Cpp::CreateInterpreter({}); | ||
| ASSERT_EQ(0, Cpp::Declare("struct DispTagProbe { int x = 0; };")); | ||
| EXPECT_EQ(0, Cpp::Process("char __buf[sizeof(DispTagProbe)];\n" | ||
| "new (__buf, __ci_newtag) DispTagProbe();\n")) | ||
| << "Tagged placement-new resolution failed. If the JIT reports " | ||
| "'Symbols not found: _ZnwmPv26__clang_Interpreter_NewTag', the " | ||
| "CppInterOpPlacementNew forwarding definition is no longer " | ||
| "registered with the JIT dylib (or the name is interned without " | ||
| "the target's global-symbol prefix)."; | ||
| } | ||
|
|
||
| // --- Enum --- | ||
|
|
||
| TEST(DispatchSmokeTest, EnumReflection) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: declaration uses identifier '__p', which is a reserved identifier [bugprone-reserved-identifier]
this fix will not be applied because it overlaps with another fix