This is an archive of the discontinued LLVM Phabricator instance.

[Bazel] Introduce `//compiler-rt:profile`
ClosedPublic

Authored by chapuni on Jul 24 2023, 3:45 AM.

Details

Summary

At the moment, provide only profile at first.

Diff Detail

Event Timeline

chapuni created this revision.Jul 24 2023, 3:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 3:45 AM
Herald added subscribers: Enna1, dberris. · View Herald Transcript
chapuni requested review of this revision.Jul 24 2023, 3:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 3:45 AM
chapuni added a subscriber: paquette.
MaskRay added inline comments.Jul 24 2023, 10:26 AM
utils/bazel/llvm-project-overlay/compiler-rt/BUILD.bazel
16

Windows doesn't define COMPILER_RT_HAS_FCNTL_LCK or COMPILER_RT_HAS_UNAME

24

We need lib/profile/*.cpp for InstrProfilingRuntime.cpp

37

Are the two -Wno- needed?

MaskRay added a reviewer: Restricted Project.Jul 24 2023, 10:26 AM
aeubanks added inline comments.
utils/bazel/llvm-project-overlay/compiler-rt/BUILD.bazel
37

should just fix the sources instead if these are finding real issues

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.

paquette added inline comments.Jul 24 2023, 6:00 PM
utils/bazel/llvm-project-overlay/compiler-rt/BUILD.bazel
27
37

+1

Thanks for feedbacks.

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.

utils/bazel/llvm-project-overlay/compiler-rt/BUILD.bazel
16

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?

24

I missed it. Thanks. I have confirmed it can be built (and identical to CMake's in my environment).

37

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

paquette added inline comments.Jul 24 2023, 6:09 PM
utils/bazel/llvm-project-overlay/compiler-rt/BUILD.bazel
16

Looks like CMake checks if the target supports fcntl, atomics, ..., and then sets COMPILER_RT_HAS_<whatever> accordingly.

For example:

https://github.com/llvm/llvm-project/blob/c8bcc48af628d9f1e6b0cfd3edda330f3c04ed9a/compiler-rt/lib/profile/CMakeLists.txt#L99

I don't have any environments for targeting Win32 at the moment.
Do we think I should make this Win32-compatible?

utils/bazel/llvm-project-overlay/compiler-rt/BUILD.bazel
27

It is excluded for non-Win32 targets also in CMake.

chapuni added inline comments.Jul 24 2023, 6:20 PM
utils/bazel/llvm-project-overlay/compiler-rt/BUILD.bazel
16

AFAIK LLVM's Bazel build is less configurable. So I think something may rely on assumptions.
See also config.bzl and llvm/Config/config.h

chapuni updated this revision to Diff 543956.Jul 25 2023, 6:54 AM
chapuni marked 3 inline comments as done.
  • Add *.cpp to srcs
  • Remove copts
chapuni added inline comments.Jul 25 2023, 6:57 AM
utils/bazel/llvm-project-overlay/compiler-rt/BUILD.bazel
37

copts may be pruned if -pedantic is not given.

I don't have any environments for targeting Win32 at the moment.
Do we think I should make this Win32-compatible?

I think we should either

  1. Support Windows.

OR

  1. Emit a useful error ("Building libprofile on Windows using Bazel not yet supported") if we can't yet support Windows.
chapuni updated this revision to Diff 555197.Aug 31 2023, 4:45 PM
  • Will invoke error unless the platform is linux

At the moment, this can be built for targeting linux, or will raise error before building begins.

The error cannot be seen unless //compiler-rt:profile is specified as a target explicitly.

paquette accepted this revision.Sep 4 2023, 1:25 AM

I think this looks good. @MaskRay WDYT?

This revision is now accepted and ready to land.Sep 4 2023, 1:25 AM
chapuni added inline comments.Sep 4 2023, 1:47 AM
utils/bazel/llvm-project-overlay/compiler-rt/BUILD.bazel
35–36

Seems wrong format. I'll fix it later.

chapuni updated this revision to Diff 555718.Sep 4 2023, 5:53 AM
  • Reformat.
MaskRay accepted this revision.Sep 5 2023, 2:35 PM
This revision was landed with ongoing or failed builds.Sep 6 2023, 6:11 AM
This revision was automatically updated to reflect the committed changes.