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

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added inline comments.


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


================
Comment at: utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h:78
 /* Define if we have sys/resource.h (rlimits) */
-#define CLANG_HAVE_RLIMITS 1
+/* CLANG_HAVE_RLIMITS defined in Bazel */
 
----------------
GMNGeoffrey wrote:
> Hmm I think we probably should have a change-detector for this config as well as the LLVM ones
Yeah, but didn't want to try to add that in this change.

IMO, the better thing would be for this to go away as it is almost entirely redundant already.... But agin, not this change....


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


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

https://reviews.llvm.org/D112399