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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: clang/include/clang/Basic/Builtins.def:1059
+#undef strcasecmp
+#undef strncasecmp
+
----------------
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.


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


================
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
----------------
You shouldn't need to prefix warning flags with `/clang:`, those are supported out of the box.


================
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"
 
----------------
Unrelated to your change, but is this stale?


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

https://reviews.llvm.org/D112399