From 6ed299f724789f57669b5b367e4d6df1259166b2 Mon Sep 17 00:00:00 2001 From: Giuseppe D'Angelo Date: Mon, 26 Dec 2022 01:00:45 +0100 Subject: [PATCH] QMetaType: fix value-initialization in a corner case If a type is trivially default constructible, QMetaType (and QVariant) think that it can be built and value-initialized by zero-filling a region of storage and then "blessing" that storage as an actual instance of the type to build. This is done as an optimization. This doesn't work for all trivially constructible types. For instance, on the Itanium C++ ABI, pointers to data members are actually value-initialized (= zero-initialized, = initialized to null) with the value -1: https://itanium-cxx-abi.github.io/cxx-abi/abi.html#data-member-pointers This means that a type like struct A { int A::*ptr; }; is trivially constructible, but its value initialization is not equivalent to zero-filling its storage. Since C++ does not offer a type trait we can use for the detection that we want to do here, and since we have also decided that Q_PRIMITIVE_TYPE isn't that trait (it just means trivially copyable / destructible), I'm rolling out a custom type trait for the purpose. This type trait is private for the moment being (there's no Q_DECLARE_TYPEINFO for it), and limited to the subset of scalar types that we know can be value-initialized by memset(0) into their storage (basically, all of them, except for pointers to data members). The fix tries to keep the pre-existing semantics of `QMetaType::NeedsConstruction`. Before, the flag was set for types which were not trivially default constructible. That included types that aren't default constructible, or types that cannot do so trivially. I've left that meaning unchanged, and simply amended the "trivial" part with the custom trait. A fix there (to clarify the semantics) can be done as a separate change. Change-Id: Id8da6acb913df83fc87e5d37e2349a4628e72e91 Fixes: QTBUG-109594 Reviewed-by: Thiago Macieira (cherry picked from commit e8f5f20319ce5f75180c531b6e26e648ce55619c) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/global/qtypeinfo.h | 15 ++ src/corelib/kernel/qmetatype.cpp | 2 +- src/corelib/kernel/qmetatype.h | 4 +- src/corelib/kernel/qvariant.cpp | 2 +- .../kernel/qmetatype/tst_qmetatype.cpp | 128 +++++++++++++++++- .../corelib/kernel/qmetatype/tst_qmetatype.h | 1 + 6 files changed, 147 insertions(+), 5 deletions(-) diff --git a/src/corelib/global/qtypeinfo.h b/src/corelib/global/qtypeinfo.h index 73bf6140c7a..4602257ff5f 100644 --- a/src/corelib/global/qtypeinfo.h +++ b/src/corelib/global/qtypeinfo.h @@ -22,6 +22,16 @@ class QDebug; template inline constexpr bool qIsRelocatable = std::is_trivially_copyable_v && std::is_trivially_destructible_v; +// Denotes types that are trivially default constructible, and for which +// value-initialization can be achieved by filling their storage with 0 bits. +// There is no type trait we can use for this, so we hardcode a list of +// possibilities that we know are OK on the architectures that we support. +// The most notable exception are pointers to data members, which for instance +// on the Itanium ABI are initialized to -1. +template +inline constexpr bool qIsValueInitializationBitwiseZero = + std::is_scalar_v && !std::is_member_object_pointer_v; + /* The catch-all template. */ @@ -35,6 +45,7 @@ public: isIntegral = std::is_integral_v, isComplex = !std::is_trivial_v, isRelocatable = qIsRelocatable, + isValueInitializationBitwiseZero = qIsValueInitializationBitwiseZero, }; }; @@ -47,6 +58,7 @@ public: isIntegral = false, isComplex = false, isRelocatable = false, + isValueInitializationBitwiseZero = false, }; }; @@ -79,6 +91,7 @@ public: static constexpr bool isRelocatable = ((QTypeInfo::isRelocatable) && ...); static constexpr bool isPointer = false; static constexpr bool isIntegral = false; + static constexpr bool isValueInitializationBitwiseZero = false; }; #define Q_DECLARE_MOVABLE_CONTAINER(CONTAINER) \ @@ -91,6 +104,7 @@ public: \ isIntegral = false, \ isComplex = true, \ isRelocatable = true, \ + isValueInitializationBitwiseZero = false, \ }; \ } @@ -131,6 +145,7 @@ public: \ isRelocatable = !isComplex || ((FLAGS) & Q_RELOCATABLE_TYPE) || qIsRelocatable, \ isPointer = false, \ isIntegral = std::is_integral< TYPE >::value, \ + isValueInitializationBitwiseZero = qIsValueInitializationBitwiseZero, \ }; \ } diff --git a/src/corelib/kernel/qmetatype.cpp b/src/corelib/kernel/qmetatype.cpp index 81647eb5dc5..3606b298dd1 100644 --- a/src/corelib/kernel/qmetatype.cpp +++ b/src/corelib/kernel/qmetatype.cpp @@ -433,7 +433,7 @@ const char *QtMetaTypePrivate::typedefNameForType(const QtPrivate::QMetaTypeInte The enum describes attributes of a type supported by QMetaType. - \value NeedsConstruction This type has a non-trivial default constructor. If the flag is not set, instances can be safely initialized with memset to 0. + \value NeedsConstruction This type has a default constructor. If the flag is not set, instances can be safely initialized with memset to 0. \value NeedsCopyConstruction (since 6.5) This type has a non-trivial copy construtcor. If the flag is not set, instances can be copied with memcpy. \value NeedsMoveConstruction (since 6.5) This type has a non-trivial move constructor. If the flag is not set, instances can be moved with memcpy. \value NeedsDestruction This type has a non-trivial destructor. If the flag is not set calls to the destructor are not necessary before discarding objects. diff --git a/src/corelib/kernel/qmetatype.h b/src/corelib/kernel/qmetatype.h index 4c92a52f9e7..97e0eae7f97 100644 --- a/src/corelib/kernel/qmetatype.h +++ b/src/corelib/kernel/qmetatype.h @@ -1251,7 +1251,7 @@ namespace QtPrivate { struct QMetaTypeTypeFlags { enum { Flags = (QTypeInfo::isRelocatable ? QMetaType::RelocatableType : 0) - | (!std::is_trivially_default_constructible_v ? QMetaType::NeedsConstruction : 0) + | ((!std::is_default_constructible_v || !QTypeInfo::isValueInitializationBitwiseZero) ? QMetaType::NeedsConstruction : 0) | (!std::is_trivially_destructible_v ? QMetaType::NeedsDestruction : 0) | (!std::is_trivially_copy_constructible_v ? QMetaType::NeedsCopyConstruction : 0) | (!std::is_trivially_move_constructible_v ? QMetaType::NeedsMoveConstruction : 0) @@ -2394,7 +2394,7 @@ public: static constexpr QMetaTypeInterface::DefaultCtrFn getDefaultCtr() { - if constexpr (std::is_default_constructible_v && !std::is_trivially_default_constructible_v) { + if constexpr (std::is_default_constructible_v && !QTypeInfo::isValueInitializationBitwiseZero) { return [](const QMetaTypeInterface *, void *addr) { new (addr) S(); }; } else { return nullptr; diff --git a/src/corelib/kernel/qvariant.cpp b/src/corelib/kernel/qvariant.cpp index 75bdef2383c..ddb0ccf75dd 100644 --- a/src/corelib/kernel/qvariant.cpp +++ b/src/corelib/kernel/qvariant.cpp @@ -266,7 +266,7 @@ static void customConstruct(const QtPrivate::QMetaTypeInterface *iface, QVariant if (QVariant::Private::canUseInternalSpace(iface)) { d->is_shared = false; if (!copy && !iface->defaultCtr) - return; // trivial default constructor, we've already memset + return; // default constructor and it's OK to build in 0-filled storage, which we've already done construct(iface, d->data.data, copy); } else { d->data.shared = customConstructShared(iface->size, iface->alignment, [=](void *where) { diff --git a/tests/auto/corelib/kernel/qmetatype/tst_qmetatype.cpp b/tests/auto/corelib/kernel/qmetatype/tst_qmetatype.cpp index b611fb146c0..f0efff34365 100644 --- a/tests/auto/corelib/kernel/qmetatype/tst_qmetatype.cpp +++ b/tests/auto/corelib/kernel/qmetatype/tst_qmetatype.cpp @@ -1025,7 +1025,7 @@ template void addFlagsRow(const char *name, int id = qMetaTypeId QTest::newRow(name) << id << bool(QTypeInfo::isRelocatable) - << bool(!std::is_trivially_default_constructible_v) + << bool(!std::is_default_constructible_v || !QTypeInfo::isValueInitializationBitwiseZero) << bool(!std::is_trivially_copy_constructible_v) << bool(!std::is_trivially_destructible_v) << bool(QtPrivate::IsPointerToTypeDerivedFromQObject::Value) @@ -1322,6 +1322,132 @@ FOR_EACH_CORE_METATYPE(RETURN_CONSTRUCT_FUNCTION) TypeTestFunctionGetter::get(type)(); } + +namespace TriviallyConstructibleTests { + +enum Enum0 {}; +enum class Enum1 {}; + +static_assert(QTypeInfo::isValueInitializationBitwiseZero); +static_assert(QTypeInfo::isValueInitializationBitwiseZero); +static_assert(QTypeInfo::isValueInitializationBitwiseZero); +static_assert(QTypeInfo::isValueInitializationBitwiseZero); +static_assert(QTypeInfo::isValueInitializationBitwiseZero); +static_assert(QTypeInfo::isValueInitializationBitwiseZero); +static_assert(QTypeInfo::isValueInitializationBitwiseZero); + +struct A {}; +struct B { B() {} }; +struct C { ~C() {} }; +struct D { D(int) {} }; +struct E { E() {} ~E() {} }; +struct F { int i; }; +struct G { G() : i(0) {} int i; }; +struct H { constexpr H() : i(0) {} int i; }; +struct I { I() : i(42) {} int i; }; +struct J { constexpr J() : i(42) {} int i; }; +struct K { K() : i(0) {} ~K() {} int i; }; + +static_assert(!QTypeInfo::isValueInitializationBitwiseZero); +static_assert(!QTypeInfo::isValueInitializationBitwiseZero); +static_assert(!QTypeInfo::isValueInitializationBitwiseZero); +static_assert(!QTypeInfo::isValueInitializationBitwiseZero); +static_assert(!QTypeInfo::isValueInitializationBitwiseZero); +static_assert(!QTypeInfo::isValueInitializationBitwiseZero); +static_assert(!QTypeInfo::isValueInitializationBitwiseZero); +static_assert(!QTypeInfo::isValueInitializationBitwiseZero); +static_assert(!QTypeInfo::isValueInitializationBitwiseZero); +static_assert(!QTypeInfo::isValueInitializationBitwiseZero); +static_assert(!QTypeInfo::isValueInitializationBitwiseZero); + +} // namespace TriviallyConstructibleTests + +// Value-initializing these trivially constructible types cannot be achieved by +// memset(0) into their storage. For instance, on Itanium, a pointer to a data +// member needs to be value-initialized by setting it to -1. + +// Fits into QVariant +struct TrivialTypeNotZeroInitableSmall { + int TrivialTypeNotZeroInitableSmall::*pdm; +}; + +static_assert(std::is_trivially_default_constructible_v); +static_assert(!QTypeInfo::isValueInitializationBitwiseZero); +static_assert(sizeof(TrivialTypeNotZeroInitableSmall) < sizeof(QVariant)); // also checked more thoroughly below + +// Does not fit into QVariant internal storage +struct TrivialTypeNotZeroInitableBig { + int a; + double b; + char c; + int array[42]; + void (TrivialTypeNotZeroInitableBig::*pmf)(); + int TrivialTypeNotZeroInitableBig::*pdm; +}; + +static_assert(std::is_trivially_default_constructible_v); +static_assert(!QTypeInfo::isValueInitializationBitwiseZero); +static_assert(sizeof(TrivialTypeNotZeroInitableBig) > sizeof(QVariant)); // also checked more thoroughly below + +void tst_QMetaType::defaultConstructTrivial_QTBUG_109594() +{ + // MSVC miscompiles value-initialization of pointers to data members, + // https://developercommunity.visualstudio.com/t/Pointer-to-data-member-is-not-initialize/10238905 + { + QMetaType mt = QMetaType::fromType(); + QVERIFY(mt.isDefaultConstructible()); + auto ptr = static_cast(mt.create()); + const auto cleanup = qScopeGuard([=] { + mt.destroy(ptr); + }); +#ifdef Q_CC_MSVC_ONLY + QEXPECT_FAIL("", "MSVC compiler bug", Continue); +#endif + QCOMPARE(ptr->pdm, nullptr); + + QVariant v(mt); + QVERIFY(QVariant::Private::canUseInternalSpace(mt.iface())); + auto obj = v.value(); +#ifdef Q_CC_MSVC_ONLY + QEXPECT_FAIL("", "MSVC compiler bug", Continue); +#endif + QCOMPARE(obj.pdm, nullptr); + } + + { + QMetaType mt = QMetaType::fromType(); + QVERIFY(mt.isDefaultConstructible()); + auto ptr = static_cast(mt.create()); + const auto cleanup = qScopeGuard([=] { + mt.destroy(ptr); + }); + QCOMPARE(ptr->a, 0); + QCOMPARE(ptr->b, 0.0); + QCOMPARE(ptr->c, '\0'); + QCOMPARE(ptr->pmf, nullptr); + for (int i : ptr->array) + QCOMPARE(i, 0); +#ifdef Q_CC_MSVC_ONLY + QEXPECT_FAIL("", "MSVC compiler bug", Continue); +#endif + QCOMPARE(ptr->pdm, nullptr); + + QVariant v(mt); + QVERIFY(!QVariant::Private::canUseInternalSpace(mt.iface())); + auto obj = v.value(); + QCOMPARE(obj.a, 0); + QCOMPARE(obj.b, 0.0); + QCOMPARE(obj.c, '\0'); + QCOMPARE(obj.pmf, nullptr); + for (int i : obj.array) + QCOMPARE(i, 0); +#ifdef Q_CC_MSVC_ONLY + QEXPECT_FAIL("", "MSVC compiler bug", Continue); +#endif + QCOMPARE(obj.pdm, nullptr); + } +} + void tst_QMetaType::typedConstruct() { auto testMetaObjectWriteOnGadget = [](QVariant &gadget, const QList &properties) diff --git a/tests/auto/corelib/kernel/qmetatype/tst_qmetatype.h b/tests/auto/corelib/kernel/qmetatype/tst_qmetatype.h index 77feff806fc..75bb3635492 100644 --- a/tests/auto/corelib/kernel/qmetatype/tst_qmetatype.h +++ b/tests/auto/corelib/kernel/qmetatype/tst_qmetatype.h @@ -77,6 +77,7 @@ private slots: void flagsBinaryCompatibility6_0(); void construct_data(); void construct(); + void defaultConstructTrivial_QTBUG_109594(); void typedConstruct(); void constructCopy_data(); void constructCopy();