FastNoise2 asserts without RTTI
Closed this issue · 19 comments
We are integrating this (awesome) library in our Unreal 5 project, and Unreal does not include RTTI (real time type information) when compiling. This makes the library assert in generator.inl on a dynamic_cast, which returns nullptr when no RTTI is available, even though the pointer and the pointer data are correct.
We fixed it by not doing this check, and directly doing reinterpret_cast(base). We're only using generators from the library, so the check is unnecessary for us it seems.
We were curious about 2 things:
- Rather than relying on dynamic_cast, why aren't you enforcing proper typing statically in the Generator class template? Can't we check statically that any subclass of Generator is well-formed (by static_assert(std::is_same_type_v) or something somehow?)
- Should the docs be updated, in case FastNoise2 doesn't support being embedded statically in projects without RTTI?
BTW, we could reproduce this error by compiling the FastNoiseTool without RTTI; it crashes at the same spot for the same reason.
Thanks for any insights!
This is because DispatchClass<Generator, SIMD> virtually inherits Generator
BTW, you can turn on RTTI in Unreal for a small compute/memory hit
This is because
DispatchClass<Generator, SIMD>virtually inheritsGenerator
I'm not super fluent in virtual inheritance, so pardon me if I'm totally in the wrong here. So actually, what you are saying is this dynamic_cast is required to get the proper SIMD functions called later from the simd pointer? Is our fix not an actual fix then in our case? Do you intend to add support for no-RTTI static linking, or should we just revert to using linking FastNoise dynamically?
I am in the same situation ( -fno-rtti is a requirement ).
On a side note, I don't understand why the SIMD level is not statically chosen at compile time instead of runtime. I don't see any scenario where a sub optimal level of simd is the desired outcome.
The dynamic_cast is required to go from a Generator* to a DispatchClass<Generator, SIMD>* because the virtual inheritance means there is a dynamic offset when converting the pointer this way. As opposed to a static_cast for normal inheritance where the offset in an object between different inherited classes is known at compile time.
Your reinterpret_cast change may work because Generator doesn't have any member variables but there is no guarantee it will work on all compilers as it is undefined behaviour.
The reason SIMD level is not statically defined at compile time is that different CPUs support different SIMD instruction sets, if you were to statically compile it for the fastest SIMD level any CPU that didn't support that instruction set would crash when trying to run the code. All the SIMD levels are compiled so that the fastest supported one can be selected at runtime
I was wondering how you were getting it to compile when you only changed that single dynamic_cast since there are many used for the metadata. But I guess you are still compiling the FastNoise static library with rtti enabled but linking it into a project with rtti disabled. So only the one dynamic_cast that is in the headers is causing you issues?
I'm not sure if there are any potential pitfalls from linking an rtti lib into a non-rtti project. But if not there are other ways to avoid that single dynamic_cast in the headers
So after looking into this more the dynamic_cast in Generator.inl SetSourceSIMDPtr() is necessary but in Generator.h SetSourceMemberVariable it's not needed and can be substituted for static_cast because it's an up cast not down cast.
Looking back at your first comment I think you meant Generator.h and not Generator.inl
I made a PR with the change, see if it works for you. #128
Thank you for your continuous support, much appreciated!
Hmmmm I've just tried compiling the NoiseTool without RTTI, and static link the library compiled with your fix and RTTI enabled, but the NoiseTool just displays a blank gray texture and no preview mesh rather than the actual noise.
If you're interested into reproing locally, here are the steps.
Added 2 new options to CMakeList at root:
option(FASTNOISE_COMPILE_RTTI "Build with RTTI" ON)
option(FASTNOISE_NOISETOOL_COMPILE_RTTI "Build tool with RTTI" ON)Appended to CMakeList in src/:
if (FASTNOISE_COMPILE_RTTI)
# Simply add the opposite flag to the target
if(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
target_compile_options(FastNoise PRIVATE "/GR")
else()
target_compile_options(FastNoise PRIVATE "-frtti") # works even if -fno-rtti is set to CXX_FLAGS
endif()
else()
if(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
target_compile_options(FastNoise PRIVATE "/GR-")
else()
target_compile_options(FastNoise PRIVATE "-fno-rtti") # works even if -fno-rtti is set to CXX_FLAGS
endif()
endif()Appended to CMakeList in NoiseTool/:
if (FASTNOISE_NOISETOOL_COMPILE_RTTI)
# Simply add the opposite flag to the target
if(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
target_compile_options(NoiseTool PRIVATE "/GR")
else()
target_compile_options(NoiseTool PRIVATE "-frtti") # works even if -fno-rtti is set to CXX_FLAGS
endif()
else()
if(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
target_compile_options(NoiseTool PRIVATE "/GR-")
else()
target_compile_options(NoiseTool PRIVATE "-fno-rtti") # works even if -fno-rtti is set to CXX_FLAGS
endif()
endif()Then built the cmake project with:
cmake -S . -B build -DFASTNOISE2_NOISETOOL=ON -DFASTNOISE_COMPILE_RTTI=ON -DFASTNOISE_NOISETOOL_COMPILE_RTTI=OFF -DBUILD_SHARED_LIBS=OFF
And buillt the tool and lib with:
cmake --build build --config Debug
For this test, the compiler was MSVC 19.38.33130.0 (Visual Studio 17 2022)
Do you get any warnings when building with rtti disabled in the tool?
The NoiseTool relies heavily on node metadata which might be causing issues without rtti, I did a similar test but with the Cpp1Include test and that worked correctly with rtti disabled which is a simpler use case
I can't test properly until later today
Yep, did get a bunch of warnings, but only in the Corrade and Magnum libraries, not on the NoiseTool project. Going to try compiling the tests rather that the tool like you mentioned and circle back!
One warning, there are some headers in the library itself still doing dynamic casts besides the one you fixed:
FastNoise\include\FastNoise\SmartNode.h(52,34): warning C4541: 'dynamic_cast' used on polymorphic type 'FastNoise::Generator' with /GR-; unpredictable behavior may result
Then running the tests yields this:
.\FastNoiseCpp11Test.exe
256
6.24979e-43, 6.24979e-43, 6.24979e-43, 6.24979e-43, 6.24979e-43, 6.24979e-43, 6.24979e-43, 6.24979e-43, 6.24979e-43, 6.24979e-43, 6.24979e-43, 6.24979e-43, 6.24979e-43, 6.24979e-43, 6.24979e-43, 6.24979e-43,
nullptr
I just tested this again, if I comment out the FastNoise::SmartNode<>::DynamicCast()s in the cpp11 include test I don't get any rtti warnings. So nothing is trying to use rtti as expected.
This is the output I get from the cpp11 test with rtti disabled:
256
0, -0.193317, -0.346159, -0.425395, -0.035198, -0.219012, -0.359478, -0.433357, -0.054894, -0.218109, -0.340588, -0.408138, -0.043832, -0.184164, -0.288178, -0.348815,
Which is expected and working correctly, so I'm not sure why you are getting weird outputs.
I tested with MSVC and ClangCl, debug and release builds, all gave the same output above. I've also checked the build output that /GR- is being used correctly
This is my change to disable rtti in the cpp11 test
if(MSVC)
target_compile_options(FastNoiseCpp11Test PRIVATE /GR-)
elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
target_compile_options(FastNoiseCpp11Test PRIVATE -fno-rtti)
endif()
FYI the NoiseTool also works correctly for me with rtti disabled
One of my coworker succeeded too, so something must have been wrong with the files I had I guess? Just confirming: you tried with a shared lib, right? A DLL, not static linking? We made it work with fastnoise as a dynamic library with RTTI on our end!
I tested both static and dynamic linking of the fastnoise library and both function the same for me, on both MSVC and ClangCl
Can you confirm it working on your side @davidtphx
I cloned your repository from scratch this morning, and can confirm: everything works just by applying your fix. NoiseTool and tests were compiled without RTTI, and statically linked FastNoise2 that was itself compiled with RTTI.
For making the test run and compile properly, I had to comment out the whole section about downcasting SmartNode like you did, but I had proper values output!
Do you think we're just lucky on undefined behavior here, with MSVC compiler? We are also targeting console platforms (with their own compilers) later this year and we wouldn't want surprises down the road if we can avoid them now!
Good to hear!
I think it was undefined behaviour when you changed it to reinterpret_cast but this should be fine, the linker would throw some kind of warning otherwise.
In any case the FastNoise nodes are always newed inside the library and are just managed via pointers (SmartNodes) through the API, which should remove any weirdness from passing objects newed with rtti disabled
Great!! Thank you again for your continued support 🙏 Should I close this issue or let it to you once you integrate your own PR? :)
The test file might have to be updated too to support no-RTTI testing in the future?