<div dir="ltr">I also tried this patch on a bunch of gtest code and on an iostream hello world program using VS 2012's iostream and it works for both. Awesome!<div><div><br></div></div><div>I can also repro the assertions Aaron hits in the Preprocessor tests. They're caused by:</div>
<div><div>- HasUCN = 0x40 // This identifier contains a UCN.</div><div>+ HasUCN = 0x40, // This identifier contains a UCN.</div><div>+ IgnoredComma = 0x08, // This comma is not a macro argument separator (MS).</div>
</div><div><br></div><div>It should be 0x80, not 0x08.</div><div><br></div><div>With that change, everything passes, and I committed it as r184968.</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jun 26, 2013 at 9:18 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I applied the patch, but am getting crashes with a few of the test<br>
cases. Eg) test\Preprocessor\macro_fn_comma_swallow2.c has an<br>
-fms-compatibility test which fails:<br>
<br>
Assertion failed: Length < Tok.getLength() && "NeedsCleaning flag set<br>
on token that didn't need cleaning!", file<br>
F:\official-llvm\llvm\tools\clang\lib\Lex\Lexer.cpp, line 290<br>
Stack dump:<br>
0. Program arguments: build\bin\debug\clang -cc1 -E -fms-compatibility<br>
llvm\tools\clang\test\Preprocessor\macro_fn_comma_swallow2.c<br>
0x65E4C94A (0x0000000A 0x00000000 0x0544E640 0x65F27114), exit() +<br>
0x13A bytes(s)<br>
0x65F37A9C (0x0544E684 0x0544E654 0x0544DB18 0x0544DB48), abort() +<br>
0x1C bytes(s)<br>
0x65F27114 (0x04527580 0x04527518 0x00000122 0x0544E888), _wassert() +<br>
0xC04 bytes(s)<br>
0x0221129B (0x0544E8F8 0x0036228E 0x0033FEE0 0x0544E77C),<br>
getSpellingSlow() + 0x19B bytes(s),<br>
f:\official-llvm\llvm\tools\clang\lib\lex\lexer.cpp, line 290 + 0x2D<br>
byte(s)<br>
0x02209890 (0x0544E8F8 0x0544E730 0x00344DE0 0x0033FEE0),<br>
clang::Lexer::getSpelling() + 0x150 bytes(s),<br>
f:\official-llvm\llvm\tools\clang\lib\lex\lexer.cpp, line 412 + 0x17<br>
byte(s)<br>
0x021BBE5D (0x0544E8F8 0x0544E730 0x00000000 0x0544E914),<br>
clang::Preprocessor::getSpelling() + 0x2D bytes(s),<br>
f:\official-llvm\llvm\tools\clang\include\clang\lex\preprocessor.h,<br>
line 992 + 0x1F byte(s)<br>
0x021BAAC9 (0x003494E0 0x0544E8F8 0x003600C8 0x00391260),<br>
PrintPreprocessedTokens() + 0x249 bytes(s),<br>
f:\official-llvm\llvm\tools\clang\lib\frontend\printpreprocessedoutput.cpp,<br>
line 634 + 0x15 byte(s)<br>
0x021B955A (0x003494E0 0x00391260 0x00342CD0 0x0544E9F4),<br>
clang::DoPrintPreprocessedInput() + 0x28A bytes(s),<br>
f:\official-llvm\llvm\tools\clang\lib\frontend\printpreprocessedoutput.cpp,<br>
line 744 + 0x15 byte(s)<br>
0x020F948D (0x0544E9A0 0xCCCCCCCC 0xCCCCCCCC 0xCCCCCCCC),<br>
clang::PrintPreprocessedAction::ExecuteAction() + 0x14D bytes(s),<br>
f:\official-llvm\llvm\tools\clang\lib\frontend\frontendactions.cpp,<br>
line 556 + 0x1B byte(s)<br>
0x020F4F6C (0x0544EA90 0x0544FAAC 0xCCCCCCCC 0xCCCCCCCC),<br>
clang::FrontendAction::Execute() + 0xAC bytes(s),<br>
f:\official-llvm\llvm\tools\clang\lib\frontend\frontendaction.cpp,<br>
line 378 + 0xF byte(s)<br>
0x020E0081 (0x00345E18 0x0544EFF0 0xCCCCCCCC 0xCCCCCCCC),<br>
clang::CompilerInstance::ExecuteAction() + 0x281 bytes(s),<br>
f:\official-llvm\llvm\tools\clang\lib\frontend\compilerinstance.cpp,<br>
line 686<br>
0x01E1B3B8 (0x0033FE50 0x0544FEE0 0xCCCCCCCC 0xCCCCCCCC),<br>
clang::ExecuteCompilerInvocation() + 0x308 bytes(s),<br>
f:\official-llvm\llvm\tools\clang\lib\frontendtool\executecompilerinvocation.cpp,<br>
line 236 + 0x11 byte(s)<br>
0x00BE6BC0 (0x0544FAB4 0x0544FAC0 0x00340298 0x00BD1320), cc1_main() +<br>
0x2F0 bytes(s),<br>
f:\official-llvm\llvm\tools\clang\tools\driver\cc1_main.cpp, line 100<br>
+ 0xE byte(s)<br>
0x00BD5399 (0x00000005 0x00339B10 0x0033B330 0x7A2FBA24), main() +<br>
0x149 bytes(s),<br>
f:\official-llvm\llvm\tools\clang\tools\driver\driver.cpp, line 360 +<br>
0x45 byte(s)<br>
0x02F32139 (0x0544FF44 0x749633AA 0x7EFDE000 0x0544FF84),<br>
__tmainCRTStartup() + 0x199 bytes(s),<br>
f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c, line 536 + 0x19<br>
byte(s)<br>
0x02F3227D (0x7EFDE000 0x0544FF84 0x76EC9EF2 0x7EFDE000),<br>
mainCRTStartup() + 0xD bytes(s),<br>
f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c, line 377<br>
0x749633AA (0x7EFDE000 0xEAA34A4D 0x00000000 0x00000000),<br>
BaseThreadInitThunk() + 0x12 bytes(s)<br>
0x76EC9EF2 (0x02F32270 0x7EFDE000 0x00000000 0x00000000),<br>
RtlInitializeExceptionChain() + 0x63 bytes(s)<br>
0x76EC9EC5 (0x02F32270 0x7EFDE000 0x00000000 0x00000000),<br>
RtlInitializeExceptionChain() + 0x36 bytes(s)<br>
<br>
I get the same failure with Preprocessor\microsoft-ext.c<br>
<br>
Do all of the test cases pass cleanly for you?<br>
<br>
Thanks!<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Wed, Jun 26, 2013 at 2:29 AM, Will Wilson <<a href="mailto:will@indefiant.com">will@indefiant.com</a>> wrote:<br>
> Thanks for the reviews!<br>
><br>
> Updated patch attached with unique flag value, spelling change and trailing<br>
> whitespace culled. Can someone commit if it looks acceptable?<br>
><br>
> - Will.<br>
><br>
><br>
> On 26 June 2013 01:57, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>><br>
>> On Tue, Jun 25, 2013 at 3:39 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> wrote:<br>
>> >> Index: test/Preprocessor/microsoft-ext.c<br>
>> >> ===================================================================<br>
>> >> --- test/Preprocessor/microsoft-ext.c (revision 184863)<br>
>> >> +++ test/Preprocessor/microsoft-ext.c (working copy)<br>
>> >> @@ -22,3 +22,14 @@<br>
>> >> ACTION_TEMPLATE(InvokeArgument,<br>
>> >> HAS_1_TEMPLATE_PARAMS(int, k),<br>
>> >> AND_2_VALUE_PARAMS(p0, p1));<br>
>> >> +<br>
>> >> +<br>
>> >> +// This tests compatibility with behaviour needed for type_traits in<br>
>> >> VS2012<br>
>> >> +// Test based on _VARIADIC_EXPAND_0X macros in xstddef of VS2012<br>
>> >> +#define _COMMA ,<br>
>> >> +<br>
>> >> +#define MAKER(_arg1, _comma, _arg2) void func(_arg1 _comma _arg2) {}<br>
>> >> +#define MAKE_FUNC(_makerP1, _makerP2, _arg1, _comma, _arg2) \<br>
>> >> + _makerP1 ## _makerP2(_arg1, _comma, _arg2)<br>
>> >> +<br>
>> >> +MAKE_FUNC(MAK, ER, int a, _COMMA, int b); // CHECK: void func(int a ,<br>
>> >> int b) {}<br>
>> >> Index: include/clang/Lex/Token.h<br>
>> >> ===================================================================<br>
>> >> --- include/clang/Lex/Token.h (revision 184863)<br>
>> >> +++ include/clang/Lex/Token.h (working copy)<br>
>> >> @@ -74,6 +74,7 @@<br>
>> >> StartOfLine = 0x01, // At start of line or only after<br>
>> >> whitespace.<br>
>> >> LeadingSpace = 0x02, // Whitespace exists before this token.<br>
>> >> DisableExpand = 0x04, // This identifier may never be macro<br>
>> >> expanded.<br>
>> >> + IgnoredComma = 0x04, // [reused] Comma is not an argument<br>
>> >> separator (MS)<br>
>> ><br>
>> > I'm not too keen on reusing this value, even though it may be safe to<br>
>> > do.<br>
>><br>
>> Agreed. We have a spare bit here, let's not worry about packing them<br>
>> until we actually run out.<br>
>><br>
>> >> NeedsCleaning = 0x08, // Contained an escaped newline or<br>
>> >> trigraph.<br>
>> >> LeadingEmptyMacro = 0x10, // Empty macro exists before this token.<br>
>> >> HasUDSuffix = 0x20, // This string or character literal has a<br>
>> >> ud-suffix.<br>
>> >> Index: lib/Lex/TokenLexer.cpp<br>
>> >> ===================================================================<br>
>> >> --- lib/Lex/TokenLexer.cpp (revision 184863)<br>
>> >> +++ lib/Lex/TokenLexer.cpp (working copy)<br>
>> >> @@ -277,6 +277,14 @@<br>
>> >> unsigned FirstResult = ResultToks.size();<br>
>> >> unsigned NumToks = MacroArgs::getArgLength(ResultArgToks);<br>
>> >> ResultToks.append(ResultArgToks, ResultArgToks+NumToks);<br>
>> >> +<br>
>> >> + // In Microsoft-compatibility mode, we follow MSVC's<br>
>> >> preprocessing<br>
>> >> + // behaviour by not considering single commas from nested<br>
>> >> macro<br>
>> >> + // expansions as argument separators. Set a flag on the token<br>
>> >> so we can<br>
>> >> + // test for this later when the macro expansion is processed.<br>
>> ><br>
>> > Trailing whitespace. Also, not certain if we have a preference for US<br>
>> > spellings or not (behavior instead of behaviour).<br>
>> ><br>
>> >> + if (PP.getLangOpts().MicrosoftMode && NumToks == 1 &&<br>
>> >> + ResultToks.back().is(tok::comma))<br>
>> >> + ResultToks.back().setFlag(Token::IgnoredComma);<br>
>> >><br>
>> >> // If the '##' came from expanding an argument, turn it into<br>
>> >> 'unknown'<br>
>> >> // to avoid pasting.<br>
>> >> Index: lib/Lex/PPMacroExpansion.cpp<br>
>> >> ===================================================================<br>
>> >> --- lib/Lex/PPMacroExpansion.cpp (revision 184863)<br>
>> >> +++ lib/Lex/PPMacroExpansion.cpp (working copy)<br>
>> >> @@ -458,7 +458,12 @@<br>
>> >> }<br>
>> >> } else if (Tok.is(tok::l_paren)) {<br>
>> >> ++NumParens;<br>
>> >> - } else if (Tok.is(tok::comma) && NumParens == 0) {<br>
>> >> + } else if (Tok.is(tok::comma) && NumParens == 0 &&<br>
>> >> + !(Tok.getFlags() & Token::IgnoredComma)) {<br>
>> >> + // In Microsoft-compatibility mode, single commas from nested<br>
>> >> macro<br>
>> >> + // expansions should not be considered as argument separators.<br>
>> >> We test<br>
>> >> + // for this with the IgnoredComma token flag above.<br>
>> ><br>
>> > Trailing whitespace<br>
>> ><br>
>> >> +<br>
>> >> // Comma ends this argument if there are more fixed arguments<br>
>> >> expected.<br>
>> >> // However, if this is a variadic macro, and this is part of<br>
>> >> the<br>
>> >> // variadic part, then the comma is just an argument token.<br>
>> ><br>
>> > Other than the nitpicks, the patch LGTM and I'm very excited for it!<br>
>> > Thank you for working on it. :-)<br>
>> ><br>
>> > ~Aaron<br>
>> ><br>
>> > On Tue, Jun 25, 2013 at 5:44 PM, Will Wilson <<a href="mailto:will@indefiant.com">will@indefiant.com</a>> wrote:<br>
>> >> Hi All,<br>
>> >><br>
>> >> This patch is a rework of João's original patch applied as r163022 and<br>
>> >> later<br>
>> >> reverted in r164672. I've spent a few (long...) days attempting to get<br>
>> >> to<br>
>> >> the bottom of what caused the failures that led to the original patch<br>
>> >> being<br>
>> >> reverted. This should fix (once and for all) the error seen when<br>
>> >> compiling<br>
>> >> VS2012 type_traits and by extension most of the VS2012 STL headers:<br>
>> >><br>
>> >> In file included from C:\Program Files (x86)\Microsoft Visual Studio<br>
>> >> 11.0\VC\include\type_traits:1820:<br>
>> >> C:\Program Files (x86)\Microsoft Visual Studio<br>
>> >> 11.0\VC\include\xrefwrap(156,20): error : too many arguments provided<br>
>> >> to<br>
>> >> function-like macro invocation<br>
>> >><br>
>> >><br>
>> >> The main changes in this patch are:<br>
>> >><br>
>> >> Tokens used as macro arguments that expand to a *single* comma have<br>
>> >> special<br>
>> >> treatment - the comma will not be treated as a macro argument separator<br>
>> >> in<br>
>> >> MicrosoftMode. Tokens that expand into multiple tokens that may include<br>
>> >> commas are treated normally.<br>
>> >> I've reused 0x04 for the IgnoredComma flag as the other case<br>
>> >> (DisableExpand)<br>
>> >> is exclusively used for identifiers and won't conflict with tok::comma.<br>
>> >> It<br>
>> >> didn't seem worth using up an entirely new value for such a rare edge<br>
>> >> case<br>
>> >> as this.<br>
>> >><br>
>> >> In terms of testing, clang should now be able to compile VS2012<br>
>> >> type_traits.<br>
>> >> I tested successfully against Nico's original GTest test case (PR13924)<br>
>> >> and<br>
>> >> against boost.python 1.42.0 which also failed with the original patch.<br>
>> >> I've<br>
>> >> also added a new test case based on the VS2012 xstddef macros that led<br>
>> >> to<br>
>> >> the patch being needed in the first place.<br>
>> >><br>
>> >> All tests passed against latest trunk. Please review and commit if it<br>
>> >> LGTY...<br>
>> >><br>
>> >> Cheers,<br>
>> >> Will.<br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> cfe-commits mailing list<br>
>> >> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>> >><br>
>> ><br>
>> > _______________________________________________<br>
>> > cfe-commits mailing list<br>
>> > <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
><br>
><br>
><br>
> --<br>
> Indefiant Ltd.<br>
><br>
> Firsby Lodge, New Main Road, Scamblesby, Louth, Lincs LN11 9XH UK<br>
> Tel: +44 20 8123 7663 England Registered No. 07936820 VAT No. 128556202<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div>