Fix Microsoft compatibility handling of commas in nested macro expansions.

[epastor]

Summary

In Microsoft-compatibility mode, we currently emulate MSVC's patterns in unpacking VA_ARGS by deliberately not treating single commas from macro expansion as argument separators, marking them to be ignored. However, in MSVC's preprocessor, subsequent expansions DO treat these commas as argument separators. Libraries (e.g., Boost, GoogleTest) routinely use this as a workaround for MSVC's non-compliant preprocessor by adding an additional layer of indirection to force recognition of argument separators.

To match this behavior, we now ignore each comma at most once.

Includes a small unit test that validates we match MSVC's behavior as shown in https://gcc.godbolt.org/z/y0twaq

[thakis]

Is this needed to parse system headers?

In general, we try to emulate only warts we must emulate, and if feasible we try to emit some -Wmicrosoft warning when the compat path is taken.

(MSVC added /experimental:preprocessor like 2 years ago, but it hasn't made it to a /Zc: flag yet so I suppose standard headers might keep needing these tweaks.)

(See https://docs.google.com/presentation/d/1oxNHaVjA9Gn_rTzX6HIpJHP7nXRua_0URXxxJ3oYRq0/edit#slide=id.g71ecd450e_2_843 slide 23-27)

[epastor]

In D69626#1728113, @thakis wrote:

Is this needed to parse system headers?

Interesting question. This change isn't - but I presume the (pre-existing) introduction of IgnoredComma was.

In general, we try to emulate only warts we must emulate, and if feasible we try to emit some -Wmicrosoft warning when the compat path is taken.

Sure - this change actually brings the preprocessor closer to compliance. I think the prior use of IgnoredComma had overshot MSVC by a bit. For an illustration, see
https://gcc.godbolt.org/z/XakAYf (same code as above, but in MSVC, clang, and clang -fms-compatibility)
MSVC shows it's non-compliant on TEST, and -fms-compatibility matches it there. However, MSVC matches clang's behavior for WRAPPED_TEST, as adding another layer of macro expansion is a standard trick for forcing MSVC's preprocessor to act compliant... but clang -fms-compatibility still gives the non-compliant result, since that trick doesn't work to remove IgnoredCommas.

If we wanted to detect use of the compatibility feature, we might need to apply the preprocessor both ways and see if the results were different; I'm not sure all IgnoredCommas will actually make a difference in the parsing. As is, we'd need to emit a -Wmicrosoft warning any time we saw an isolated comma as a macro parameter... which I think might get confusing?

[rnk]

lgtm

Based on the comments it looks like you confirmed this matches MSVC's behavior, at least in this regard. I haven't stared at this deeply to think of all the corner cases, though.

To Nico's point, I think this change doesn't move us away from conformance, it's a bug in our compatibility. We've created a bad situation here were we are buggily implementing MSVC pre-processor rules, and nobody should have to add a third set of ifdefs to deal with that.

[epastor]

Thanks, Reid; I'm not 100% sure I've checked all the corner cases either, but this at least seems like a step forward.

As a reminder: I don't have commit access. Could someone else commit this for me?

clang/lib/Lex/PPMacroExpansion.cpp

while (true) {
           if (!ArgTokens.empty() &&
               ArgTokens.back().commaAfterElided()) {
             FoundElidedComma = true;
           }
           break;
         }
       } else if (Tok.is(tok::l_paren)) {
         ++NumParens;
-      } else if (Tok.is(tok::comma) && NumParens == 0 &&
-                 !(Tok.getFlags() & Token::IgnoredComma)) {
+      } else if (Tok.is(tok::comma)) {
         // In Microsoft-compatibility mode, single commas from nested macro
         // expansions should not be considered as argument separators. We test
-        // for this with the IgnoredComma token flag above.
-
-        // Comma ends this argument if there are more fixed arguments expected.
-        // However, if this is a variadic macro, and this is part of the
-        // variadic part, then the comma is just an argument token.
-        if (!isVariadic) break;
+        // for this with the IgnoredComma token flag.
+        if (Tok.getFlags() & Token::IgnoredComma) {
+          // However, in MSVC's preprocessor, subsequent expansions do treat
+          // these commas as argument separators. This leads to a common
+          // workaround used in macros that need to work in both MSVC and
+          // compliant preprocessors. Therefore, the IgnoredComma flag can only
+          // apply once to any given token.
+          Tok.clearFlag(Token::IgnoredComma);
+        } else if (NumParens == 0) {
+          // Comma ends this argument if there are more fixed arguments
+          // expected. However, if this is a variadic macro, and this is part of
+          // the variadic part, then the comma is just an argument token.
+          if (!isVariadic)
+            break;
           if (NumFixedArgsLeft > 1)
             break;
+        }
       } else if (Tok.is(tok::comment) && !KeepMacroComments) {
         // If this is a comment token in the argument list and we're just in
         // -C mode (not -CC mode), discard the comment.
         continue;
       } else if (!Tok.isAnnotation() && Tok.getIdentifierInfo() != nullptr) {
         // Reading macro arguments can cause macros that we are currently
         // expanding from to be popped off the expansion stack.  Doing so causes
         // them to be reenabled for expansion.  Here we record whether any

clang/test/Preprocessor/microsoft-ext.c

 #define ACTION_TEMPLATE(name, template_params, value_params)\
 class GMOCK_ACTION_CLASS_(name, value_params) {\
 }
 
 ACTION_TEMPLATE(InvokeArgument,
                 HAS_1_TEMPLATE_PARAMS(int, k),
                 AND_2_VALUE_PARAMS(p0, p1));
 
+// Regression test for PR43282; check that we match MSVC's failure to unpack
+// __VA_ARGS__ unless forwarded through another macro.
+#define THIRD_ARGUMENT(A, B, C, ...) C
+#define TEST(...) THIRD_ARGUMENT(__VA_ARGS__, 1, 2)
+#define COMBINE(...) __VA_ARGS__
+#define WRAPPED_TEST(...) COMBINE(THIRD_ARGUMENT(__VA_ARGS__, 1, 2))
+// Check that we match MSVC's failure to unpack __VA_ARGS__, unless forwarded
+// through another macro
+auto packed = TEST(,);
+auto unpacked = WRAPPED_TEST(,);
+// CHECK: auto packed = 2;
+// CHECK: auto unpacked = 1;
+
 // This tests compatibility with behaviour needed for type_traits in VS2012
 // Test based on _VARIADIC_EXPAND_0X macros in xstddef of VS2012
 #define _COMMA ,
 
 #define MAKER(_arg1, _comma, _arg2)                                            \
   void func(_arg1 _comma _arg2) {}
 #define MAKE_FUNC(_makerP1, _makerP2, _arg1, _comma, _arg2)                    \
   _makerP1##_makerP2(_arg1, _comma, _arg2)