[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0198d76e1e76: [Bazel] Get `//clang` building on Windows with
clang-cl. (authored by chandlerc).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D112399?vs=383634=383951#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112399/new/
https://reviews.llvm.org/D112399
Files:
utils/bazel/.bazelrc
utils/bazel/llvm-project-overlay/clang/BUILD.bazel
utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel
utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
utils/bazel/llvm-project-overlay/llvm/config.bzl
utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
Index: utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
===
--- utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
+++ utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
@@ -352,10 +352,10 @@
#define HAVE_STD_IS_TRIVIALLY_COPYABLE 1
/* Define to a function implementing stricmp */
-/* stricmp defined in Bazel */
+/* stricmp defined conditionally below. */
/* Define to a function implementing strdup */
-/* strdup defined in Bazel */
+/* strdup defined conditionally below. */
/* Whether GlobalISel rule coverage is being collected */
#define LLVM_GISEL_COV_ENABLED 0
@@ -368,4 +368,17 @@
/* HAVE_PROC_PID_RUSAGE defined in Bazel */
+/* Directly provide definitions here behind platform preprocessor definitions.
+ * The preprocessor conditions are sufficient to handle all of the configuration
+ * on platforms targeted by Bazel, and defining these here more faithfully
+ * matches how the users of this header expect things to work with CMake.
+ * FIXME: We should consider moving other platform defines to use this technique
+ * as well.
+ */
+
+#ifdef _WIN32
+#define stricmp _stricmp
+#define strdup _strdup
+#endif
+
#endif
Index: utils/bazel/llvm-project-overlay/llvm/config.bzl
===
--- utils/bazel/llvm-project-overlay/llvm/config.bzl
+++ utils/bazel/llvm-project-overlay/llvm/config.bzl
@@ -57,9 +57,15 @@
]
win32_defines = [
-# MSVC specific
-"stricmp=_stricmp",
-"strdup=_strdup",
+# Windows system library specific defines.
+"_CRT_SECURE_NO_DEPRECATE",
+"_CRT_SECURE_NO_WARNINGS",
+"_CRT_NONSTDC_NO_DEPRECATE",
+"_CRT_NONSTDC_NO_WARNINGS",
+"_SCL_SECURE_NO_DEPRECATE",
+"_SCL_SECURE_NO_WARNINGS",
+"UNICODE",
+"_UNICODE",
# LLVM features
r'LTDL_SHLIB_EXT=\".dll\"',
Index: utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
===
--- utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
+++ utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
@@ -4,51 +4,72 @@
"""A macro to produce a loadable plugin library for the target OS.
-This macro produces a `cc_binary` rule with the name `name + "_impl"`. It
-forces the rule to statically link in its dependencies but to be linked as a
-shared "plugin" library. It then creates binary aliases to `.so`, `.dylib`
-,and `.dll` suffixed names for use on various platforms and selects between
-these into a filegroup with the exact name passed to the macro.
+This macro produces a set of platform-specific `cc_binary` rules, by appending
+the platform suffix (`.dll`, `.dylib`, or `.so`) to the provided `name`. It then
+connects these to a `cc_import` rule with `name` exactly and `hdrs` that can be
+used by other Bazel rules to depend on the plugin library.
+
+The `srcs` attribute for the `cc_binary` rules is `srcs + hdrs`. Other explicit
+arguments are passed to all of the rules where they apply, and can be used to
+configure generic aspects of all generated rules such as `testonly`. Lastly,
+`kwargs` is expanded into all the `cc_binary` rules.
"""
-load("@rules_cc//cc:defs.bzl", "cc_binary")
-load(":binary_alias.bzl", "binary_alias")
+load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_import", "cc_library")
-def cc_plugin_library(name, **kwargs):
+def cc_plugin_library(name, srcs, hdrs, include_prefix = None, strip_include_prefix = None, alwayslink = False, features = [], tags = [], testonly = False, **kwargs):
# Neither the name of the plugin binary nor tags on whether it is built are
-# configurable. Instead, we build a `cc_binary` that implements the plugin
-# library using a `_impl` suffix. Bazel will use appropriate flags to cause
-# this file to be a plugin library regardless of its name. We then create
-# binary aliases in the different possible platform names, and select
-# between these different names into a filegroup. The macro's name becomes
-
[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
chandlerc added inline comments.
Comment at: utils/bazel/llvm-project-overlay/clang/BUILD.bazel:1828
],
-copts = [
-"-Wno-uninitialized",
-],
+copts = select({
+"@bazel_tools//src/conditions:windows": [],
rnk wrote:
> Enabling warnings is good, but what made this change necessary? The `-Wno-`
> flag should have worked with clang-cl. If this change isn't necessary, maybe
> go ahead and delete this copt in a separate commit.
Restored.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112399/new/
https://reviews.llvm.org/D112399
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
chandlerc added a comment. Thanks, making suggested changes and then landing! Comment at: utils/bazel/llvm-project-overlay/clang/BUILD.bazel:364 +# Clang-specific define on non-Windows platforms. +"CLANG_HAVE_RLIMITS=1", +], GMNGeoffrey wrote: > Seems like this should be in clang/config.h? Ah, yeah. Might as well. Comment at: utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel:485 +"@bazel_tools//src/conditions:windows": [ +# Need to disable the VFS tests that don't use Windows friendly paths. +"--gtest_filter=-*VirtualFileOverlay*", GMNGeoffrey wrote: > Probably good to note here that these are also disabled in the CMake build Good idea, will do. Comment at: utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h:375 + * matches how the users of this header expect things to work with CMake. + * FIXME: We should consider moving other platform defines to use this technique + * as well. GMNGeoffrey wrote: > Oh nice, yeah this seems like a reasonable approach. I can't believe we > didn't think of this before... The less Bazel goop the better. We're still > going to need some way to deal with the things that are more complicated than > just platform and some way to handle keeping this up to date. That might push > us back to doing it with Bazel. IDK Well, *all* of Abseil's configuration is now done this way I think... so I've some optimism. Anyways, I'll send subsequent patches when I get some time. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112399/new/ https://reviews.llvm.org/D112399 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
GMNGeoffrey accepted this revision. GMNGeoffrey added inline comments. This revision is now accepted and ready to land. Comment at: utils/bazel/llvm-project-overlay/clang/BUILD.bazel:364 +# Clang-specific define on non-Windows platforms. +"CLANG_HAVE_RLIMITS=1", +], Seems like this should be in clang/config.h? Comment at: utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel:485 +"@bazel_tools//src/conditions:windows": [ +# Need to disable the VFS tests that don't use Windows friendly paths. +"--gtest_filter=-*VirtualFileOverlay*", Probably good to note here that these are also disabled in the CMake build Comment at: utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h:375 + * matches how the users of this header expect things to work with CMake. + * FIXME: We should consider moving other platform defines to use this technique + * as well. Oh nice, yeah this seems like a reasonable approach. I can't believe we didn't think of this before... The less Bazel goop the better. We're still going to need some way to deal with the things that are more complicated than just platform and some way to handle keeping this up to date. That might push us back to doing it with Bazel. IDK CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112399/new/ https://reviews.llvm.org/D112399 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
rnk added inline comments.
Comment at: utils/bazel/.bazelrc:81
+build:windows --copt=/Oi --host_copt=/Oi
+build:windows --cxxopt=/Zc:rvalueCast --host_cxxopt=/Zc:rvalueCast
+
Try adding `/permissive-` to get more conforming behavior from clang-cl. If
that doesn't work, try `/Zc:twoPhase-`, aka `-fno-delayed-template-parsing`.
Comment at: utils/bazel/.bazelrc:84
+# Use the more flexible bigobj format for C++ files that have lots of symbols.
+build:windows --cxxopt=/bigobj --host_cxxopt=/bigobj
This is only necessary for MSVC. LLVM MC auto-detects when bigobj is needed.
IMO, less flags is always better.
Comment at: utils/bazel/llvm-project-overlay/clang/BUILD.bazel:1828
],
-copts = [
-"-Wno-uninitialized",
-],
+copts = select({
+"@bazel_tools//src/conditions:windows": [],
Enabling warnings is good, but what made this change necessary? The `-Wno-`
flag should have worked with clang-cl. If this change isn't necessary, maybe go
ahead and delete this copt in a separate commit.
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"
chandlerc wrote:
> 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? ;]
Nope, ship it.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112399/new/
https://reviews.llvm.org/D112399
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
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 ` 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
chandlerc updated this revision to Diff 383634.
chandlerc marked 5 inline comments as done.
chandlerc edited the summary of this revision.
chandlerc added a reviewer: rnk.
chandlerc added a comment.
Major update to better fix some of the issues here. No longer requires any
changes outside of Bazel.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112399/new/
https://reviews.llvm.org/D112399
Files:
utils/bazel/.bazelrc
utils/bazel/llvm-project-overlay/clang/BUILD.bazel
utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel
utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
utils/bazel/llvm-project-overlay/llvm/config.bzl
utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
Index: utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
===
--- utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
+++ utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h
@@ -352,10 +352,10 @@
#define HAVE_STD_IS_TRIVIALLY_COPYABLE 1
/* Define to a function implementing stricmp */
-/* stricmp defined in Bazel */
+/* stricmp defined conditionally below. */
/* Define to a function implementing strdup */
-/* strdup defined in Bazel */
+/* strdup defined conditionally below. */
/* Whether GlobalISel rule coverage is being collected */
#define LLVM_GISEL_COV_ENABLED 0
@@ -368,4 +368,17 @@
/* HAVE_PROC_PID_RUSAGE defined in Bazel */
+/* Directly provide definitions here behind platform preprocessor definitions.
+ * The preprocessor conditions are sufficient to handle all of the configuration
+ * on platforms targeted by Bazel, and defining these here more faithfully
+ * matches how the users of this header expect things to work with CMake.
+ * FIXME: We should consider moving other platform defines to use this technique
+ * as well.
+ */
+
+#ifdef _WIN32
+#define stricmp _stricmp
+#define strdup _strdup
+#endif
+
#endif
Index: utils/bazel/llvm-project-overlay/llvm/config.bzl
===
--- utils/bazel/llvm-project-overlay/llvm/config.bzl
+++ utils/bazel/llvm-project-overlay/llvm/config.bzl
@@ -57,9 +57,15 @@
]
win32_defines = [
-# MSVC specific
-"stricmp=_stricmp",
-"strdup=_strdup",
+# Windows system library specific defines.
+"_CRT_SECURE_NO_DEPRECATE",
+"_CRT_SECURE_NO_WARNINGS",
+"_CRT_NONSTDC_NO_DEPRECATE",
+"_CRT_NONSTDC_NO_WARNINGS",
+"_SCL_SECURE_NO_DEPRECATE",
+"_SCL_SECURE_NO_WARNINGS",
+"UNICODE",
+"_UNICODE",
# LLVM features
r'LTDL_SHLIB_EXT=\".dll\"',
Index: utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
===
--- utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
+++ utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
@@ -4,51 +4,72 @@
"""A macro to produce a loadable plugin library for the target OS.
-This macro produces a `cc_binary` rule with the name `name + "_impl"`. It
-forces the rule to statically link in its dependencies but to be linked as a
-shared "plugin" library. It then creates binary aliases to `.so`, `.dylib`
-,and `.dll` suffixed names for use on various platforms and selects between
-these into a filegroup with the exact name passed to the macro.
+This macro produces a set of platform-specific `cc_binary` rules, by appending
+the platform suffix (`.dll`, `.dylib`, or `.so`) to the provided `name`. It then
+connects these to a `cc_import` rule with `name` exactly and `hdrs` that can be
+used by other Bazel rules to depend on the plugin library.
+
+The `srcs` attribute for the `cc_binary` rules is `srcs + hdrs`. Other explicit
+arguments are passed to all of the rules where they apply, and can be used to
+configure generic aspects of all generated rules such as `testonly`. Lastly,
+`kwargs` is expanded into all the `cc_binary` rules.
"""
-load("@rules_cc//cc:defs.bzl", "cc_binary")
-load(":binary_alias.bzl", "binary_alias")
+load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_import", "cc_library")
-def cc_plugin_library(name, **kwargs):
+def cc_plugin_library(name, srcs, hdrs, include_prefix = None, strip_include_prefix = None, alwayslink = False, features = [], tags = [], testonly = False, **kwargs):
# Neither the name of the plugin binary nor tags on whether it is built are
-# configurable. Instead, we build a `cc_binary` that implements the plugin
-# library using a `_impl` suffix. Bazel will use appropriate flags to cause
-# this file to be a plugin library regardless of its name. We then create
-# binary aliases in the different possible platform names, and select
-# between these different names into a filegroup. The macro's name becomes
-# the filegroup name and it contains exactly one target that is the target
-
[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
GMNGeoffrey added inline comments. 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" 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... CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112399/new/ https://reviews.llvm.org/D112399 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
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 > > > ` 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
thakis added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:1059 +#undef strcasecmp +#undef strncasecmp + 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 > > ` 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.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112399/new/ https://reviews.llvm.org/D112399 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
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 > ` 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
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 ` 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
thakis added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:1059 +#undef strcasecmp +#undef strncasecmp + Why do we need this with bazel but not with other windows builds? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112399/new/ https://reviews.llvm.org/D112399 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
GMNGeoffrey accepted this revision. GMNGeoffrey added inline comments. This revision is now accepted and ready to land. 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 */ Hmm I think we probably should have a change-detector for this config as well as the LLVM ones 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, What about this makes `binary_alias` no longer work? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112399/new/ https://reviews.llvm.org/D112399 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.
chandlerc created this revision.
chandlerc added a reviewer: GMNGeoffrey.
Herald added a subscriber: mcrosier.
chandlerc requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.
This required substantially more invasive changes I'm afraid.
First, there is an issue with a Clang header that isn't really robust on
Windows unless you get the includes *just* right. I've reworked it to be
a bit more defensive.
Second, I needed a different approach to get `libclang` working well.
This, IMO, improves things on all platforms. Now we build the plugin and
actually wrap it back up with `cc_import`. We have to use a collection
of manually tagged `cc_binary` rules to get the naming to work out the
right way, but this isn't too different from the prior approach. By
directly having a `cc_binary` rule for each platform spelling of
`libclang`, we can actually extract the interface library from it and
correctly depend on it with `cc_import`. I think the result now is much
closer to the intent and to the CMake build for libclang.
Last but not least, some tests needed disabling. This is actually
narrower than what CMake does. The issue isn't indicative of anything
serious -- the test just assumes Unix-style paths.
https://reviews.llvm.org/D112399
Files:
clang/include/clang/Basic/Builtins.def
utils/bazel/.bazelrc
utils/bazel/llvm-project-overlay/clang/BUILD.bazel
utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
utils/bazel/llvm-project-overlay/clang/unittests/BUILD.bazel
utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
Index: utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
===
--- utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
+++ utils/bazel/llvm-project-overlay/llvm/cc_plugin_library.bzl
@@ -4,51 +4,72 @@
"""A macro to produce a loadable plugin library for the target OS.
-This macro produces a `cc_binary` rule with the name `name + "_impl"`. It
-forces the rule to statically link in its dependencies but to be linked as a
-shared "plugin" library. It then creates binary aliases to `.so`, `.dylib`
-,and `.dll` suffixed names for use on various platforms and selects between
-these into a filegroup with the exact name passed to the macro.
+This macro produces a set of platform-specific `cc_binary` rules, by appending
+the platform suffix (`.dll`, `.dylib`, or `.so`) to the provided `name`. It then
+connects these to a `cc_import` rule with `name` exactly and `hdrs` that can be
+used by other Bazel rules to depend on the plugin library.
+
+The `srcs` attribute for the `cc_binary` rules is `srcs + hdrs`. Other explicit
+arguments are passed to all of the rules where they apply, and can be used to
+configure generic aspects of all generated rules such as `testonly`. Lastly,
+`kwargs` is expanded into all the `cc_binary` rules.
"""
-load("@rules_cc//cc:defs.bzl", "cc_binary")
-load(":binary_alias.bzl", "binary_alias")
+load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_import", "cc_library")
-def cc_plugin_library(name, **kwargs):
+def cc_plugin_library(name, srcs, hdrs, include_prefix = None, strip_include_prefix = None, alwayslink = False, features = [], tags = [], testonly = False, **kwargs):
# Neither the name of the plugin binary nor tags on whether it is built are
-# configurable. Instead, we build a `cc_binary` that implements the plugin
-# library using a `_impl` suffix. Bazel will use appropriate flags to cause
-# this file to be a plugin library regardless of its name. We then create
-# binary aliases in the different possible platform names, and select
-# between these different names into a filegroup. The macro's name becomes
-# the filegroup name and it contains exactly one target that is the target
-# platform suffixed plugin library.
+# configurable. Instead, we build a `cc_binary` with each name and
+# selectively depend on them based on platform.
#
# All-in-all, this is a pretty poor workaround. I think this is part of the
# Bazel issue: https://github.com/bazelbuild/bazel/issues/7538
-cc_binary(
-name = name + "_impl",
-linkshared = True,
-linkstatic = True,
-**kwargs
-)
-binary_alias(
-name = name + ".so",
-binary = ":" + name + "_impl",
-)
-binary_alias(
-name = name + ".dll",
-binary = ":" + name + "_impl",
-)
-binary_alias(
-name = name + ".dylib",
-binary = ":" + name + "_impl",
-)
+so_name = name + ".so"
+dll_name = name + ".dll"
+dylib_name = name + ".dylib"
+interface_output_name = name + "_interface_output"
+import_name = name + "_import"
+for impl_name in [dll_name, dylib_name, so_name]:
+cc_binary(
+name = impl_name,
+srcs = srcs + hdrs,
+linkshared = True,
+
