Include corecrt.h/vcruntime.h to improve MS compatibility

[mikerice]

Summary

Include corecrt.h in stddef.h and vcruntime.h in stdarg.h to improve MS compatibility. This allows some applications developed with MSVC to compile with clang without any extra changes.

Fixes: llvm.org/PR40789

[rnk]

lib/Headers/stdarg.h

13 ↗

(On Diff #198501)

Please add a comment about why we're including this otherwise unneeded header.

lib/Headers/stddef.h

22 ↗

(On Diff #198501)

Please add a similar comment here.

[mikerice]

Added comments as requested.

[rnk]

lgtm, thanks

[thakis]

Did I get the bug right that this adds almost 400kB to every file that includes stddef.h?

[rnk]

In D61646#1495517, @thakis wrote:

Did I get the bug right that this adds almost 400kB to every file that includes stddef.h?

I didn't put much confidence in that number because it was with -frewrite-includes which adds line markers etc, but I did this and now I'm not sure I'm in favor anymore:

$ du -h /c/Program\ Files\ \(x86\)/Microsoft\ Visual\ Studio/2019/Professional/VC/Tools/MSVC/14.20.27508/include/sal.h
212K    /c/Program Files (x86)/Microsoft Visual Studio/2019/Professional/VC/Tools/MSVC/14.20.27508/include/sal.h

I'm guessing that the main dependency that's leaking out of msvc's stddef.h is in fact sal.h, but I think we really *don't* want to include that if we can avoid it. I think we'd be doing users a favor by keeping those macros out of their compilation if they don't need them.

[thakis]

I agree with that.

[mikerice]

For the most part, these headers are going to be included in almost every compilation unit anyway since they will come in through other C/C++ library headers. So this would presumably affect only a small numbers of compilation units that only include stddef.h and none of the others.

It’s been our experience that if a user has code that compiles fine with their current compiler, and it doesn’t compile with the new compiler, they just won’t use the new compiler. Many don’t really care that it would work fine if they added #includes where needed. So changes like this can really improve compiler uptake. I guess it is a trade-off between helping users easily moving their code to clang or a possible small compile time improvement.

[phosek]

This change broke our build which is using Clang and the x86_64-windows-msvc target to cross-compile our EFI bootloader. Now the compilation fails because it cannot find Windows headers (as expected). We're probably going to work around the issue by setting -U_MSC_VER, but I'm going to start a discussion whether we should maybe define a new target for EFI.

[rnk]

In D61646#1495778, @phosek wrote:

This change broke our build which is using Clang and the x86_64-windows-msvc target to cross-compile our EFI bootloader. Now the compilation fails because it cannot find Windows headers (as expected). We're probably going to work around the issue by setting -U_MSC_VER, but I'm going to start a discussion whether we should maybe define a new target for EFI.

We could fix forward by conditionalizing this under __has_include or something like that, but I think there's enough reasons to say that perhaps we don't want to do this after all. I'll go ahead and revert this and we can discuss it more.

In D61646#1495597, @mikerice wrote:

For the most part, these headers are going to be included in almost every compilation unit anyway since they will come in through other C/C++ library headers. So this would presumably affect only a small numbers of compilation units that only include stddef.h and none of the others.

This is true, and it's part of why I figured we should approve this. However, the fact that users actually encounter these issues suggests that there are actually files in the wild that only use stddef.h, so avoiding including sal.h for them might be worthwhile.

It’s been our experience that if a user has code that compiles fine with their current compiler, and it doesn’t compile with the new compiler, they just won’t use the new compiler. Many don’t really care that it would work fine if they added #includes where needed. So changes like this can really improve compiler uptake. I guess it is a trade-off between helping users easily moving their code to clang or a possible small compile time improvement.

That's certainly the case, but even considering clang's approach to GCC compatibility, we've always drawn the line somewhere around "system headers". We're willing to make changes to clang to accommodate code that the user cannot change: code from OS SDKs, glibc, Windows SDK, STL, etc, but we usually assume that the user is willing to put in some effort to make their code clang-compatible.

[rnk]

@echristo mentioned that this patch was also causing issues in another freestanding environment where _MSC_VER was defined, but no corecrt.h header existed on the system. So, I think it's likely that we don't want to do this in the long run. The compiler-provided headers and system headers are already too entangled, and this is one more dependency between them.

cfe/trunk/lib/Headers/stdarg.h

 /*===---- stdarg.h - Variable argument handling ----------------------------===
  *
  * Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
  * See https://llvm.org/LICENSE.txt for license information.
  * SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
  *
  *===-----------------------------------------------------------------------===
  */
 
 #ifndef __STDARG_H
 #define __STDARG_H
 
+#if defined(_MSC_VER)
+/* Include otherwise unneeded header for MSVC compatibility. */
+#include <vcruntime.h>
+#endif
+
 #ifndef _VA_LIST
 typedef __builtin_va_list va_list;
 #define _VA_LIST
 #endif
 #define va_start(ap, param) __builtin_va_start(ap, param)
 #define va_end(ap)          __builtin_va_end(ap)
 #define va_arg(ap, type)    __builtin_va_arg(ap, type)
 

cfe/trunk/lib/Headers/stddef.h

 
 #if !defined(__need_ptrdiff_t) && !defined(__need_size_t) &&                   \
     !defined(__need_wchar_t) && !defined(__need_NULL) &&                       \
     !defined(__need_wint_t)
 /* Always define miscellaneous pieces when modules are available. */
 #if !__has_feature(modules)
 #define __STDDEF_H
 #endif
+
+#if defined(_MSC_VER)
+/* Include otherwise unneeded header for MSVC compatibility. */
+#include <corecrt.h>
+#endif
+
 #define __need_ptrdiff_t
 #define __need_size_t
 #define __need_wchar_t
 #define __need_NULL
 #define __need_STDDEF_H_misc
 /* __need_wint_t is intentionally not defined here. */
 #endif
 

cfe/trunk/test/Headers/Inputs/ms-crt/corecrt.h

+#pragma once

cfe/trunk/test/Headers/Inputs/ms-crt/vcruntime.h

+#pragma once

cfe/trunk/test/Headers/c11.c

 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c11 %s
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c11 -fmodules -fmodules-cache-path=%t %s -D__STDC_WANT_LIB_EXT1__=1
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c11 -ffreestanding %s
-// RUN: %clang_cc1 -fsyntax-only -verify -std=c11 -triple i686-pc-win32 -fms-compatibility-version=17.00 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c11 -triple i686-pc-win32 \
+// RUN:  -fms-compatibility-version=17.00 -isystem %S/Inputs/ms-crt %s
 
 noreturn int f(); // expected-error 1+{{}}
 
 #include <stdnoreturn.h>
 #include <stdnoreturn.h>
 #include <stdnoreturn.h>
 
 int g();

cfe/trunk/test/Headers/ms-additional-includes.cpp

+// RUN: %clang_cc1 -fsyntax-only --show-includes -triple i686-pc-win32 \
+// RUN:  -isystem %S/Inputs/ms-crt -fms-compatibility-version=17.00 %s \
+// RUN:  | FileCheck %s
+
+#include <stddef.h>
+// CHECK: including file:{{.*}}stddef.h
+// CHECK: including file:{{.*}}corecrt.h
+#include <stdarg.h>
+// CHECK: including file:{{.*}}stdarg.h
+// CHECK: including file:{{.*}}vcruntime.h

cfe/trunk/test/Headers/ms-null-ms-header-vs-stddef.cpp

-// RUN: %clang_cc1 -fsyntax-only -triple i686-pc-win32 -fms-compatibility -fms-compatibility-version=17.00 %s
+// RUN: %clang_cc1 -fsyntax-only -triple i686-pc-win32 -fms-compatibility \
+// RUN:  -isystem %S/Inputs/ms-crt -fms-compatibility-version=17.00 %s
 // RUN: %clang_cc1 -fsyntax-only -triple i386-mingw32 %s
 
 // Something in MSVC's headers (pulled in e.g. by <crtdefs.h>) defines __null
 // to something, mimic that.
 #define __null
 
 #include <stddef.h>
 
 // __null is used as a type annotation in MS headers, with __null defined to
 // nothing in regular builds. This should continue to work even with stddef.h
 // included.
 void f(__null void* p) { }
 
 // NULL should work fine even with __null defined to nothing.
 void* p = NULL;