VkFFT

VkFFT copied to clipboard

Hi,

as vkFFT uses sprintf for generating the shader code, it is dependent on the locale that is set globally for the program. To circumvent printing e.g. commas for floating point numbers given certain locales, vkFFT currently changes the locale "temporarily" using setlocale. But it looks like the implementation for this is bugged. Starting from this locale (result of setlocale(LC_ALL, 0);) before using vkFFT: LC_COLLATE=C;LC_CTYPE=German_Germany.1252;LC_MONETARY=C;LC_NUMERIC=C;LC_TIME=C it is set to C as soon as a vkFFT plan is initialized through initializeVkFFT and stays like that. Sometimes it switches back to German_Germany.1252 after initializing another vkFFT plan, but it still is not the locale it was before and kind of random if that happens or not.

I think the problem is storing a pointer to the result of setlocale as oldLocale - which probably is a bad idea, as this is not a memory location under vkFFTs control, so it likely is changed whenever setlocale is called again. Probably storing a copy of the string it returns would already fix some problems. But generally, I'd like to get rid of setting the locale in vkFFT, as this is a general problem in applications, that might rely on the globally set locale in multiple threads.

With the common sprintf implementation this seems to be not possible (though there are platform dependent versions that accept a locale as parameter, like _sprintf_s_l on windows). But for vkFFT it should be simple to at least optionally replace the sprintf calls with e.g. fmt::sprintf or stbsp_sprintf, both of which are locale agnostic.

If you are interested, I prototyped this for fmt::sprintf, which works fine so far. Maybe you could include something into the library to drop in a sprintf replacement e.g. via some define?

Thanks!

The prototypical modifications:

// some cpp using vkfft:

#include <fmt/printf.h>
#include <utility>

template<typename... Ts>
int vkfftSprintfReplacement(char * str, char const *fmt, Ts&&... args) {
    std::string formatted = fmt::sprintf(fmt, std::forward<Ts>(args)...);
    std::copy(formatted.begin(), formatted.end(), str);
    auto result = formatted.length();
    str[result] = '\0';
    return (int)result;
}

#define VKFFT_REPLACE_SPRINTF vkfftSprintfReplacement

#include <vkFFT.h>

// code ...

// in vkFFT.h:

// vkFFT.h includes go here

#ifdef VKFFT_REPLACE_SPRINTF
# define sprintf VKFFT_REPLACE_SPRINTF
#endif

// vkFFT.h body goes here, the setlocale calls are guarded with #ifndef VKFFT_REPLACE_SPRINTF

// vkFFT.h at the very bottom

#ifdef VKFFT_REPLACE_SPRINTF
# undef sprintf
#endif

Hello,

Thanks for pointing out that locale can be an issue with thread safety. The use of sprintf comes that originally VkFFT was a collection of pre-written glsl shaders that had no such problem. This collection was managed by a relatively simple C code. Once I needed to scale the algorithms to accommodate different primes/Bluestein/DCT etc - this is when the choice of sprintf for runtime generation happened. Well, it was the simplest choice, as I still didn't really need what C++ offers, so I just continued with pure C. While being fast, the current sprintf approach is not simple to read at all and the issue you mentioned is another justification to improve it. I am currently redesigning the single file approach and the ideas on the way kernel code is printed are very welcome - let's go beyond just fixing sprintf implementation .

I guess switching to C++ can also be considered - mainly to use C++ libraries as in your example and for multithreading.

Best regards, Dmitrii

FYI... I'm seeing crashes related to thread safety with this as well.

==2537019==ERROR: AddressSanitizer: heap-use-after-free on address 0x61200000f340 at pc 0x7faadf55d7e1 bp 0x7faa9092a8a0 sp 0x7faa9092a050
READ of size 2 at 0x61200000f340 thread T49 (FilterGraph)
    #0 0x7faadf55d7e0 in __interceptor_setlocale ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:3288
    #1 0x7faade5dbcc6 in freeShaderGenVkFFT /ceph/fast/home/azonenberg/code/scopehal-apps/lib/scopehal/../VkFFT/vkFFT/vkFFT.h:24503
    #2 0x7faade5deeef in shaderGenVkFFT /ceph/fast/home/azonenberg/code/scopehal-apps/lib/scopehal/../VkFFT/vkFFT/vkFFT.h:25501
    #3 0x7faade626d8a in VkFFTPlanAxis /ceph/fast/home/azonenberg/code/scopehal-apps/lib/scopehal/../VkFFT/vkFFT/vkFFT.h:33548
    #4 0x7faade63415c in initializeVkFFT /ceph/fast/home/azonenberg/code/scopehal-apps/lib/scopehal/../VkFFT/vkFFT/vkFFT.h:35581
    #5 0x7faade637bbc in VulkanFFTPlan::VulkanFFTPlan(unsigned long, unsigned long, int) /ceph/fast/home/azonenberg/code/scopehal-apps/lib/scopehal/VulkanFFTPlan.h:113
    #6 0x7faade638d05 in std::_MakeUniq<VulkanFFTPlan>::__single_object std::make_unique<VulkanFFTPlan, unsigned long&, unsigned long&, VulkanFFTPlan::VulkanFFTPlanFlags>(unsigned long&, unsigned long&, VulkanFFTPlan::VulkanFFTPlanFlags&&) /usr/include/c++/10/bits/unique_ptr.h:962
    #7 0x7faade48bde5 in FFTFilter::ReallocateBuffers(unsigned long, unsigned long, unsigned long) /ceph/fast/home/azonenberg/code/scopehal-apps/lib/scopeprotocols/FFTFilter.cpp:166
    #8 0x7faade48c053 in FFTFilter::Refresh(vk::raii::CommandBuffer&, vk::raii::Queue&) /ceph/fast/home/azonenberg/code/scopehal-apps/lib/scopeprotocols/FFTFilter.cpp:191
    #9 0x7faadc5071e4 in FilterGraphExecutor::DoExecutorThread(unsigned long) /ceph/fast/home/azonenberg/code/scopehal-apps/lib/scopehal/FilterGraphExecutor.cpp:227
    #10 0x7faadc506956 in FilterGraphExecutor::ExecutorThread(FilterGraphExecutor*, unsigned long) /ceph/fast/home/azonenberg/code/scopehal-apps/lib/scopehal/FilterGraphExecutor.cpp:175
    #11 0x7faadc50d382 in void std::__invoke_impl<void, void (*)(FilterGraphExecutor*, unsigned long), FilterGraphExecutor*, unsigned long>(std::__invoke_other, void (*&&)(FilterGraphExecutor*, unsigned long), FilterGraphExecutor*&&, unsigned long&&) /usr/include/c++/10/bits/invoke.h:60
    #12 0x7faadc50d261 in std::__invoke_result<void (*)(FilterGraphExecutor*, unsigned long), FilterGraphExecutor*, unsigned long>::type std::__invoke<void (*)(FilterGraphExecutor*, unsigned long), FilterGraphExecutor*, unsigned long>(void (*&&)(FilterGraphExecutor*, unsigned long), FilterGraphExecutor*&&, unsigned long&&) /usr/include/c++/10/bits/invoke.h:95
    #13 0x7faadc50d194 in void std::thread::_Invoker<std::tuple<void (*)(FilterGraphExecutor*, unsigned long), FilterGraphExecutor*, unsigned long> >::_M_invoke<0ul, 1ul, 2ul>(std::_Index_tuple<0ul, 1ul, 2ul>) /usr/include/c++/10/thread:264
    #14 0x7faadc50d131 in std::thread::_Invoker<std::tuple<void (*)(FilterGraphExecutor*, unsigned long), FilterGraphExecutor*, unsigned long> >::operator()() /usr/include/c++/10/thread:271
    #15 0x7faadc50d115 in std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(FilterGraphExecutor*, unsigned long), FilterGraphExecutor*, unsigned long> > >::_M_run() /usr/include/c++/10/thread:215
    #16 0x7faadb741ecf in execute_native_thread_routine ../../../../../src/libstdc++-v3/src/c++11/thread.cc:80
    #17 0x7faadb848ea6 in start_thread nptl/pthread_create.c:477
    #18 0x7faadb44bdee in __clone (/lib/x86_64-linux-gnu/libc.so.6+0xfddee)

0x61200000f340 is located 0 bytes inside of 271-byte region [0x61200000f340,0x61200000f44f)
freed by thread T49 (FilterGraph) here:
    #0 0x7faadf5c5b6f in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:123
    #1 0x7faadb37ff54 in setname locale/setlocale.c:199
    #2 0x7faadb37ff54 in __GI_setlocale locale/setlocale.c:385

previously allocated by thread T0 here:
    #0 0x7faadf5c5e8f in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x7faadb37f7e7 in new_composite_name locale/setlocale.c:170

Hello,

I have added the disableSetLocale option to the configuration. It removes VkFFT attempts to set locale - if enabled, it should be done by user. Also, oldLocale is now a string copy of the setlocale output, not a copy of a returned pointer. I know this is not the perfect solution.

Best regards, Dmitrii