[MS][clang] Make sure vector deleting dtor calls correct operator delete

[Fznamznon]

During additional testing I spotted that vector deleting dtor calls operator delete, not operator delete[] when performing array deletion. This patch fixes that.

[github-actions]

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

[rnk]

Thanks, looks good to me!

[DKLoehr]

We're seeing several errors building chromium that bisect to this change: error: member 'operator delete[]' found in multiple base classes of different types. An example build is here; the output is

../../third_party/pdfium/xfa/fxfa/cxfa_ffexclgroup.cpp:16:19: error: member 'operator delete[]' found in multiple base classes of different types
   16 | CXFA_FFExclGroup::~CXFA_FFExclGroup() = default;
      |                   ^
../../third_party/pdfium/xfa/fxfa/cxfa_ffexclgroup.cpp:16:41: note: in defaulted destructor for 'CXFA_FFExclGroup' first required here
   16 | CXFA_FFExclGroup::~CXFA_FFExclGroup() = default;
      |                                         ^
../../v8/include/cppgc/garbage-collected.h:69:8: note: member found by ambiguous name lookup
   69 |   void operator delete[](void*) = delete;
      |        ^
../../v8/include/cppgc/garbage-collected.h:103:8: note: member found by ambiguous name lookup
  103 |   void operator delete[](void*) = delete;
      |        ^

I'm working on putting together a minimal repro.

[Fznamznon]

@DKLoehr , does clang that is being used also have #134357 in?

[DKLoehr]

Yes, I've reproduced it locally with current trunk clang.

[DKLoehr]

This is the smallest reproduction I could find:

struct BaseDelete1 {
  void operator delete[](void *);
};
struct BaseDelete2 {
  void operator delete[](void *);
};
struct BaseDestructor {
  virtual ~BaseDestructor() = default;
};
struct Final : BaseDelete1, BaseDelete2, BaseDestructor {};

When run with clang++ repro.cc -fsyntax-only -std=c++20, it yields the following error. Removing any one of the three base classes causes it to compile without problems.

repro.cc:10:8: error: member 'operator delete[]' found in multiple base classes of different types
   10 | struct Final : BaseDelete1, BaseDelete2, BaseDestructor {};
      |        ^
repro.cc:10:8: note: in implicit destructor for 'Final' first required here
repro.cc:2:8: note: member found by ambiguous name lookup
    2 |   void operator delete[](void *);
      |        ^
repro.cc:5:8: note: member found by ambiguous name lookup
    5 |   void operator delete[](void *);
      |        ^

[Fznamznon]

Looking... Does not reproduce without c++20.

[Fznamznon]

I posted a PR with a potential fix.

[DKLoehr]

Thanks! Unfortunately it looks like the fix introduced a regression of #134265 on Windows. Repro:

struct Base {
  virtual ~Base();
  void operator delete[](void *) = delete;
};
class __declspec(dllexport) Derived : Base {};

Running clang-cl repro.cc /std:c++20 results in the following error after

repro.cc(5,29): error: attempt to use a deleted function
    5 | class __declspec(dllexport) Derived : Base {};
      |                             ^
repro.cc(5,29): note: in implicit destructor for 'Derived' first required here
repro.cc(5,18): note: due to 'Derived' being dllexported
    5 | class __declspec(dllexport) Derived : Base {};
      |                  ^
repro.cc(3,8): note: 'operator delete[]' has been explicitly marked deleted here
    3 |   void operator delete[](void *) = delete;
      |        ^

[Fznamznon]

Yeah, but this is kind of expected. Otherwise it is unclear how to emit vector deleting destructor body. Should we fallback to calling something, like MSVC does? See #135041 (comment) for example.

[DKLoehr]

I'm not familiar enough to say what we should be doing; perhaps @rnk or @zmodem have thoughts? It seems like we should at least emit a better error message, since just looking at the code doesn't make it clear to me why operator delete[] is relevant.

Since this is causing clang to become more restrictive, the best thing to do might be to revert to whatever we used to do in this situation, and then make a followup change which specifically explains why clang is becoming more restrictive.

[zmodem]

Thanks! Unfortunately it looks like the fix introduced a regression of #134265 on Windows. Repro:

(For reference, this is https://crbug.com/410000261 on our side.)

The "deleted operator delete[]" is coming from here: https://source.chromium.org/chromium/chromium/src/+/main:v8/include/cppgc/garbage-collected.h;l=61

Rejecting the code seems wrong, since that's only happening due to this extension. I'm not sure what the best option is here, but I think we should look closer at what exactly MSVC is emitting.

[Fznamznon]

I'm working on a revert of the extension.