[clang][cli] Simplify repetitive macro invocations

[jansvoboda11]

Summary

Since we now only support Visual Studio 2019 16.7 and newer, we're able to use the /Zc:preprocessor flag that turns on the standards-conforming preprocessor. It (among other things) correctly expands __VA_ARGS__ (see https://learn.microsoft.com/en-us/cpp/preprocessor/preprocessor-experimental-overview?view=msvc-170#macro-arguments-are-unpacked). This enables us to get rid of some repetitive boilerplate in Clang's command-line parser/generator.

[jansvoboda11]

Actually add the MSVC flag after confirming the failure in CI.

tools\clang\include\clang/Driver/Options.inc(7003): warning C4003: not enough arguments for function-like macro invocation 'GENERATE_OPTION_WITH_MARSHALLING'
tools\clang\include\clang/Driver/Options.inc(7002): error C2059: syntax error: ')'
tools\clang\include\clang/Driver/Options.inc(7003): error C2143: syntax error: missing ';' before '{'
...

[jansvoboda11]

Squash the original macro with its _IMPL counterpart

[jansvoboda11]

Adding @thieta as a reviewer for the MSVC flag change, since it's possible due to his D122976 and D130689.

Note that this is essentially a re-commit of D95532.

[thieta]

This looks fine to me in principle. But I wonder if we should land the flag change first separately and make sure that no buildbots break because of it. Then we can merge the simplification a few days later when we are sure it's stabilized, since something similar happened when we upgraded to C++17 - and people already had merged code that used C++17 it was really hard to revert.

[jansvoboda11]

In D135128#3839312, @thieta wrote:

This looks fine to me in principle. But I wonder if we should land the flag change first separately and make sure that no buildbots break because of it. Then we can merge the simplification a few days later when we are sure it's stabilized, since something similar happened when we upgraded to C++17 - and people already had merged code that used C++17 it was really hard to revert.

That makes sense, I'll commit the flag change separately. Thanks!

[Bigcheese]

Looks good with the suggestion to split the change.

[stella.stamenova]

This caused some failures on the windows mlir buildbot: https://lab.llvm.org/buildbot/#/builders/13/builds/27829

[jansvoboda11]

In D135128#3902698, @stella.stamenova wrote:

This caused some failures on the windows mlir buildbot: https://lab.llvm.org/buildbot/#/builders/13/builds/27829

Thanks for the heads-up. Who would be the best person to look into this? Seems like this patch exposes an UB in Windows Kits (https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/c5105).

[stella.stamenova]

In D135128#3902772, @jansvoboda11 wrote:

In D135128#3902698, @stella.stamenova wrote:

This caused some failures on the windows mlir buildbot: https://lab.llvm.org/buildbot/#/builders/13/builds/27829

Thanks for the heads-up. Who would be the best person to look into this? Seems like this patch exposes an UB in Windows Kits (https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/c5105).

We can have a look - but let's revert this change in the meantime to make the bot green again. We'll try to have a fix or a workaround sometime next week.

[jansvoboda11]

Thanks, that sounds good. Reverting shortly.

[jansvoboda11]

@stella.stamenova Any updates?

[stella.stamenova]

In D135128#3921902, @jansvoboda11 wrote:

@stella.stamenova Any updates?

We haven't had time to come up with a proper solution yet. One short-term solution (assuming there's urgency in making the change) would be to temporarily disable treating warnings as errors on the mlir buildbot. This will remove the issue there and I don't know of other buildbots that enforce warning as error on Windows, so it will likely unblock the commit.

[jansvoboda11]

In D135128#3926404, @stella.stamenova wrote:

In D135128#3921902, @jansvoboda11 wrote:

@stella.stamenova Any updates?

We haven't had time to come up with a proper solution yet. One short-term solution (assuming there's urgency in making the change) would be to temporarily disable treating warnings as errors on the mlir buildbot. This will remove the issue there and I don't know of other buildbots that enforce warning as error on Windows, so it will likely unblock the commit.

It's not urgent to get this change in, but I also don't want to put it off for too long. Especially since only one bot failed (AFAIK).

[jansvoboda11]

@stella.stamenova ping

[stella.stamenova]

In D135128#4107816, @jansvoboda11 wrote:

@stella.stamenova ping

Thanks for the reminder!

I spent some time looking for a combination of VS + Win 10 SDK that would work with the new flag, and I believe I've found (at least one). It looks like versions of the Win 10 SDK prior to 20348 do not work, but 20348 works fine in local testing with the latest versions of VS 2019 -- some newer versions of VS have a crash in some of the mlir tests, so I needed to find the right version there too.

Long story short, I believe VS 2019 16.11.23 with Win 10 SDK 10.0.20348.0 should work fine based on some local tests. I've updated the bot to these versions and I'd like to give it a day or so to make sure something else didn't break. After that, the new flag should work.

[jansvoboda11]

In D135128#4108588, @stella.stamenova wrote:

In D135128#4107816, @jansvoboda11 wrote:

@stella.stamenova ping

Thanks for the reminder!

I spent some time looking for a combination of VS + Win 10 SDK that would work with the new flag, and I believe I've found (at least one). It looks like versions of the Win 10 SDK prior to 20348 do not work, but 20348 works fine in local testing with the latest versions of VS 2019 -- some newer versions of VS have a crash in some of the mlir tests, so I needed to find the right version there too.

Long story short, I believe VS 2019 16.11.23 with Win 10 SDK 10.0.20348.0 should work fine based on some local tests. I've updated the bot to these versions and I'd like to give it a day or so to make sure something else didn't break. After that, the new flag should work.

Thanks a lot! I'll give it a try tomorrow.

llvm/cmake/modules/HandleLLVMOptions.cmake

if( MSVC )
   endif()
 
   if (LLVM_ENABLE_WERROR)
     append("/WX" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
   endif (LLVM_ENABLE_WERROR)
 
   append("/Zc:inline" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
 
+  # Enable standards-conforming preprocessor.
+  # https://learn.microsoft.com/en-us/cpp/build/reference/zc-preprocessor
+  append("/Zc:preprocessor" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+
   # Some projects use the __cplusplus preprocessor macro to check support for
   # a particular version of the C++ standard. When this option is not specified
   # explicitly, macro's value is "199711L" that implies C++98 Standard.
   # https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/
   append("/Zc:__cplusplus" CMAKE_CXX_FLAGS)
 
   # Allow users to request PDBs in release mode. CMake offeres the
   # RelWithDebInfo configuration, but it uses different optimization settings