-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Preprocessor bug/incompatibility regarding __VA_ARGS__ and comma when compiling with -fms-compatibility on Windows #42627
Comments
I think this has been reported a few times. Issue 42112 looks like the closest, but there may be others. We're in a bad situation where clang -fms-compatibility is neither standards conformant nor MSVC compatible, and users (Boost) in the past have been frustrated with that, since they don't want to add ifdefs to cope with our buggy compatibility hacks. My hope is that MSVC will implement conforming rules, as described here, and then we will tell users to turn that on and everything will be standard: |
*** Bug llvm/llvm-bugzilla-archive#43584 has been marked as a duplicate of this bug. *** |
Working with an instrumented version of pp-trace, I've managed to minimize the repro a bit further: #define TEST(...) COMBINE(THIRD_ARGUMENT(VA_ARGS, "a", "b")) This should output "a" because TEST() gets a comma inside the VA_ARGS... but with -fms-compatibility, single commas are flagged with Token::IgnoredComma to keep from being considered as an argument separator, so we instead output "b". If we instead use #define TEST(...) THIRD_ARGUMENT(VA_ARGS, "a", "b") without the COMBINE(), we still output "b" successfully replicate the non-compliance of the MSVC preprocessor. However, MSVC behaves correctly when the intervening COMBINE() is in place - and we don't! To fix this, we need to clear the IgnoredComma flag at the right point when expanding with an intervening macro. I don't really understand PPMacroExpansion.cpp yet, so I haven't found the right point to do this. Anyone else have an idea? |
I believe https://reviews.llvm.org/D69626 will fix this. |
*** Bug llvm/llvm-bugzilla-archive#45005 has been marked as a duplicate of this bug. *** |
mentioned in issue llvm/llvm-bugzilla-archive#43584 |
mentioned in issue llvm/llvm-bugzilla-archive#45005 |
Extended Description
We are currently exploring to compile our software on Windows with Clang, but hit a couple Clang/LLVM bugs along the way. This one is about incorrect emulation of preprocessor.
Minimal code to reproduction the problem:
#define BUILD_LIB_XYZ ,
#define DLL(...) COMBINE(DLL_HELP_GET_THIRD_ARGUMENT(VA_ARGS, "dllexport", "dllimport"))
#define DLL_HELP_GET_THIRD_ARGUMENT(A, B, C, ...) C
#define COMBINE(...) VA_ARGS
static_assert(0, DLL(BUILD_LIB_XYZ));
The code should output "dllexport" because DLL() gets a comma inside the VA_ARGS which shifts the "dllexport" from the second to the third argument.
However on Clang with -fms-compatibility this is not the case, and "dllimport" is printed instead. Note that only on Microsoft's compiler the COMBINE is necessary to get this correct behavour.
Background: In our software project we use this preprocessor trick to choose between __declspec(dllimport) and __declspec(dllexport) (or actually attribute((dllexport)) and attribute((dllimport)) on Clang). It works like this: In the build system we define a preprocessor symbol like BUILD_LIB_XYZ=, only when building library XYZ. This automatically evaluates to DLLEXPORT for all the API of library XYZ and DLLIMPORT for everything else without having to setup new macros for each library. It worked fine on Microsoft's Compiler since at least 2015, GCC and even Clang, normally at least. However not when provided with -fms-compatibility. We cannot disable that option either though because our source files sometimes use <Windows.h> for platformspecific API and as far as we know, there is no way to only selectively apply -fms-compatibility for certain #includes.
Live code on Godbolt: https://gcc.godbolt.org/z/KODv16
The text was updated successfully, but these errors were encountered: