cameron314 / concurrentqueue

A fast multi-producer, multi-consumer lock-free concurrent queue for C++11
Other
10k stars 1.69k forks source link

Linux fs.h causes compilation problems #376

Open mark-99 opened 8 months ago

mark-99 commented 8 months ago

/usr/include/linux/fs.h does this:

#define BLOCK_SIZE_BITS 10
#define BLOCK_SIZE (1<<BLOCK_SIZE_BITS)

This definition of BLOCK_SIZE as a preprocessor constant causes subsequent use of concurrentqueue.h or blockingconcurrentqueue.h to not compile correctly, as BLOCK_SIZE is used in moodycamel::ConcurrentQueueDefaultTraits.

This is clearly a Linux / C issue, but it's not something that's likely to be fixable in <linux/fs.h>.

My workaround was #undef BLOCK_SIZE before including concurrentqueue.h. You might consider doing this by default in the header, as clearly a macro of that name is going to cause problems.

Another option would be to #error and at least noisily warn the user of the problem - the compiler errors appear nonsensical and give no real clue what the problem is, so it took me a while to track this down. I wasn't even including <linux/fs.h>, it was being transitively included by liburing.h, which in turn was a dependency for Boost.ASIO's BOOST_ASIO_HAS_IO_URING mode. So while I suspected a #define was to blame, it didn't actually appear in any source code or package dependencies.

vcpkg_installed/x64-linux-cpp20/include/concurrentqueue/concurrentqueue.h:343:29: error: expected unqualified-id before numeric constant
  343 |         static const size_t BLOCK_SIZE = 32;
      |                             ^~~~~~~~~~
vcpkg_installed/x64-linux-cpp20/include/concurrentqueue/concurrentqueue.h:343:29: error: expected ‘)’ before numeric constant
  343 |         static const size_t BLOCK_SIZE = 32;
      |                             ^~~~~~~~~~

I guess the other option would be change the name of the variable you use, but that's a back-compat problem for your own users seeing as it's a customisation point.

cameron314 commented 8 months ago

IMO silently undefining a system macro would be far more confusing. I could add an #ifdef #error but then the user still wouldn't know where the macro is defined.

mark-99 commented 8 months ago

The user's code won't compile either way, but I'd agree probably letting the user decide what to do would be the best bet.

You don't have to be definitive, but pointing towards <linux/fs.h> is certainly the most likely culprit, as any well-behaved 3rd party C headers should be prefixing their macros as has been best practise for decades. I assume it's just basically unfixable in Linux itself.

Indeed any clue that a rogue preprocessor macro is defined would be helpful.

Other option is changing the queue traits var name, which is the best end result, but is a change for existing users who don't currently see any issues.

cameron314 commented 8 months ago

Looks like I could use push_macro/pop_macro to avoid this for GCC and clang within the header itself. But it's not really improving anything, just hiding it. Also clang's implementation of push_macro appears buggy (warns about the original macro being unused after it's popped back).

mark-99 commented 8 months ago

This implies it's present on msvc also: https://gcc.gnu.org/onlinedocs/gcc/Push_002fPop-Macro-Pragmas.html edit yes here: https://learn.microsoft.com/en-us/cpp/preprocessor/push-macro?view=msvc-170

I guess the warning is because you didn't #define another macro of the same name inside the scope, it doesn't seem to be taking into account there could be other reasons. I guess the warning can also be push/pop disabled.

But yeah... that (if it works) only solves part of the problem, but not if the user has customised ConcurrentQueueDefaultTraits::BLOCK_SIZE.

You could argue an #undef or a similar push_macro/pop_macro in user code is more palatable, and that a #define of a generic symbol can cause all sorts of problem with downstream code in any case. Although it may also make root cause even more obscure, if the identical traits struct in the header compiles just fine.