Browse Source

Vk: properly test std::lower_bound() corner cases.

Like, no wonder C++ codebases are so bug-ridden, given all APIs in the
standard library are weird like this, requiring the user to completely
understand their inner workings in order to be able to use them safely.

A fix for this got made in 390d3fc06f
(2022) already, but somehow the corresponding tests were living in a
half-done stash ever since. I should probably also just do my own binary
search thing so I don't need to test everything so extensively.
pull/680/head
Vladimír Vondruš 10 months ago
parent
commit
07e4047aee
  1. 4
      src/Magnum/Vk/Device.cpp
  2. 4
      src/Magnum/Vk/Instance.cpp
  3. 41
      src/Magnum/Vk/Test/DeviceVkTest.cpp
  4. 34
      src/Magnum/Vk/Test/InstanceVkTest.cpp

4
src/Magnum/Vk/Device.cpp

@ -846,6 +846,10 @@ void Device::initialize(Instance& instance, const Version version, const Contain
const auto found = std::lower_bound(knownExtensions.begin(), knownExtensions.end(), extension, [](const Extension& a, Containers::StringView b) { const auto found = std::lower_bound(knownExtensions.begin(), knownExtensions.end(), extension, [](const Extension& a, Containers::StringView b) {
return a.string() < b; return a.string() < b;
}); });
/* Thanks, C++, for forcing me to have a larger bug surface instead
of providing a library helper to find the damn thing. See
DeviceVkTest::wrapExtensionNotFound() for verification of both
cases. */
if(found == knownExtensions.end() || found->string() != extension) continue; if(found == knownExtensions.end() || found->string() != extension) continue;
_enabledExtensions.set(found->index(), true); _enabledExtensions.set(found->index(), true);
} }

4
src/Magnum/Vk/Instance.cpp

@ -346,6 +346,10 @@ void Instance::initialize(const Version version, const Containers::StringIterabl
const auto found = std::lower_bound(knownExtensions.begin(), knownExtensions.end(), extension, [](const InstanceExtension& a, Containers::StringView b) { const auto found = std::lower_bound(knownExtensions.begin(), knownExtensions.end(), extension, [](const InstanceExtension& a, Containers::StringView b) {
return a.string() < b; return a.string() < b;
}); });
/* Thanks, C++, for forcing me to have a larger bug surface instead
of providing a library helper to find the damn thing. See
InstanceVkTest::wrapExtensionNotFound() for verification of both
cases. */
if(found == knownExtensions.end() || found->string() != extension) continue; if(found == knownExtensions.end() || found->string() != extension) continue;
_extensionStatus.set(found->index(), true); _extensionStatus.set(found->index(), true);
} }

41
src/Magnum/Vk/Test/DeviceVkTest.cpp

@ -95,6 +95,7 @@ struct DeviceVkTest: VulkanTester {
void tryCreateUnknownExtension(); void tryCreateUnknownExtension();
void wrap(); void wrap();
void wrapExtensionNotFound();
void wrapAlreadyCreated(); void wrapAlreadyCreated();
void populateGlobalFunctionPointers(); void populateGlobalFunctionPointers();
@ -233,6 +234,7 @@ DeviceVkTest::DeviceVkTest(): VulkanTester{NoCreate} {
&DeviceVkTest::tryCreateUnknownExtension, &DeviceVkTest::tryCreateUnknownExtension,
&DeviceVkTest::wrap, &DeviceVkTest::wrap,
&DeviceVkTest::wrapExtensionNotFound,
&DeviceVkTest::wrapAlreadyCreated, &DeviceVkTest::wrapAlreadyCreated,
&DeviceVkTest::populateGlobalFunctionPointers}); &DeviceVkTest::populateGlobalFunctionPointers});
@ -1189,6 +1191,45 @@ void DeviceVkTest::wrap() {
wrapped->DestroyDevice(device, nullptr); wrapped->DestroyDevice(device, nullptr);
} }
void DeviceVkTest::wrapExtensionNotFound() {
if(std::getenv("MAGNUM_DISABLE_EXTENSIONS"))
CORRADE_SKIP("Can't test with the MAGNUM_DISABLE_EXTENSIONS environment variable set");
/* Creating a dedicated instance so we can enable device extensions
independently */
Instance instance2;
DeviceProperties deviceProperties = pickDevice(instance2);
ExtensionProperties extensions = deviceProperties.enumerateExtensionProperties({});
if(!extensions.isSupported<Extensions::KHR::bind_memory2>())
CORRADE_SKIP(Extensions::KHR::bind_memory2::string() << "not supported, can't test");
Queue queue{NoCreate};
Device device{instance2, DeviceCreateInfo{deviceProperties}
.addQueues(0, {0.0f}, {queue})
.addEnabledExtensions<Extensions::KHR::bind_memory2>()
};
/* Verify that we don't dereference garbage when std::lower_bound() ... */
Device wrapped{NoCreate};
wrapped.wrap(instance2, deviceProperties, device, device.version(), {
/* ... returns `last` for an unknown extension, */
"VK_ZZZ_this_does_not_exist",
Extensions::KHR::bind_memory2::string(),
/* ... or returns an extension that isn't actually the one we're
looking for, in this case being right before VK_KHR_maintenance1
because of this here
-----------------v */
"VK_KHR_maintenancd1",
}, {}, {});
/* This one shouldn't be listed */
CORRADE_VERIFY(!wrapped.isExtensionEnabled<Extensions::KHR::maintenance1>());
/* The valid extension should be */
CORRADE_VERIFY(wrapped.isExtensionEnabled<Extensions::KHR::bind_memory2>());
}
void DeviceVkTest::wrapAlreadyCreated() { void DeviceVkTest::wrapAlreadyCreated() {
CORRADE_SKIP_IF_NO_ASSERT(); CORRADE_SKIP_IF_NO_ASSERT();

34
src/Magnum/Vk/Test/InstanceVkTest.cpp

@ -64,6 +64,7 @@ struct InstanceVkTest: TestSuite::Tester {
void tryCreateUnknownExtension(); void tryCreateUnknownExtension();
void wrap(); void wrap();
void wrapExtensionNotFound();
void wrapAlreadyCreated(); void wrapAlreadyCreated();
void populateGlobalFunctionPointers(); void populateGlobalFunctionPointers();
@ -172,6 +173,7 @@ InstanceVkTest::InstanceVkTest() {
&InstanceVkTest::tryCreateUnknownExtension, &InstanceVkTest::tryCreateUnknownExtension,
&InstanceVkTest::wrap, &InstanceVkTest::wrap,
&InstanceVkTest::wrapExtensionNotFound,
&InstanceVkTest::wrapAlreadyCreated, &InstanceVkTest::wrapAlreadyCreated,
&InstanceVkTest::populateGlobalFunctionPointers}); &InstanceVkTest::populateGlobalFunctionPointers});
@ -526,6 +528,38 @@ void InstanceVkTest::wrap() {
wrapped->DestroyInstance(instance, nullptr); wrapped->DestroyInstance(instance, nullptr);
} }
void InstanceVkTest::wrapExtensionNotFound() {
if(std::getenv("MAGNUM_DISABLE_EXTENSIONS"))
CORRADE_SKIP("Can't test with the MAGNUM_DISABLE_EXTENSIONS environment variable set");
InstanceExtensionProperties properties = enumerateInstanceExtensionProperties();
if(!properties.isSupported<Extensions::EXT::debug_report>())
CORRADE_SKIP(Extensions::EXT::debug_report::string() << "not supported, can't test");
Instance instance{InstanceCreateInfo{}
.addEnabledExtensions<Extensions::EXT::debug_report>()
};
/* Verify that we don't dereference garbage when std::lower_bound() ... */
Instance wrapped{NoCreate};
wrapped.wrap(instance, Version::Vk11, {
/* ... returns `last` for an unknown extension, */
"VK_ZZZ_this_does_not_exist",
Extensions::EXT::debug_report::string(),
/* ... or returns an extension that isn't actually the one we're
looking for, in this case being right before
VK_KHR_get_physical_device_properties2 because of this here
------------------------------------v */
"VK_KHR_get_physical_device_propertier2",
}, {});
/* This one shouldn't be listed */
CORRADE_VERIFY(!wrapped.isExtensionEnabled<Extensions::KHR::get_physical_device_properties2>());
/* The valid extension should be */
CORRADE_VERIFY(wrapped.isExtensionEnabled<Extensions::EXT::debug_report>());
}
void InstanceVkTest::wrapAlreadyCreated() { void InstanceVkTest::wrapAlreadyCreated() {
CORRADE_SKIP_IF_NO_ASSERT(); CORRADE_SKIP_IF_NO_ASSERT();

Loading…
Cancel
Save