This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mikerice on May 7 2019, 11:03 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

mikerice created this revision.May 7 2019, 11:03 AM
rnk added inline comments.May 7 2019, 3:45 PM
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 updated this revision to Diff 198559.May 7 2019, 4:13 PM

Added comments as requested.

mikerice marked 2 inline comments as done.May 7 2019, 4:13 PM
rnk accepted this revision.May 7 2019, 4:19 PM

lgtm, thanks

This revision is now accepted and ready to land.May 7 2019, 4:19 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2019, 10:13 AM
thakis added a subscriber: thakis.May 8 2019, 11:37 AM

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

rnk added a comment.May 8 2019, 11:48 AM

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.

I agree with that.

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 added a subscriber: phosek.May 8 2019, 2:41 PM

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 added a comment.May 8 2019, 2:56 PM

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.

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 added a subscriber: echristo.May 9 2019, 3:20 PM

@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.