From e18b1cdc3e071b89b85b1308e97ad03948102998 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 13 Jul 2026 00:59:49 -0700 Subject: [PATCH] Don't emit duplicate members for structs with multiple vtbl bases A struct deriving from two or more bases that each carry a virtual table flattens every base into the single lpVtbl. Members from different bases that map to the same C# name and signature (most notably each base's virtual destructor becoming Dispose) were emitted more than once, producing a CS0111 compile error. Deduplicate the flattened vtbl members by their emitted name and canonical parameter types so the output at least compiles. The bindings remain incomplete for multiple virtual bases (a warning is still reported), since correctly modeling that requires a distinct vtable pointer per base subobject. Also ensure the "multiple virtual bases" warning fires even when the derived type introduces its own virtual method. Previously HasVtbl only counted the indirect vtables when the derived type had no vtable of its own, so the non-unifying case (distinct secondary-base methods flattened onto the primary lpVtbl) was silently emitted with incorrect dispatch and no diagnostic. Fixes #592 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../PInvokeGenerator.VisitRecordDecl.cs | 69 ++++-- .../PInvokeGenerator.cs | 29 ++- .../MultipleBaseVtblDuplicationTest.cs | 226 ++++++++++++++++++ 3 files changed, 297 insertions(+), 27 deletions(-) create mode 100644 tests/ClangSharp.PInvokeGenerator.UnitTests/MultipleBaseVtblDuplicationTest.cs diff --git a/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.VisitRecordDecl.cs b/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.VisitRecordDecl.cs index db9dd758..9ef6edcf 100644 --- a/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.VisitRecordDecl.cs +++ b/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.VisitRecordDecl.cs @@ -532,7 +532,7 @@ private void VisitRecordDecl(RecordDecl recordDecl) if (hasVtbl || hasBaseVtbl) { - OutputDelegateSignatures(cxxRecordDecl, cxxRecordDecl); + OutputDelegateSignatures(cxxRecordDecl, cxxRecordDecl, new HashSet(StringComparer.Ordinal)); } } @@ -565,7 +565,7 @@ private void VisitRecordDecl(RecordDecl recordDecl) _outputBuilder.EmitFnPtrSupport(); } - OutputVtblHelperMethods(cxxRecordDecl, cxxRecordDecl); + OutputVtblHelperMethods(cxxRecordDecl, cxxRecordDecl, new HashSet(StringComparer.Ordinal)); if (_config.GenerateMarkerInterfaces) { @@ -587,7 +587,7 @@ private void VisitRecordDecl(RecordDecl recordDecl) } _outputBuilder.BeginExplicitVtbl(); - OutputVtblEntries(cxxRecordDecl, cxxRecordDecl); + OutputVtblEntries(cxxRecordDecl, cxxRecordDecl, new HashSet(StringComparer.Ordinal)); _outputBuilder.EndExplicitVtbl(); } } @@ -618,7 +618,7 @@ string FixupNameForMultipleHits(CXXMethodDecl cxxMethodDecl) return remappedName; } - void OutputDelegateSignatures(CXXRecordDecl rootCxxRecordDecl, CXXRecordDecl cxxRecordDecl) + void OutputDelegateSignatures(CXXRecordDecl rootCxxRecordDecl, CXXRecordDecl cxxRecordDecl, HashSet emittedMemberNames) { if (!_config.ExcludeFnptrCodegen) { @@ -628,7 +628,7 @@ void OutputDelegateSignatures(CXXRecordDecl rootCxxRecordDecl, CXXRecordDecl cxx foreach (var cxxBaseSpecifier in cxxRecordDecl.Bases) { var baseCxxRecordDecl = GetRecordDecl(cxxBaseSpecifier); - OutputDelegateSignatures(rootCxxRecordDecl, baseCxxRecordDecl); + OutputDelegateSignatures(rootCxxRecordDecl, baseCxxRecordDecl, emittedMemberNames); } var cxxMethodDecls = cxxRecordDecl.Methods; @@ -647,9 +647,15 @@ void OutputDelegateSignatures(CXXRecordDecl rootCxxRecordDecl, CXXRecordDecl cxx continue; } + var remappedName = FixupNameForMultipleHits(cxxMethodDecl); + + if (!emittedMemberNames.Add(GetVtblMemberDeduplicationKey(remappedName, cxxMethodDecl))) + { + continue; + } + _outputBuilder.WriteDivider(); - var remappedName = FixupNameForMultipleHits(cxxMethodDecl); Debug.Assert(CurrentContext.Cursor == rootCxxRecordDecl); Visit(cxxMethodDecl); } @@ -772,12 +778,12 @@ void OutputMarkerInterfaces(CXXRecordDecl rootCxxRecordDecl, CXXRecordDecl cxxRe } } - void OutputVtblEntries(CXXRecordDecl rootCxxRecordDecl, CXXRecordDecl cxxRecordDecl) + void OutputVtblEntries(CXXRecordDecl rootCxxRecordDecl, CXXRecordDecl cxxRecordDecl, HashSet emittedMemberNames) { foreach (var cxxBaseSpecifier in cxxRecordDecl.Bases) { var baseCxxRecordDecl = GetRecordDecl(cxxBaseSpecifier); - OutputVtblEntries(rootCxxRecordDecl, baseCxxRecordDecl); + OutputVtblEntries(rootCxxRecordDecl, baseCxxRecordDecl, emittedMemberNames); } var cxxMethodDecls = cxxRecordDecl.Methods; @@ -786,12 +792,12 @@ void OutputVtblEntries(CXXRecordDecl rootCxxRecordDecl, CXXRecordDecl cxxRecordD { foreach (var cxxMethodDecl in cxxMethodDecls.OrderBy((cxxmd) => cxxmd.VtblIndex)) { - OutputVtblEntry(rootCxxRecordDecl, cxxMethodDecl); + OutputVtblEntry(rootCxxRecordDecl, cxxMethodDecl, emittedMemberNames); } } } - void OutputVtblEntry(CXXRecordDecl cxxRecordDecl, CXXMethodDecl cxxMethodDecl) + void OutputVtblEntry(CXXRecordDecl cxxRecordDecl, CXXMethodDecl cxxMethodDecl, HashSet emittedMemberNames) { if (!cxxMethodDecl.IsVirtual) { @@ -814,6 +820,11 @@ void OutputVtblEntry(CXXRecordDecl cxxRecordDecl, CXXMethodDecl cxxMethodDecl) var remappedName = FixupNameForMultipleHits(cxxMethodDecl); var escapedName = EscapeAndStripMethodName(remappedName); + if (!emittedMemberNames.Add(GetVtblMemberDeduplicationKey(escapedName, cxxMethodDecl))) + { + return; + } + var desc = new FieldDesc { AccessSpecifier = AccessSpecifier.Public, NativeTypeName = nativeTypeName, @@ -837,7 +848,7 @@ void OutputVtblEntry(CXXRecordDecl cxxRecordDecl, CXXMethodDecl cxxMethodDecl) _outputBuilder.WriteDivider(); } - void OutputVtblHelperMethod(CXXRecordDecl cxxRecordDecl, CXXMethodDecl cxxMethodDecl) + void OutputVtblHelperMethod(CXXRecordDecl cxxRecordDecl, CXXMethodDecl cxxMethodDecl, HashSet emittedMemberNames) { if (!cxxMethodDecl.IsVirtual) { @@ -849,6 +860,11 @@ void OutputVtblHelperMethod(CXXRecordDecl cxxRecordDecl, CXXMethodDecl cxxMethod return; } + if (!emittedMemberNames.Add(GetVtblMemberDeduplicationKey(EscapeAndStripMethodName(GetRemappedCursorName(cxxMethodDecl)), cxxMethodDecl))) + { + return; + } + var currentContext = _context.AddLast((cxxMethodDecl, null)); var returnType = cxxMethodDecl.ReturnType; @@ -1092,12 +1108,12 @@ void OutputVtblHelperMethod(CXXRecordDecl cxxRecordDecl, CXXMethodDecl cxxMethod _context.RemoveLast(); } - void OutputVtblHelperMethods(CXXRecordDecl rootCxxRecordDecl, CXXRecordDecl cxxRecordDecl) + void OutputVtblHelperMethods(CXXRecordDecl rootCxxRecordDecl, CXXRecordDecl cxxRecordDecl, HashSet emittedMemberNames) { foreach (var cxxBaseSpecifier in cxxRecordDecl.Bases) { var baseCxxRecordDecl = GetRecordDecl(cxxBaseSpecifier); - OutputVtblHelperMethods(rootCxxRecordDecl, baseCxxRecordDecl); + OutputVtblHelperMethods(rootCxxRecordDecl, baseCxxRecordDecl, emittedMemberNames); } var cxxMethodDecls = cxxRecordDecl.Methods; @@ -1107,7 +1123,7 @@ void OutputVtblHelperMethods(CXXRecordDecl rootCxxRecordDecl, CXXRecordDecl cxxR foreach (var cxxMethodDecl in cxxMethodDecls.OrderBy((cxxmd) => cxxmd.VtblIndex)) { _outputBuilder.WriteDivider(); - OutputVtblHelperMethod(rootCxxRecordDecl, cxxMethodDecl); + OutputVtblHelperMethod(rootCxxRecordDecl, cxxMethodDecl, emittedMemberNames); } } } @@ -2023,4 +2039,29 @@ void StartTestOutput(ref bool testOutputStarted, bool generateTestsClass, bool i } } } + + // Multiple base classes can each contribute a virtual member that maps to the same C# name and + // signature (most notably each base's virtual destructor becoming `Dispose`). The single `lpVtbl` + // model flattens every base into one vtable, so the same member would otherwise be emitted more + // than once, which is a compile error. This builds a key of the emitted name plus the canonical + // parameter types so that legitimate overloads remain distinct. See https://github.com/dotnet/ClangSharp/issues/592 + private static string GetVtblMemberDeduplicationKey(string emittedName, CXXMethodDecl cxxMethodDecl) + { + var builder = new StringBuilder(emittedName); + _ = builder.Append('('); + + var parameters = cxxMethodDecl.Parameters; + + for (var index = 0; index < parameters.Count; index++) + { + if (index != 0) + { + _ = builder.Append(','); + } + _ = builder.Append(parameters[index].Type.CanonicalType.AsString); + } + + _ = builder.Append(')'); + return builder.ToString(); + } } diff --git a/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.cs b/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.cs index da28b2e9..681dc60b 100644 --- a/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.cs +++ b/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.cs @@ -1430,25 +1430,28 @@ private bool HasVtbl(CXXRecordDecl cxxRecordDecl, out bool hasBaseVtbl) var hasVtbl = cxxRecordDecl.Methods.Any((method) => method.IsVirtual && (method.OverriddenMethods.Count == 0)); hasBaseVtbl = false; - if (!hasVtbl) + var indirectVtblCount = 0; + + foreach (var cxxBaseSpecifier in cxxRecordDecl.Bases) { - var indirectVtblCount = 0; + var baseCxxRecordDecl = GetRecordDecl(cxxBaseSpecifier); - foreach (var cxxBaseSpecifier in cxxRecordDecl.Bases) + if ((HasVtbl(baseCxxRecordDecl, out var baseHasBaseVtbl) || baseHasBaseVtbl) && !HasField(baseCxxRecordDecl)) { - var baseCxxRecordDecl = GetRecordDecl(cxxBaseSpecifier); - - if ((HasVtbl(baseCxxRecordDecl, out var baseHasBaseVtbl) || baseHasBaseVtbl) && !HasField(baseCxxRecordDecl)) - { - indirectVtblCount++; - } + indirectVtblCount++; } + } - if (indirectVtblCount > 1) - { - AddDiagnostic(DiagnosticLevel.Warning, "Unsupported cxx record declaration: 'multiple virtual bases'. Generated bindings may be incomplete.", cxxRecordDecl); - } + // Multiple virtual bases require a distinct vtable pointer per base subobject, but the + // generated bindings only model a single, flattened `lpVtbl`. This is true even when the + // derived type introduces its own virtuals (`hasVtbl`), so the warning must fire regardless. + if (indirectVtblCount > 1) + { + AddDiagnostic(DiagnosticLevel.Warning, "Unsupported cxx record declaration: 'multiple virtual bases'. Generated bindings may be incomplete.", cxxRecordDecl); + } + if (!hasVtbl) + { hasBaseVtbl = indirectVtblCount != 0; } diff --git a/tests/ClangSharp.PInvokeGenerator.UnitTests/MultipleBaseVtblDuplicationTest.cs b/tests/ClangSharp.PInvokeGenerator.UnitTests/MultipleBaseVtblDuplicationTest.cs new file mode 100644 index 00000000..02aa7ad0 --- /dev/null +++ b/tests/ClangSharp.PInvokeGenerator.UnitTests/MultipleBaseVtblDuplicationTest.cs @@ -0,0 +1,226 @@ +// Copyright (c) .NET Foundation and Contributors. All Rights Reserved. Licensed under the MIT License (MIT). See License.md in the repository root for more information. + +using System.Threading.Tasks; +using NUnit.Framework; + +namespace ClangSharp.UnitTests; + +/// +/// Regression test for https://github.com/dotnet/ClangSharp/issues/592. +/// A struct deriving from two or more bases that each carry a virtual table flattens every base into +/// the single lpVtbl. Members from different bases that map to the same C# name and signature +/// (most notably each base's virtual destructor becoming Dispose) were emitted more than once, +/// producing a CS0111 compile error. The duplicated member is now emitted only once. The generated +/// bindings remain incomplete for multiple virtual bases (a warning is still reported), because +/// correctly modeling that requires a distinct vtable pointer per base subobject. +/// +[Platform("win")] +public sealed class MultipleBaseVtblDuplicationTest : PInvokeGeneratorTest +{ + private const string InputContents = @"struct Foo +{ + virtual ~Foo() = default; +}; + +struct Bar +{ + virtual ~Bar() = default; +}; + +struct Baz : Foo, Bar +{ +}; +"; + + [Test] + public Task DoesNotEmitDuplicateMembers() + { + var expectedOutputContents = @"using System.Runtime.CompilerServices; + +namespace ClangSharp.Test +{ + public unsafe partial struct Foo + { + public void** lpVtbl; + + public void Dispose() + { + ((delegate* unmanaged[Thiscall])(lpVtbl[0]))((Foo*)Unsafe.AsPointer(ref this)); + } + } + + public unsafe partial struct Bar + { + public void** lpVtbl; + + public void Dispose() + { + ((delegate* unmanaged[Thiscall])(lpVtbl[0]))((Bar*)Unsafe.AsPointer(ref this)); + } + } + + [NativeTypeName(""struct Baz : Foo, Bar"")] + public unsafe partial struct Baz + { + public void** lpVtbl; + + public void Dispose() + { + ((delegate* unmanaged[Thiscall])(lpVtbl[0]))((Baz*)Unsafe.AsPointer(ref this)); + } + } +} +"; + + var expectedDiagnostics = new[] { + new Diagnostic(DiagnosticLevel.Warning, "Unsupported cxx record declaration: 'multiple virtual bases'. Generated bindings may be incomplete.", "Line 11, Column 8 in ClangUnsavedFile.h") + }; + + return ValidateGeneratedCSharpLatestWindowsBindingsAsync(InputContents, expectedOutputContents, expectedDiagnostics: expectedDiagnostics); + } + + [Test] + public Task DoesNotEmitDuplicateExplicitVtblEntries() + { + var expectedOutputContents = @"using System.Runtime.CompilerServices; + +namespace ClangSharp.Test +{ + public unsafe partial struct Foo + { + public Vtbl* lpVtbl; + + public void Dispose() + { + lpVtbl->Dispose((Foo*)Unsafe.AsPointer(ref this)); + } + + public partial struct Vtbl + { + [NativeTypeName(""void () noexcept"")] + public delegate* unmanaged[Thiscall] Dispose; + } + } + + public unsafe partial struct Bar + { + public Vtbl* lpVtbl; + + public void Dispose() + { + lpVtbl->Dispose((Bar*)Unsafe.AsPointer(ref this)); + } + + public partial struct Vtbl + { + [NativeTypeName(""void () noexcept"")] + public delegate* unmanaged[Thiscall] Dispose; + } + } + + [NativeTypeName(""struct Baz : Foo, Bar"")] + public unsafe partial struct Baz + { + public Vtbl* lpVtbl; + + public void Dispose() + { + lpVtbl->Dispose((Baz*)Unsafe.AsPointer(ref this)); + } + + public partial struct Vtbl + { + [NativeTypeName(""void () noexcept"")] + public delegate* unmanaged[Thiscall] Dispose; + } + } +} +"; + + var expectedDiagnostics = new[] { + new Diagnostic(DiagnosticLevel.Warning, "Unsupported cxx record declaration: 'multiple virtual bases'. Generated bindings may be incomplete.", "Line 11, Column 8 in ClangUnsavedFile.h") + }; + + return ValidateGeneratedCSharpLatestWindowsBindingsAsync(InputContents, expectedOutputContents, PInvokeGeneratorConfigurationOptions.GenerateExplicitVtbls, expectedDiagnostics: expectedDiagnostics); + } + + // The non-unifying case: two bases contributing distinctly named virtual methods, plus a virtual + // method introduced by the derived type itself. There is no name collision here, so nothing is + // deduplicated, but the flattened single-vtable model is still wrong: the secondary base's + // `BarMethod` is dispatched through `lpVtbl[0]` (the primary vtable slot shared with `FooMethod`) + // rather than the Bar subobject's own vtable. Because the derived type introduces its own virtual + // (`BazMethod`), this case previously produced NO diagnostic at all; the warning must still fire so + // the incompleteness is never silent. + private const string NonUnifyingInputContents = @"struct Foo +{ + virtual void FooMethod(); +}; + +struct Bar +{ + virtual void BarMethod(); +}; + +struct Baz : Foo, Bar +{ + virtual void BazMethod(); +}; +"; + + [Test] + public Task WarnsForNonUnifyingMultipleBases() + { + var expectedOutputContents = @"using System.Runtime.CompilerServices; + +namespace ClangSharp.Test +{ + public unsafe partial struct Foo + { + public void** lpVtbl; + + public void FooMethod() + { + ((delegate* unmanaged[Thiscall])(lpVtbl[0]))((Foo*)Unsafe.AsPointer(ref this)); + } + } + + public unsafe partial struct Bar + { + public void** lpVtbl; + + public void BarMethod() + { + ((delegate* unmanaged[Thiscall])(lpVtbl[0]))((Bar*)Unsafe.AsPointer(ref this)); + } + } + + [NativeTypeName(""struct Baz : Foo, Bar"")] + public unsafe partial struct Baz + { + public void** lpVtbl; + + public void FooMethod() + { + ((delegate* unmanaged[Thiscall])(lpVtbl[0]))((Baz*)Unsafe.AsPointer(ref this)); + } + + public void BarMethod() + { + ((delegate* unmanaged[Thiscall])(lpVtbl[0]))((Baz*)Unsafe.AsPointer(ref this)); + } + + public void BazMethod() + { + ((delegate* unmanaged[Thiscall])(lpVtbl[1]))((Baz*)Unsafe.AsPointer(ref this)); + } + } +} +"; + + var expectedDiagnostics = new[] { + new Diagnostic(DiagnosticLevel.Warning, "Unsupported cxx record declaration: 'multiple virtual bases'. Generated bindings may be incomplete.", "Line 11, Column 8 in ClangUnsavedFile.h") + }; + + return ValidateGeneratedCSharpLatestWindowsBindingsAsync(NonUnifyingInputContents, expectedOutputContents, expectedDiagnostics: expectedDiagnostics); + } +}