Fix make_shared with throwing destructors

[StephanTLavavej]

Fixes #1733.

The STL is allowed to assume that user destructors don't emit exceptions (WG21-N4878 [tab:cpp17.destructible]), but user destructors can be marked noexcept(false) as long as they never emit exceptions when interacting with the STL. The bug being fixed here is that 9 out of 10 shared_ptr control blocks would fail to compile for noexcept(false) destructors. (The only unaffected control block was the simplest, _Ref_count, which stores only a pointer.)

We can mark ~_Ref_count_resource() and ~_Ref_count_resource_alloc() as noexcept and define them as = default; thanks to WG21-N4878 [dcl.fct.def.default]/2.2:

The type T₁ of an explicitly defaulted special member function F is allowed to differ from the type T₂ it would have had if it were implicitly declared, as follows: [...] T₁ and T₂ may have differing exception specifications

For the other destructors which were already being provided by the library, the fix is to mark them as noexcept. I'm additionally marking them as virtual override for consistency, and copying the // TRANSITION, should be non-virtual comment from the base class.

Because this interacts with virtual member functions, we need to think carefully about ABI. I manually verified that the vtable layout isn't changing;

C:\Temp>type meow.cpp

#include <memory>
using namespace std;

int main() {
    shared_ptr<int> sp{new int, default_delete<int>{}};
}

C:\Temp>cl /EHsc /nologo /W4 /std:c++latest /c /d1reportSingleClassLayout_Ref_count_resource meow.cpp
meow.cpp

Before:

class std::_Ref_count_resource<int *,struct std::default_delete<int> >  size(16):
        +---
 0      | +--- (base class std::_Ref_count_base)
 0      | | {vfptr}
 4      | | _Uses
 8      | | _Weaks
        | +---
12      | ?$_Compressed_pair@U?$default_delete@H@std@@PAH$00 _Mypair
        +---

std::_Ref_count_resource<int *,struct std::default_delete<int> >::$vftable@:
        | &?$_Ref_count_resource@PAHU?$default_delete@H@std@@_meta
        |  0
 0      | &std::_Ref_count_resource<int *,struct std::default_delete<int> >::_Destroy
 1      | &std::_Ref_count_resource<int *,struct std::default_delete<int> >::_Delete_this
 2      | &std::_Ref_count_resource<int *,struct std::default_delete<int> >::{dtor}
 3      | &std::_Ref_count_resource<int *,struct std::default_delete<int> >::_Get_deleter

std::_Ref_count_resource<int *,struct std::default_delete<int> >::_Get_deleter this adjustor: 0
std::_Ref_count_resource<int *,struct std::default_delete<int> >::_Destroy this adjustor: 0
std::_Ref_count_resource<int *,struct std::default_delete<int> >::_Delete_this this adjustor: 0
std::_Ref_count_resource<int *,struct std::default_delete<int> >::{dtor} this adjustor: 0
std::_Ref_count_resource<int *,struct std::default_delete<int> >::__delDtor this adjustor: 0
std::_Ref_count_resource<int *,struct std::default_delete<int> >::__vecDelDtor this adjustor: 0

After:

class std::_Ref_count_resource<int *,struct std::default_delete<int> >  size(16):
        +---
 0      | +--- (base class std::_Ref_count_base)
 0      | | {vfptr}
 4      | | _Uses
 8      | | _Weaks
        | +---
12      | ?$_Compressed_pair@U?$default_delete@H@std@@PAH$00 _Mypair
        +---

std::_Ref_count_resource<int *,struct std::default_delete<int> >::$vftable@:
        | &?$_Ref_count_resource@PAHU?$default_delete@H@std@@_meta
        |  0
 0      | &std::_Ref_count_resource<int *,struct std::default_delete<int> >::_Destroy
 1      | &std::_Ref_count_resource<int *,struct std::default_delete<int> >::_Delete_this
 2      | &std::_Ref_count_resource<int *,struct std::default_delete<int> >::{dtor}
 3      | &std::_Ref_count_resource<int *,struct std::default_delete<int> >::_Get_deleter

std::_Ref_count_resource<int *,struct std::default_delete<int> >::{dtor} this adjustor: 0
std::_Ref_count_resource<int *,struct std::default_delete<int> >::_Get_deleter this adjustor: 0
std::_Ref_count_resource<int *,struct std::default_delete<int> >::_Destroy this adjustor: 0
std::_Ref_count_resource<int *,struct std::default_delete<int> >::_Delete_this this adjustor: 0
std::_Ref_count_resource<int *,struct std::default_delete<int> >::__delDtor this adjustor: 0
std::_Ref_count_resource<int *,struct std::default_delete<int> >::__vecDelDtor this adjustor: 0

The virtual function table order is unchanged, and it follows the base class order (which, after thinking about it for a moment, is how virtual functions must work):

STL/stl/inc/memory

Lines 1046 to 1121 in c5ee3b3

	class __declspec(novtable) _Ref_count_base { // common code for reference counting
	private:
	#ifdef _M_CEE_PURE
	// permanent workaround to avoid mentioning _purecall in msvcurt.lib, ptrustu.lib, or other support libs
	virtual void _Destroy() noexcept {
	_STD terminate();
	}
	virtual void _Delete_this() noexcept {
	_STD terminate();
	}
	#else // ^^^ _M_CEE_PURE / !_M_CEE_PURE vvv
	virtual void _Destroy() noexcept = 0; // destroy managed resource
	virtual void _Delete_this() noexcept = 0; // destroy self
	#endif // _M_CEE_PURE
	_Atomic_counter_t _Uses = 1;
	_Atomic_counter_t _Weaks = 1;
	protected:
	constexpr _Ref_count_base() noexcept = default; // non-atomic initializations
	public:
	_Ref_count_base(const _Ref_count_base&) = delete;
	_Ref_count_base& operator=(const _Ref_count_base&) = delete;
	virtual ~_Ref_count_base() noexcept {} // TRANSITION, should be non-virtual
	bool _Incref_nz() noexcept { // increment use count if not zero, return true if successful
	auto& _Volatile_uses = reinterpret_cast<volatile long&>(_Uses);
	#ifdef _M_CEE_PURE
	long _Count = *_Atomic_address_as<const long>(&_Volatile_uses);
	#else
	long _Count = __iso_volatile_load32(reinterpret_cast<volatile int*>(&_Volatile_uses));
	#endif
	while (_Count != 0) {
	const long _Old_value = _INTRIN_RELAXED(_InterlockedCompareExchange)(&_Volatile_uses, _Count + 1, _Count);
	if (_Old_value == _Count) {
	return true;
	}
	_Count = _Old_value;
	}
	return false;
	}
	void _Incref() noexcept { // increment use count
	_MT_INCR(_Uses);
	}
	void _Incwref() noexcept { // increment weak reference count
	_MT_INCR(_Weaks);
	}
	void _Decref() noexcept { // decrement use count
	if (_MT_DECR(_Uses) == 0) {
	_Destroy();
	_Decwref();
	}
	}
	void _Decwref() noexcept { // decrement weak reference count
	if (_MT_DECR(_Weaks) == 0) {
	_Delete_this();
	}
	}
	long _Use_count() const noexcept {
	return static_cast<long>(_Uses);
	}
	virtual void* _Get_deleter(const type_info&) const noexcept {
	return nullptr;
	}
	};

The only difference is that /d1reportSingleClassLayout is reporting "this adjustors" in derived order, and I'm inserting the definitions of ~_Ref_count_resource() and ~_Ref_count_resource_alloc() near the top. As far as I know, this difference is purely cosmetic (it doesn't reflect any physical ordering).

The test contains comments indicating which control blocks are being tested; mostly for clarity, partially so that any future searches for control block names will find these tests.

Along the way, I reported (internally, because I was a lazy kitty) and worked around:

• VSO-1292292 "EDG mishandles trivial throwing destructors"
• VSO-1292293 "EDG incorrectly rejects defaulted noexcept dtor when a data member has a throwing dtor"

[miscco]

As said i do not like virtual override together but do not really care too much either

[MikeGitb]

Is there a (easy to suppress) way to warn about it? If I go all the way to mark a destructor as noexcept(false) I'm pretty sure I actually expect it to fail at some point or another and a warning, that this configuration is not supported by the STL would be nice.

[StephanTLavavej]

@MikeGitb

Is there a (easy to suppress) way to warn about it?

That's a reasonable question - the answer is "unfortunately no". The STL doesn't have great mechanisms to emit such warnings, or to allow users to suppress them. The main mechanism available to us is [[deprecated]], which we reserve for only ISO and Microsoft deprecations, and the suppression system involves verbose macros (where the verbosity is somewhat of an advantage). Warning about conformant, non-deprecated code is a big step, and we need to be cautious to avoid emitting annoying false positives. [[nodiscard]] is an example where we do emit warnings far beyond what the Standard suggests, but we are extremely careful to not emit false positives. (We have #pragma message for file-level problems, like including C++20 headers in C++14 mode, but those don't activate /WX.) When we've previously warned about conformant code that wasn't inherently flawed, it really annoyed users (that was warning about using raw pointers as output iterators).

In this case, although you would like to be warned, other users might not, and they might not enjoy having to suppress a new warning when upgrading. The last problem I can think of is there's no good way to suppress such a warning in a targeted manner - to say "okay, I know that ~Meow() noexcept(false) shouldn't emit exceptions at runtime, mark it as okay but continue to warn for other types". This leads me to think that we shouldn't attempt to implement such a warning.