[MS][clang] Error about ambiguous operator delete[] only when required

[Fznamznon]

And issue was reported in
#133950 (comment) . Since we don't always emit vector deleting dtors, only error out about ambiguous operator delete[] when it will be required for vector deleting dtor emission.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Mariya Podchishchaeva (Fznamznon)

Changes

And issue was reported in
#133950 . Since we don't always emit vector deleting dtors, only error out about ambiguous operator delete[] when it will be required for vector deleting dtor emission.

---

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

3 Files Affected:

• (modified) clang/lib/Sema/SemaDeclCXX.cpp (+12-9)
• (modified) clang/lib/Sema/SemaExprCXX.cpp (+16-2)
• (modified) clang/test/SemaCXX/gh134265.cpp (+36-3)

diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index b86f7118e0b34..6732062965aef 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -11045,15 +11045,18 @@ bool Sema::CheckDestructor(CXXDestructorDecl *Destructor) {
       DiagnoseUseOfDecl(OperatorDelete, Loc);
       MarkFunctionReferenced(Loc, OperatorDelete);
       Destructor->setOperatorDelete(OperatorDelete, ThisArg);
-      // Lookup delete[] too in case we have to emit a vector deleting dtor;
-      DeclarationName VDeleteName =
-          Context.DeclarationNames.getCXXOperatorName(OO_Array_Delete);
-      FunctionDecl *ArrOperatorDelete = FindDeallocationFunctionForDestructor(
-          Loc, RD, VDeleteName, /*Diagnose=*/false);
-      // delete[] in the TU will make sure the operator is referenced and its
-      // uses diagnosed, otherwise vector deleting dtor won't be called anyway,
-      // so just record it in the destructor.
-      Destructor->setOperatorArrayDelete(ArrOperatorDelete);
+      if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+        // Lookup delete[] too in case we have to emit a vector deleting dtor;
+        DeclarationName VDeleteName =
+            Context.DeclarationNames.getCXXOperatorName(OO_Array_Delete);
+        FunctionDecl *ArrOperatorDelete = FindDeallocationFunctionForDestructor(
+            Loc, RD, VDeleteName,
+            Destructor->hasAttr<DLLExportAttr>() && Destructor->isDefined());
+        // delete[] in the TU will make sure the operator is referenced and its
+        // uses diagnosed, otherwise vector deleting dtor won't be called
+        // anyway, so just record it in the destructor.
+        Destructor->setOperatorArrayDelete(ArrOperatorDelete);
+      }
     }
   }
 
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 42bc4db20e818..247cd02b23522 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -2789,8 +2789,19 @@ bool Sema::FindAllocationFunctions(SourceLocation StartLoc, SourceRange Range,
       return true;
   }
 
+  // new[] will force emission of vector deleting dtor which needs delete[].
+  bool MaybeVectorDeletingDtor = false;
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft()) {
+    if (AllocElemType->isRecordType() && IsArray) {
+      auto *RD =
+          cast<CXXRecordDecl>(AllocElemType->castAs<RecordType>()->getDecl());
+      CXXDestructorDecl *DD = RD->getDestructor();
+      MaybeVectorDeletingDtor = DD && DD->isVirtual() && !DD->isDeleted();
+    }
+  }
+
   // We don't need an operator delete if we're running under -fno-exceptions.
-  if (!getLangOpts().Exceptions) {
+  if (!getLangOpts().Exceptions && !MaybeVectorDeletingDtor) {
     OperatorDelete = nullptr;
     return false;
   }
@@ -3290,8 +3301,11 @@ bool Sema::FindDeallocationFunction(SourceLocation StartLoc, CXXRecordDecl *RD,
   // Try to find operator delete/operator delete[] in class scope.
   LookupQualifiedName(Found, RD);
 
-  if (Found.isAmbiguous())
+  if (Found.isAmbiguous()) {
+    if (!Diagnose)
+      Found.suppressDiagnostics();
     return true;
+  }
 
   Found.suppressDiagnostics();
 
diff --git a/clang/test/SemaCXX/gh134265.cpp b/clang/test/SemaCXX/gh134265.cpp
index c7bdeb2add0cc..a2d360d7ba02a 100644
--- a/clang/test/SemaCXX/gh134265.cpp
+++ b/clang/test/SemaCXX/gh134265.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_cc1 %s -verify -fsyntax-only
+// RUN: %clang_cc1 %s -verify=expected -fsyntax-only
+// RUN: %clang_cc1 %s -verify=expected -fsyntax-only -std=c++20
+// RUN: %clang_cc1 %s -verify=expected,ms -fms-extensions -fms-compatibility -triple=x86_64-pc-windows-msvc -DMS
 
 struct Foo {
   virtual ~Foo() {} // expected-error {{attempt to use a deleted function}}
@@ -13,10 +15,41 @@ struct Bar {
 
 struct Baz {
   virtual ~Baz() {}
-  static void operator delete[](void* ptr) = delete; // expected-note {{explicitly marked deleted here}}
+  static void operator delete[](void* ptr) = delete; // expected-note {{explicitly marked deleted here}}\
+                                                        ms-note{{explicitly marked deleted here}}}
+};
+
+struct BarBaz {
+  ~BarBaz() {}
+  static void operator delete[](void* ptr) = delete;
 };
 
 void foobar() {
-  Baz *B = new Baz[10]();
+  Baz *B = new Baz[10](); // ms-error {{attempt to use a deleted function}}
   delete [] B; // expected-error {{attempt to use a deleted function}}
+  BarBaz *BB = new BarBaz[10]();
+}
+
+struct BaseDelete1 {
+  void operator delete[](void *); //ms-note 2{{member found by ambiguous name lookup}}
+};
+struct BaseDelete2 {
+  void operator delete[](void *); //ms-note 2{{member found by ambiguous name lookup}}
+};
+struct BaseDestructor {
+  BaseDestructor() {}
+  virtual ~BaseDestructor() = default;
+};
+struct Final : BaseDelete1, BaseDelete2, BaseDestructor {
+  Final() {}
+};
+
+#ifdef MS
+struct Final1 : BaseDelete1, BaseDelete2, BaseDestructor {
+  __declspec(dllexport) ~Final1() {} // ms-error {{member 'operator delete[]' found in multiple base classes of different types}}
+};
+#endif // MS
+
+void foo() {
+    Final* a = new Final[10](); // ms-error {{member 'operator delete[]' found in multiple base classes of different types}}
 }

[Fznamznon]

I'm concerned though, that MSVC calls SOME destructor and doesn't error out which seems like a bug. See https://godbolt.org/z/q8znTG5aa .

virtual void * Final::`vector deleting destructor'(unsigned int) PROC 
has a call to
call    static void BaseDelete1::operator delete[](void *)          ; BaseDelete1::operator delete[]

Should we also do the same since this is a nonconforming extension?

[DKLoehr]

Can't comment on MSVC's behavior, but this does seem to eliminate the errors we were seeing in chromium (at least for my one local build).

[Fznamznon]

Alternatively we can silently fallback to calling scalar operator delete whose presence is enforced by diagnoistics (i.e call SOMETHING in the vector deleting destructor like MSVC does) so we don't crash while emitting the destructor and rely on new[]/delete[] diagnostics.

[rnk]

I have some minor suggestions, but please go ahead and land this without additional review since it is a forward-fix.

[rnk]

Maybe throw in a third case for good measure: an explicit, inline, non-exported destructor, with ambiguous lookup. I'm guessing we want the same diagnostics we get for Final, i.e. we emit the diagnostic only when array-new gets called, as in the case with the implicit destructor.

[Fznamznon]

Added, thanks!

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.