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); + } +}