Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VmaCountBitsSet crashes on CPUs without support for MSVC __popcnt (ARM, older x86) #245

Closed
akien-mga opened this issue Feb 25, 2022 · 4 comments
Assignees
Labels
compatibility Compatibility with some platforms next release To be done as soon as possible

Comments

@akien-mga
Copy link
Contributor

__popcnt is used here unconditionally for MSVC without checking that it's actually supported by the CPU:

#ifdef _MSC_VER
return __popcnt(v);

This has been reported to triggering a crash in Godot: godotengine/godot#58463

I don't know much about this instruction myself but Stackoverflow seems to have decent advice on checking capability with cpuid here:
https://stackoverflow.com/a/42913358

@adam-sawicki-a
Copy link
Contributor

Thank you for reporting this issue. I removed usage of __popcnt intrinsic function. User can still provide optimized implementation by defining macro VMA_COUNT_BITS_SET.

I hope that functions like _BitScanForward are not affected in the same way?

@akien-mga
Copy link
Contributor Author

akien-mga commented Feb 25, 2022

Thank you for reporting this issue. I removed usage of __popcnt intrinsic function. User can still provide optimized implementation by defining macro VMA_COUNT_BITS_SET.

Thanks! That works, but if that optimization is important, I think you should be able to improve things to actually check CPU capabilities. The MSDN docs provide some guidance on how to check for Advanced Bit Manipulation support: https://docs.microsoft.com/en-us/cpp/intrinsics/popcnt16-popcnt-popcnt64?view=msvc-170

image

So far I didn't find information that would indicate that the GCC/Clang __builtin_popcount has a similar requirement, so you might be able to always use it, and only need to check CPU capabilities (or add a macro entry point) for MSVC. Might need double checking though.

I hope that functions like _BitScanForward are not affected in the same way?

Based on https://docs.microsoft.com/en-us/cpp/intrinsics/bitscanforward-bitscanforward64?view=msvc-170 I think it might be fine, it seems supported without specific requirements on all architectures supported by MSVC:

image

@akien-mga
Copy link
Contributor Author

Reading https://stackoverflow.com/questions/60165922/stdbitsetncount-vs-builtin-popcount it sounds like std::bitset<T>::count might be a good cross-platform alternative? http://www.cplusplus.com/reference/bitset/bitset/count/

@adam-sawicki-a
Copy link
Contributor

std::bitset is an STL container, so something different than we need here, where we want to just calculate bit count in a single number.

There is a nice header <bit> that offers what we need: https://en.cppreference.com/w/cpp/numeric/popcount but it is available only since C++20, while the library uses C++14.

So I am going to leave it as-is for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Compatibility with some platforms next release To be done as soon as possible
Projects
None yet
Development

No branches or pull requests

3 participants