Strict aliasing rules violation

[th3or14]

I'm compiling with GCC.

When compiling with -Wstrict-aliasing=1 warnings level enabled and at least -O2 optimizations level, I see multiple warnings like warning: dereferencing type-punned pointer might break strict-aliasing rules. For example, for the line

pNext_chain.push_back(reinterpret_cast<VkBaseOutStructure*>(&messengerCreateInfo));

in VkBootstrap.cpp. I looked closely and confirmed that accessing data through VkBaseOutStructure pointers may be dangerous when data has a type other than VkBaseOutStructure.

I wrote a reproducer for the undefined behavior. Something like the first 3 lines of code is happening in VkBootstrap.cpp, next I'm attempting to abuse the strict aliasing violation.

std::vector<VkBaseOutStructure*> chain;
VkDebugUtilsMessengerCreateInfoEXT info = {};
chain.push_back(reinterpret_cast<VkBaseOutStructure*>(&info));

// set pNext to an arbitrary value 1
info.pNext = reinterpret_cast<const void *>(1);
// nullify the same pNext using a pointer to VkBaseOutStructure
chain[0]->pNext = nullptr;

printf("Value of pNext obtained through info: %p.\n", info.pNext);
printf("Value of pNext obtained through chain: %p.\n", chain[0]->pNext);
bool areAddressesEqual = reinterpret_cast<const void *>(&info.pNext) == reinterpret_cast<const void *>(&chain[0]->pNext);
printf("Are pNext addresses physically equal in memory? %s.\n", (areAddressesEqual ? "Yes" : "No"));

Compiling and running with at least -O2 optimizations level gives the output

Value of pNext obtained through info: 0x1.
Value of pNext obtained through chain: (nil).
Are pNext addresses physically equal in memory? Yes.

because accessing data through chain[0]->pNext is undefined behavior here. Even though pNext addresses are physically equal in memory.

Adding -fno-strict-aliasing to the compilation flags changes the output to

Value of pNext obtained through info: (nil).
Value of pNext obtained through chain: (nil).
Are pNext addresses physically equal in memory? Yes.

because now the compiler knows that the strict aliasing rules may be violated and doesn't apply related optimizations.

Should I add -fno-strict-aliasing to CMakeLists.txt? I also see that, for example, Vulkan-Samples https://github.com/KhronosGroup/Vulkan-Samples/blob/main/CMakeLists.txt uses it:

# globally set -fno-strict-aliasing, needed due to using reinterpret_cast
if (NOT MSVC)
  add_compile_options(-fno-strict-aliasing)
endif()

And this is KhronosGroup/Vulkan-Samples#1207 why it was introduced.

[th3or14]

Another way could be an attempt to stop violating the strict aliasing rules instead of adding -fno-strict-aliasing to CMakeLists.txt.

For example, std::vector<VkBaseOutStructure*> could be replaced with std::vector<ChainElement>, where ChainElement is

struct ChainElement {
    const void *beginPtr;
    const void **pNextPtr;
    VkStructureType sType;
};

beginPtr is a pointer to the very VkBaseOutStructure-like structure. It may be needed inside setup_pNext_chain (needs to be rewritten) in VkBootstrap.cpp. pNextPtr is a pointer to pNext. And sType is for assertions (would need to change node->sType to node.sType) in VkBootstrap.cpp that at the moment look like

#if !defined(NDEBUG)
    for (auto& node : pNext_chain) {
        assert(node->sType != VK_STRUCTURE_TYPE_APPLICATION_INFO);
    }
#endif

So the undefined behavior reproducer may be rewritten to

    std::vector<ChainElement> chain;
    VkDebugUtilsMessengerCreateInfoEXT info = {};
    ChainElement chainElement;
    chainElement.beginPtr = &info;
    chainElement.pNextPtr = &info.pNext;
    chainElement.sType = info.sType;
    chain.push_back(chainElement);

    // set pNext to an arbitrary value 1
    info.pNext = reinterpret_cast<const void *>(1);
    // nullify the same pNext using the address at pNextPtr
    *chain[0].pNextPtr = nullptr;

    printf("Value of pNext obtained through info: %p.\n", info.pNext);
    printf("Value of pNext obtained through chain: %p.\n", *chain[0].pNextPtr);
    bool areAddressesEqual = reinterpret_cast<const void *>(&info.pNext) == reinterpret_cast<const void *>(chain[0].pNextPtr);
    printf("Are pNext addresses physically equal in memory? %s.\n", (areAddressesEqual ? "Yes" : "No"));

And then the output becomes

Value of pNext obtained through info: (nil).
Value of pNext obtained through chain: (nil).
Are pNext addresses physically equal in memory? Yes.

with -O2 and without -fno-strict-aliasing. No more undefined behavior. This is a solution for cases like

pNext_chain.push_back(reinterpret_cast<VkBaseOutStructure*>(&messengerCreateInfo));

in VkBootstrap.cpp.

If this way becomes considered more preferable than adding -fno-strict-aliasing to CMakeLists.txt, I may attempt to implement.

[charles-lunarg]

I know about this issue, and is essentially a duplicate of #295 (but wont close since it was found using different means).

I do have a branch thats WIP which will fix this https://github.com/charles-lunarg/vk-bootstrap/tree/generate_feature_struct_comparisons but its still not done since its a non-trivial pile of code that isn't easy to do piece meal.

[th3or14]

But this issue is not related to inconsistent presentation of data in memory between structures, as it is described in #295. In my rerproducer pNext addresses are same when obtained in different ways, I even added a check for that. pNext addresses would be different if padding was the issue.

To be clear, this is the issue. https://gist.github.com/shafik/848ae25ee209f698763cffee272a58f8

I haven't tested if anything unexpected happens in the current code regarding this issue, but I see dangerous accesses in at least setup_pNext_chain

template <typename T> void setup_pNext_chain(T& structure, std::vector<VkBaseOutStructure*> const& structs) {
    structure.pNext = nullptr;
    if (structs.size() <= 0) return;
    for (size_t i = 0; i < structs.size() - 1; i++) {
        structs.at(i)->pNext = structs.at(i + 1);
    }
    structure.pNext = structs.at(0);
}

There are accesses to pNext through a pointer to VkBaseOutStructure. And my reproducer successfully abused the undefined behavior when accessing pNext through a pointer to VkBaseOutStructure when compiled with -O2 with GCC.

[charles-lunarg]

Ah you are right, the issue you raise is distinct. I want to say that the PR will fix both this issue and the other issue. Put another way, since the original GenericFeatureStruct PR went in, I have learned about how to handle pNext chains in a UB safe fasion and will employ that in the PR. I will try to remember to add -fno-strict-aliasing to the code to validate the PR before. Of course I'd appreciate your review when that lands.

[th3or14]

Thank you, please ping me when it's ready for a review. Just in case, I attach the full list of related warnings I got when compiled with -O2 -Wstrict-aliasing=1 GCC compiler flags.

../vk-bootstrap/src/VkBootstrap.cpp: In member function ‘vkb::Result<vkb::Instance> vkb::InstanceBuilder::build() const’:
../vk-bootstrap/src/VkBootstrap.cpp:721:69: warning: dereferencing type-punned pointer might break strict-aliasing rules [-Wstrict-aliasing]
  721 |         pNext_chain.push_back(reinterpret_cast<VkBaseOutStructure*>(&messengerCreateInfo));
      |                                                                     ^~~~~~~~~~~~~~~~~~~~
../vk-bootstrap/src/VkBootstrap.cpp:732:69: warning: dereferencing type-punned pointer might break strict-aliasing rules [-Wstrict-aliasing]
  732 |         pNext_chain.push_back(reinterpret_cast<VkBaseOutStructure*>(&features));
      |                                                                     ^~~~~~~~~
../vk-bootstrap/src/VkBootstrap.cpp:741:69: warning: dereferencing type-punned pointer might break strict-aliasing rules [-Wstrict-aliasing]
  741 |         pNext_chain.push_back(reinterpret_cast<VkBaseOutStructure*>(&checks));
      |                                                                     ^~~~~~~
../vk-bootstrap/src/VkBootstrap.cpp: In member function ‘vkb::Result<vkb::Device> vkb::DeviceBuilder::build() const’:
../vk-bootstrap/src/VkBootstrap.cpp:1682:79: warning: dereferencing type-punned pointer might break strict-aliasing rules [-Wstrict-aliasing]
 1682 |             final_pnext_chain.push_back(reinterpret_cast<VkBaseOutStructure*>(&local_features2));
      |                                                                               ^~~~~~~~~~~~~~~~
../vk-bootstrap/src/VkBootstrap.cpp:1684:83: warning: dereferencing type-punned pointer might break strict-aliasing rules [-Wstrict-aliasing]
 1684 |                 final_pnext_chain.push_back(reinterpret_cast<VkBaseOutStructure*>(&features_node));
      |