Skip to content
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

Closed
llvmbot opened this issue Sep 11, 2019 · 7 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 11, 2019

Bugzilla Link 43282
Resolution FIXED
Resolved on Feb 25, 2020 14:28
Version trunk
OS All
Reporter LLVM Bugzilla Contributor
CC @Corristo,@ericastor,@rnk
Fixed by commit(s) D69626

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

@rnk
Copy link
Collaborator

rnk commented Sep 12, 2019

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/

@rnk
Copy link
Collaborator

rnk commented Oct 7, 2019

*** Bug llvm/llvm-bugzilla-archive#43584 has been marked as a duplicate of this bug. ***

@ericastor
Copy link
Contributor

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?

@ericastor
Copy link
Contributor

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

@rnk
Copy link
Collaborator

rnk commented Feb 25, 2020

*** Bug llvm/llvm-bugzilla-archive#45005 has been marked as a duplicate of this bug. ***

@rnk
Copy link
Collaborator

rnk commented Nov 27, 2021

mentioned in issue llvm/llvm-bugzilla-archive#43584

@rnk
Copy link
Collaborator

rnk commented Nov 27, 2021

mentioned in issue llvm/llvm-bugzilla-archive#45005

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla
Projects
None yet
Development

No branches or pull requests

3 participants