[PATCH] D156095: [Bazel] Introduce `//compiler-rt:profile`
NAKAMURA Takumi via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 24 18:07:10 PDT 2023
chapuni added a reviewer: aaronmondal.
chapuni added a comment.
Thanks for feedbacks.
In D156095#4529073 <https://reviews.llvm.org/D156095#4529073>, @aaronmondal wrote:
> FYI I have buildfiles for a bunch of the compiler-rt targets at https://github.com/eomii/rules_ll/blob/main/llvm-project-overlay/compiler-rt/lib/profile/BUILD.bazel
>
> I'd a bit hesitant to build those with the potentially very old host toolchain since these targets are linked into non clang-toolchain executables. For something like profiling this might not be as impactful as building `libcxx` with an old toolchain but I'm a bit worried that this could lead to issues when using bootstrapped toolchains.
I assume the host toolchain should be capable of building runtimes for bootstrapping.
[RFC] RUNTIMES may be built in-tree and be linked to tools
https://discourse.llvm.org/t/rfc-runtimes-may-be-built-in-tree-and-be-linked-to-tools/70357
`rules_ll` itself is interesting. I will read and play it later. Thanks.
================
Comment at: utils/bazel/llvm-project-overlay/compiler-rt/BUILD.bazel:16
+ "COMPILER_RT_HAS_ATOMICS=1",
+ "COMPILER_RT_HAS_FCNTL_LCK=1",
+ "COMPILER_RT_HAS_UNAME=1",
----------------
MaskRay wrote:
> Windows doesn't define COMPILER_RT_HAS_FCNTL_LCK or COMPILER_RT_HAS_UNAME
I haven't introduced conditions since I don't use other environments. Do you think I could use `select()` with minimal conditions here? Could we conditionalize defs individually?
================
Comment at: utils/bazel/llvm-project-overlay/compiler-rt/BUILD.bazel:24
+ srcs = glob([
+ "lib/profile/*.c",
+ "lib/profile/*.h",
----------------
MaskRay wrote:
> We need `lib/profile/*.cpp` for `InstrProfilingRuntime.cpp`
I missed it. Thanks. I have confirmed it can be built (and identical to CMake's in my environment).
================
Comment at: utils/bazel/llvm-project-overlay/compiler-rt/BUILD.bazel:37
+ "-Wno-pedantic",
+ "-Wno-strict-prototypes",
+ ],
----------------
aeubanks wrote:
> MaskRay wrote:
> > Are the two -Wno- needed?
> should just fix the sources instead if these are finding real issues
`-Wno-strict-prototypes` was redundant. It has been there since I checked `-std=c11` for `strdup`.
`-Wno-pedantic` is required since my environment enforces `-pedantic`. I know compiler-rt doesn't like `-pedantic`. I will remove `-pedantic` in my environment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156095/new/
https://reviews.llvm.org/D156095
More information about the llvm-commits
mailing list