llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.79k stars 11.9k forks source link

Incorrect macro expansion in clang on Windows with VC++ target #20651

Open b1622bb8-0285-400e-ba44-ba0e5027e546 opened 10 years ago

b1622bb8-0285-400e-ba44-ba0e5027e546 commented 10 years ago
Bugzilla Link 20277
Version trunk
OS Windows XP
CC @rnk

Extended Description

Clang is not doing macro expansion correctly in an instance which I will show. The build is clang on Windows targeting VC++ built from the latest trunk. The source is:

define REM(...) __VA_ARGS__

define EAT(...)

define CAT(a, b) CAT_I(a, b)

define CAT_I(a, b) a ## b

define LPAREN() (

define RPAREN() )

define COMMA() ,

define STL(seq) STL_I(SBT(seq))

define STL_I(bseq) STL_A bseq NIL STL_B bseq

define STL_A(m, e) m(LPAREN() e COMMA() STL_A_ID)

define STL_A_ID() STL_A

define STL_B(m, e) m(RPAREN() STL_B_ID)

define STL_B_ID() STL_B

define SBT(seq) CAT(SBT_A seq, 0)

define SBT_A(...) (SBT_GET_REM(VA_ARGS), VA_ARGS)() SBT_B

define SBT_B(...) (SBT_GET_REM(VA_ARGS), VA_ARGS)() SBT_A

define SBT_GET_REM(...) REM

define SBT_A0 (EAT, ?)

define SBT_B0 (EAT, ?)

int main() { STL(()) return 0; }

The expansion of STL should be:

(,BOOST_PP_NIL)

Admittedly placed in a CPP main function this should lead to a compiler error etc. since '(, BOOST_PP_NIL )' by itself as C++ is invalid, but I have included it in this way to keep things simple and to show that the macro expansion itself is valid. The point is that clang.exe gives these errors trying to expand the macro:


seq_clang_failure.cpp(23,1) : error: too few arguments provided to function-like macro invocation STL(()) ^ seq_clang_failure.cpp(8,25) : note: expanded from macro 'STL'

define STL(seq) STL_I(SBT(seq))

                    ^

seq_clang_failure.cpp(14,23) : note: expanded from macro 'SBT'

define SBT(seq) CAT(SBT_A seq, 0)

                  ^

seq_clang_failure.cpp(15,59) : note: expanded from macro 'SBT_A'

define SBT_A(...) (SBT_GET_REM(VA_ARGS), VA_ARGS)() SBT_B

                                                      ^

seq_clang_failure.cpp(3,26) : note: expanded from macro 'CAT'

define CAT(a, b) CAT_I(a, b)

                     ^

seq_clang_failure.cpp(4,22) : note: expanded from macro 'CAT_I'

define CAT_I(a, b) a ## b

                 ^

seq_clang_failure.cpp(9,28) : note: expanded from macro 'STL_I'

define STL_I(bseq) STL_A bseq NIL STL_B bseq

                       ^

seq_clang_failure.cpp(10,10) : note: macro 'STL_A' defined here

define STL_A(m, e) m(LPAREN() e COMMA() STL_A_ID)

     ^

seq_clang_failure.cpp(23,1) : error: use of undeclared identifier 'STL_A' STL(()) ^ seq_clang_failure.cpp(8,19) : note: expanded from macro 'STL'

define STL(seq) STL_I(SBT(seq))

              ^

seq_clang_failure.cpp(9,22) : note: expanded from macro 'STL_I'

define STL_I(bseq) STL_A bseq NIL STL_B bseq

                 ^

seq_clang_failure.cpp(23,1) : error: use of undeclared identifier 'EAT' seq_clang_failure.cpp(8,25) : note: expanded from macro 'STL'

define STL(seq) STL_I(SBT(seq))

                    ^

seq_clang_failure.cpp(14,19) : note: expanded from macro 'SBT'

define SBT(seq) CAT(SBT_A seq, 0)

              ^

seq_clang_failure.cpp(3,20) : note: expanded from macro 'CAT'

define CAT(a, b) CAT_I(a, b)

               ^

note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)

(2,1) : note: expanded from here SBT_B0 ^ seq_clang_failure.cpp(19,18) : note: expanded from macro 'SBT_B0' # define SBT_B0 (EAT, ?) ^ seq_clang_failure.cpp(9,28) : note: expanded from macro 'STL_I' # define STL_I(bseq) STL_A bseq NIL STL_B bseq ^ seq_clang_failure.cpp(23,1) : error: too few arguments provided to function-like macro invocation seq_clang_failure.cpp(8,25) : note: expanded from macro 'STL' # define STL(seq) STL_I(SBT(seq)) ^ seq_clang_failure.cpp(14,23) : note: expanded from macro 'SBT' # define SBT(seq) CAT(SBT_A seq, 0) ^ seq_clang_failure.cpp(15,59) : note: expanded from macro 'SBT_A' # define SBT_A(...) (SBT_GET_REM(__VA_ARGS__), __VA_ARGS__)() SBT_B ^ seq_clang_failure.cpp(3,26) : note: expanded from macro 'CAT' # define CAT(a, b) CAT_I(a, b) ^ seq_clang_failure.cpp(4,22) : note: expanded from macro 'CAT_I' # define CAT_I(a, b) a ## b ^ seq_clang_failure.cpp(9,43) : note: expanded from macro 'STL_I' # define STL_I(bseq) STL_A bseq NIL STL_B bseq ^ seq_clang_failure.cpp(12,10) : note: macro 'STL_B' defined here # define STL_B(m, e) m(RPAREN() STL_B_ID) ^ ------------------------------------------------------------------------------ The code is actually taken from Boost PP from the BOOST_PP_SEQ_TO_LIST functionality when using clang. I have simplified its presentation as a bug for convenience. A '()' is a valid Boost PP seq and '(,BOOST_PP_NIL)' is a valid Boost PP list. The STL macro is just my shorthand for the actual BOOST_PP_SEQ_TO_LIST. This is a serious bug. Doing straightforward macro expansion should not fail in clang. The macro expansion exceeds in all versions of gcc, as well as in the Boost Wave driver. So unless clang know something that I do not know about macro expansion in the latest C++, this needs to be fixed. The command line parameters are: -TP /Od /Ob0 /W3 /GR /MDd /Zc:forScope /Zc:wchar_t -fmsc-version=1800 /wd4675 /EHs -c This is being compiled by the Boost Build system on Windows 7 Ultimate SP1, with the latest clang.exe built successfully in Visual Studio 2012 using the release build.
rnk commented 10 years ago

If clang-cl.exe is actually going to be emulating Microsoft's broken preprocessor, then it is a useless product and I will never personally use it. This is completely the wrong thing to do. I will voice this on the clang developer mailing list in as strong a way that I can, but I cannot believe that this decision was made by any persons or people among the clang developers who know how broken the VC++ preprocessor is and why trying to emulate that brokeness is a foolish thing to do.

There is a certain amount of preprocessor emulation that we cannot avoid if we wish to use the Microsoft STL headers. Consider r184968 from June 2013, which may be relevant to Boost PP. That commit was necessary to parse VS2012 , which I am certain Boost is using elsewhere. In general, we try as hard as possible to avoid compatibility hacks like this, but sometimes there are system headers that force our hand.

See also the commits that Alp made in May to support things that Boost itself does like "#ifdef and". Boost is actually sufficiently popular that just fixing the latest Boost isn't actually enough, we end up having to implement hacks in the compiler just to deal with the large body of existing code.

b1622bb8-0285-400e-ba44-ba0e5027e546 commented 10 years ago

It may be specific to the Windows build of clang using Visual Studio. It cannot have anything to do with running under Windows per se AFAICS.

I'm not so sure. Visual C++ has a non-standard preprocessor, which clang emulates in Microsoft-compatibility mode. Looking into clang's source I found the following:

lib/Lex/TokenLexer.cpp#131

// In Microsoft-compatibility mode, a comma is removed in the expansion // of " ... , VA_ARGS " if VA_ARGS is empty. This extension is // not supported by gcc. if (!HasPasteOperator && !PP.getLangOpts().MSVCCompat) return false;

If I'm not mistaken this means that this macro:

define SBT_A(...) (SBT_GET_REM(VA_ARGS), VA_ARGS)() SBT_B

when called with no parameters, will expand to this:

define SBT_A(...) (SBT_GET_REM())() SBT_B

instead of this (note the comma):

define SBT_A(...) (SBT_GET_REM(), )() SBT_B

If clang-cl.exe is actually going to be emulating Microsoft's broken preprocessor, then it is a useless product and I will never personally use it. This is completely the wrong thing to do. I will voice this on the clang developer mailing list in as strong a way that I can, but I cannot believe that this decision was made by any persons or people among the clang developers who know how broken the VC++ preprocessor is and why trying to emulate that brokeness is a foolish thing to do.

llvmbot commented 10 years ago

GCC does something similar when using ##__VA_ARGS__ (clang supports this as an extension). Modifying your test case like this:

define SBT_A(...) (SBT_GET_REM(VA_ARGS), ##VA_ARGS)() SBT_B

define SBT_B(...) (SBT_GET_REM(VA_ARGS), ##VA_ARGS)() SBT_A

results in almost the same error messages.

llvmbot commented 10 years ago

It may be specific to the Windows build of clang using Visual Studio. It cannot have anything to do with running under Windows per se AFAICS.

I'm not so sure. Visual C++ has a non-standard preprocessor, which clang emulates in Microsoft-compatibility mode. Looking into clang's source I found the following:

lib/Lex/TokenLexer.cpp#131

// In Microsoft-compatibility mode, a comma is removed in the expansion // of " ... , VA_ARGS " if VA_ARGS is empty. This extension is // not supported by gcc. if (!HasPasteOperator && !PP.getLangOpts().MSVCCompat) return false;

If I'm not mistaken this means that this macro:

define SBT_A(...) (SBT_GET_REM(VA_ARGS), VA_ARGS)() SBT_B

when called with no parameters, will expand to this:

define SBT_A(...) (SBT_GET_REM())() SBT_B

instead of this (note the comma):

define SBT_A(...) (SBT_GET_REM(), )() SBT_B

b1622bb8-0285-400e-ba44-ba0e5027e546 commented 10 years ago

It may be specific to the Windows build of clang using Visual Studio. It cannot have anything to do with running under Windows per se AFAICS.

It would be nice if someone on Windows would:

1) Use CMake to generate the "Visual Studio 12 2013" solution 2) Build the release version of clang using the solution file generated by CMake. 3) Test the release version of cmake passing the parameters as specified in my original report.

This would offer verification that ny testing is correct and this is a bug somewhere in the generation of clang-cl.exe. Of course finding and fixing the bug is the most important thing.

llvmbot commented 10 years ago

Preprocessing this with clang r212447 (trunk from Monday) on Mac OS X results in:

% ~/LLVM/build/Release+Asserts/bin/clang -v -E clang.cpp clang version 3.5.0 (trunk 212447) [...] int main() { ( , NIL ) return 0; }

So this seems to be Windows-specific.