Vulkan-hpp is reinterpret_casting non-standard-layout class to another class. Is this legal?
Asked Answered
S

1

7

So recently I have been working with Vulkan-Hpp (The official c++ bindings of Vulkan Api, Github Link).

Looking into the source, I have found that they create wrapper classes around native Vulkan structs (e.g. vk::InstanceCreateInfo wraps around VkInstanceCreateInfo). (Note: Wrapping around, not deriving from)

When calling native Vulkan API, the pointers to wrapper classes are reinterpret_cast ed into the native Vulkan structs. An example using vk::InstanceCreateInfo:

//definition of vk::InstanceCreateInfo
struct InstanceCreateInfo
{
    /*  member function omitted  */
private:
  StructureType sType = StructureType::eInstanceCreateInfo;
public:
  const void* pNext = nullptr;
  InstanceCreateFlags flags;
  const ApplicationInfo* pApplicationInfo;
  uint32_t enabledLayerCount;
  const char* const* ppEnabledLayerNames;
  uint32_t enabledExtensionCount;
  const char* const* ppEnabledExtensionNames;
};

//definition of VkInstanceCreateInfo
typedef struct VkInstanceCreateInfo {
    VkStructureType             sType;
    const void*                 pNext;
    VkInstanceCreateFlags       flags;
    const VkApplicationInfo*    pApplicationInfo;
    uint32_t                    enabledLayerCount;
    const char* const*          ppEnabledLayerNames;
    uint32_t                    enabledExtensionCount;
    const char* const*          ppEnabledExtensionNames;
} VkInstanceCreateInfo;

//And the usage where reinterpret_cast takes place
template<typename Dispatch>
VULKAN_HPP_INLINE ResultValueType<Instance>::type createInstance( const InstanceCreateInfo &createInfo, Optional<const AllocationCallbacks> allocator, Dispatch const &d )
{
  Instance instance;
  Result result = static_cast<Result>( d.vkCreateInstance( reinterpret_cast<const VkInstanceCreateInfo*>( &createInfo ), reinterpret_cast<const VkAllocationCallbacks*>( static_cast<const AllocationCallbacks*>( allocator ) ), reinterpret_cast<VkInstance*>( &instance ) ) );
  return createResultValue( result, instance, VULKAN_HPP_NAMESPACE_STRING"::createInstance" );
}

So my question is: vk::InstanceCreateInfo and VkInstanceCreateInfo are two different types. Moreover, VkInstanceCreateInfo is standard layout but vk::InstanceCreateInfo is not (since it has mixed access specifiers). Is reinterpret_casting between the pointer of those two types (as done by Vulkan-Hpp) legal? Does this violate the strict aliasing rule?


Note: you can assume that VkInstanceCreateFlags and vk::InstanceCreateFlags are interchangeable in this case (otherwise it would make my question recursive)

Shout answered 11/7, 2019 at 9:21 Comment(8)
Presumably it does. The private member breaks the layout.Groundage
@L.F. can you elaborate how private member breaks the layout?Furst
@MarekR I remember reading somewhere that this has something to do with standard layout, but I am not sure either ..Groundage
@MarekR Actually this kind of reinterpret_cast are everywhere around the library (I've checked the macros, and verified that it is not a compiler specific feature). And about access specifiers, All non-static data members have the same access control in order for a class to be standard layout.Shout
@L.F. And I do remember that the standard only allow pointer aliasing between same effective types (which simply means type when the type is declared). So this means even those two types are both standard layout types and have exact same members, type punning is not allowed. Maybe the correct way is to use inheritance? Feel free to correct me if I am wrong.Shout
@KaenbyouRin Even if this code is technically valid, it should be redesigned anyway ...Groundage
The Type aliasing rule is pretty clear here - this is undefined behavior.Hagerty
@Hagerty post this as an answer.Furst
I
6

As far as the standard is concerned, yes, accessing a vk::InstanceCreateInfo object through a pointer to a VkInstanceCreateInfo object violates strict aliasing. And it would still violate strict aliasing even if both types were standard layout with equivalent layout. The standard doesn't allow you to just pretend that one class type is another, even if they have equivalent layouts.

So this code is relying on some implementation-specific behavior.

Is that wrong? Would doing an additional copy for every single Vulkan interface function call make you feel better about it, even if it would work without it? It's ultimately up to you how much UB you feel like tolerating.

Ingeingeberg answered 11/7, 2019 at 13:29 Comment(5)
They should make C++ light with less UB added but more sugar substitutes.Upcountry
I believe they can derive their wrapper classes from the native ones, since pointer casting from derived to base is 100% well defined behavior.Shout
@KaenbyouRin: The whole point of making that field private is so that users cannot accidentally change it. Deriving from the native type would work against that purpose.Ingeingeberg
@NicolBolas This defeats the purpose of course, but I believe automating the initialization of that field should be enough (you don't fill in the wrong value). Any sane implementation won't try to modify that value after initialization anyway. I can see another approach that we derive protectedly or privately from the base, then we write access functions for everything except the sType field. This would make things really complicated though...Shout
@NicolBolas To correct the mistake from the last comment: we should use public inheritance and do additional using access declaration to make the field private.Shout

© 2022 - 2024 — McMap. All rights reserved.