[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc requested review of this revision.
chandlerc added a comment.

PTAL, and thanks for feedback so far!



================
Comment at: clang/include/clang/Basic/Builtins.def:1059
+#undef strcasecmp
+#undef strncasecmp
+
----------------
rnk wrote:
> thakis wrote:
> > GMNGeoffrey wrote:
> > > chandlerc wrote:
> > > > thakis wrote:
> > > > > Why do we need this with bazel but not with other windows builds?
> > > > I don't know how this never was hit by other builders. I don't usually develop on Windows so I have very little experience with different builds there.
> > > > 
> > > > My guess is that it is the particular way that Bazel+clang-cl compile on Windows causes sligthtly more to be transitively included with `#include <windows.h>` and that grows the number of functions hit with this. I thought about trying to "fix" that, but the existing `#undef` lines made me think it wouldn't be completely successful.
> > > > 
> > > > Are you worried about the change? Looking for a different fix?
> > > Yeah I want to note that my review is basically only for the Bazel stuff. This looked fine to me based on the existing `undef`s, but mostly trusting Chandler to determine that this is OK and isn't in the territory of the Bazel build requiring unsavory things in the core project.
> > Worried is a strong word :) It just feels off. The redundant build system config changes shouldn't need changes to LLVM's code itself imho. If there's some difference in build behavior here, it feels like we should fix that on the build config side of things, else we'll have weird one-off fixes like this in other places potentially. I feel we should at least understand what's going on.
> > 
> > (This isn't a terribly strong opinion fwiw.)
> Does Bazel build clang with modules? That could lead to windows.h proliferation.
> 
> If you are motivated to investigate, you can re-run the failing compile command with /showIncludes to work out where the windows.h include comes in. Take the corresponding object file, build it with cmake ninja, extract the compile command there, run it with showincludes, and compare the output.
I figured this all out. It was right in front of me. New patch fixes.


================
Comment at: utils/bazel/.bazelrc:113-116
 # Use Clang's internal warning flags instead of the ones that sometimes map
 # through to MSVC's flags.
 build:clang-cl --copt=/clang:-Wall --host_copt=/clang:-Wall
 build:clang-cl --copt=/clang:-Werror --host_copt=/clang:-Werror
----------------
rnk wrote:
> It is true that MSVC's -Wall is Clang's -Weverything, so this needs a prefix. -Werror could be -WX, but it's just cosmetic.
Yeah, I just figured it was more readable in this section to consistently use `/clang:-W`.


================
Comment at: utils/bazel/.bazelrc:125
+# Disable some warnings hit even with `clang-cl` in Clang's own code.
+build:clang-cl --copt=/clang:-Wno-inconsistent-dllimport --host_copt=/clang:-Wno-inconsistent-dllimport
+build:clang-cl --cxxopt=/clang:-Wno-c++11-narrowing --host_cxxopt=/clang:-Wno-c++11-narrowing
----------------
rnk wrote:
> You shouldn't need to prefix warning flags with `/clang:`, those are supported out of the box.
Again, this just seemed more consistent. I can change if you think the other is better though.


================
Comment at: utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h:81
 /* The LLVM product name and version */
 #define BACKEND_PACKAGE_STRING "LLVM 12.0.0git"
 
----------------
GMNGeoffrey wrote:
> rnk wrote:
> > Unrelated to your change, but is this stale?
> Yes and we don't currently have a good way to keep it up to date. The overall problem of how to make sure we keep up to date with CMake configure knobs is unsolved. I wonder if we could run CMake in some limited capacity, but that's a whole can of worms...
I'll just commit an update to this separately. I assume that doesn't need separate review? ;]


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112399/new/

https://reviews.llvm.org/D112399