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

[Geoffrey Martin-Noble via Phabricator via cfe-commits]

GMNGeoffrey added inline comments.


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


================
Comment at: utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl:33
+    for impl_name in [dll_name, dylib_name, so_name]:
+        cc_binary(
+            name = impl_name,
----------------
chandlerc wrote:
> GMNGeoffrey wrote:
> > What about this makes `binary_alias` no longer work?
> The `output_group` in the subsequent `filegroup`. It requires the actions in the sources to have a correctly named `output_group`, which `cc_binary` with `linkshared` does.
> 
> It's possible that I could just rename things enough by adding multiple layers of file groups and maybe some added "copy this file" rules... But I'm worried that wouldn't work well long term. For example, it seems reasonable for the `.lib` file to *name* the `.dll` file that should be loaded. And so if we build it with `cc_binary` that has the wrong name, I'm worried things won't work as expected. I have higher confidence in this arrangement where the `cc_binary` directly produces the desired name.
> 
> Something like that actually happened when I tried a different way of wiring this up that still used the `binary_alias` on Linux -- it ended up actually loading `liblibclang_impl.so` and not `libclang.so`. That was a different setup, so maaaybe I wouldn't hit that exact situation, but its the kind of thing that inclined me towards this model.
That's sufficiently convincing. Hopefully some layer will figure out these are all the same and not build the same thing 3 times 🤞


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

https://reviews.llvm.org/D112399