Fix v22 regen regressions: vftable dtor index, --with-base, Dispose, namespace qualifier - #829
Merged
Merged
Conversation
The restored `Namespace::` prefix was inserted whole whenever the spelling didn't already start with the full qualifier, so a reference that already carried a trailing run of the namespace (e.g. `Windows::Foundation::PropertyValue` spelled inside `Abi`) became `Abi::Windows::Foundation::Windows::Foundation::PropertyValue`. Insert only the leading segments the spelling is missing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A user-declared destructor lowers to `Dispose`, but `CXXDeleteExpr` was
unsupported, so `delete[] p` produced nothing and left a silently empty
`if (p != null) { }` no-op that leaks. Emit a `cxx_delete(...)` placeholder,
mirroring the existing `cxx_new<T>(...)` lowering for `new`.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Lets a generated type derive from user-supplied base types, applied to the marker `Interface` for COM/vtbl types and to the struct itself for plain value types. Restores terrafx's hand-maintained `IUnknown.Interface : INativeGuid` without a manual post-regen patch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
clang 22's MicrosoftVTableContext reports Index == 0 for a virtual destructor's vftable location regardless of its declaration position, so the destructor collides with whatever occupies slot 0 whenever it is not declared first (e.g. CHttpModule's trailing ~CHttpModule after 30 On* notification methods, which regressed from VtblIndex 30 to 0). The vftable layout itself is ordered correctly, so recover the true slot by counting the function-pointer components that precede the deleting-destructor component; non-slot components are skipped so the count stays in the same space as MethodVFTableLocation::Index. Validated by building libClangSharp against LLVM 22.1.8 locally and exercising it through both the interop layer and the generator: trailing, middle, leading, overloaded, and pure-virtual destructor placements now all resolve to the correct slot. The checked-in generator golden tests still load the pinned prebuilt libClangSharp package (which has the bug), so a baseline regression test cannot be added until a new native package is published. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The vftable destructor-index fix changes sources/libClangSharp, so the regenerate-native workflow will produce new libClangSharp.runtime.* packages. Bump the revision so they don't collide with the published 22.1.8.2. The managed libClangSharp pin in Directory.Packages.props stays at 22.1.8.2 (the published package) until 22.1.8.3 is published; bumping the pin, regenerating any affected baselines, and adding the destructor-slot regression test are a follow-up once the native package is available. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fourth batch of fixes for regressions found regenerating terrafx.interop.windows against
ClangSharp v22.1.8. Each fix is a separate commit and has its own repro; the two SERIOUS
correctness issues are G1 and G2.
G1 -- Microsoft vftable index for virtual destructors (native)
clang 22's
MicrosoftVTableContextreportsIndex == 0for a virtual destructor's vftablelocation regardless of its declaration position, so the destructor collides with whatever
occupies slot 0 whenever it is not declared first. In terrafx this moved
CHttpModule'strailing
~CHttpModulefrom[VtblIndex(30)](after the 30On*notification methods) to[VtblIndex(0)], where it aliasesOnBeginRequest-- wrong vtable dispatch.The vftable layout itself is ordered correctly, so
getVtblIdxnow recovers the destructor'strue slot by counting the function-pointer components that precede the deleting-destructor
component (non-slot components such as offsets are skipped so the count stays in the same space
as
MethodVFTableLocation::Index).Validated by building
libClangSharpagainst LLVM 22.1.8 locally and exercising it through boththe interop layer and the generator: trailing, middle, leading, overloaded, and pure-virtual
destructor placements all resolve to the correct slot, and the full generator suite still passes
with the rebuilt native (no existing baseline changes). The checked-in golden tests load the
pinned prebuilt
libClangSharppackage (which still has the bug), so the managed pin bump, anybaseline regeneration, and a destructor-slot regression test are a follow-up once
22.1.8.3ispublished -- see the version-bump commit.
G2 --
--with-basefor injecting additional base typesterrafx hand-maintains
IUnknown.Interface : INativeGuid, which the regen drops (the root COMinterface is the one interface that doesn't otherwise derive it). Rather than special-case it,
this adds a
--with-base/-wboption that lets a generation inject additional base type(s)onto a type, keyed the same way as other remaps (
Type=Base). The extra bases land on themarker
Interfacefor vtbl types when marker interfaces are generated, and on the structitself otherwise.
G3 -- empty
Dispose()for structs whose destructor frees membersA struct whose C++ destructor
delete[]s pointer members (e.g.GpPathData) emitted aDispose()whose bodies were bare empty statements -- a silent leak.VisitStmtnow emits thecxx_deletefordelete/delete[]expressions in destructor bodies.G4 -- doubled namespace qualifier in NativeTypeName
GetSource's already-qualifiedWindows::Foundation::IPropertyValue **was re-prefixed toABI::Windows::Foundation::Windows::Foundation::IPropertyValue **. The qualifier is now dedupedagainst the existing source spelling. Cosmetic (the C# type was already correct).
Out of scope / follow-up: bumping the managed
libClangSharppin to22.1.8.3, regenerating anyaffected baselines, and adding the G1 destructor-slot golden test, all of which depend on the
22.1.8.3native package being published first.