volk
volk copied to clipboard
gnuradio
gnuradio/volk/#430 move log2f_non_ieee back to kernel header
This minor change allows VOLK to build with older versions of GCC. Currently GCC6 is the minimum, and some developers are stuck using older tools.
Project fully builds for x86-64 as well as armv7. Tested by running volk_profile on both targets.
Older versions of G++ (pre v6) don't automatically sub in "std::" when building C code. A user in #430 linked this:
https://developers.redhat.com/blog/2016/02/29/why-cstdlib-is-more-complicated-than-you-might-think/
which describes it well.
The fix comes about because the kernels are all C code, and like the syntax. The C++ code which includes volk_common.h does not like the syntax, however. The C++ function would require "std::isinf".
I got away with moving it to the kernel header, because none of the C++ application code actually uses that function. Only a couple of the kernels do.
make_master_ububtu1604.log cmake_master_ububtu1604.log
Build logs attached. This is on Ubuntu 16.04 with GCC 5.4.0
I got away with moving it to the kernel header, because none of the C++ application code actually uses that function. Only a couple of the kernels do.
Gotcha! Thank you for the detailed explanation. What you bring up is indeed a critical oversight in mixing C & C++ code. Can you test wrapping the original function with extern "C" {}? That might do the trick, and if so then it's a better and safer solution. I will boot up an older computer of mine that uses GCC5 optionally to see if I can replicate the issue & verify the fix independently.
It does not build, with the same errors.
However, wrapping the entire function with a #ifndef __cplusplus macro does the trick.
Clearly, this disables the function for C++ code, but it does have the added safety of not relying on the kernel headers being included in the right order.
hmm ... interesting. Thanks for testing! I think a point in having that code where it is is to make it available outside the kernel. Which means it needs to be usable in C or C++. Hence my hoping that extern would do the trick. I need to look at the code more to decide if I'm OK with just moving that function into the kernel.
Understood. It's a simple enough function that I can likely macro wrap 2 versions. Let me give that a shot and update the branch.
Now that Ubuntu 16.04 is EOL, I'd say we can avoid this work around (and thus added complexity).
Still, I'd be happy to merge your README updates. Would you mind to reduce your PR to these commits?
Closing this PR now. There's reason to hope that this issue doesn't get raised again on later distros.