Preprocessor bug/incompatibility regarding __VA_ARGS__ and comma when compiling with -fms-compatibility on Windows

[Quuxplusone]

Bugzilla Link	PR43282
Status	RESOLVED FIXED
Importance	P normal
Reported by	Tom Hender (ToHe_EMA@gmx.de)
Reported on	2019-09-11 10:23:01 -0700
Last modified on	2020-02-25 14:28:25 -0800
Version	trunk
Hardware	PC All
CC	Balazs.CZIRAKI@nng.com, berglerma@gmail.com, epastor@google.com, htmldeveloper@gmail.com, llvm-bugs@lists.llvm.org, rnk@google.com
Fixed by commit(s)	D69626
Attachments
Blocks
Blocked by
See also

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

[Quuxplusone]

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:
https://devblogs.microsoft.com/cppblog/msvc-preprocessor-progress-towards-conformance/

[Quuxplusone]

_Bug 43584 has been marked as a duplicate of this bug._

[Quuxplusone]

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"))
#define THIRD_ARGUMENT(A, B, C, ...) C
#define COMBINE(...) __VA_ARGS__
static_assert(0, TEST(,));

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?

[Quuxplusone]

I believe https://reviews.llvm.org/D69626 will fix this.

[Quuxplusone]

_Bug 45005 has been marked as a duplicate of this bug._