Skip to content

Commit

Permalink
Set kernel default visibility to hidden (#3060)
Browse files Browse the repository at this point in the history
Summary:

When we compile the kernel into a shared library, we don't know whether the definition of kernel implementation symbol can be dropped or not based on op registry. The kernel itself is just a normal function and the user can find it. We set its visibility to hidden by default. Then these kernels are gone when we do `objdump -TC`

This reduces binary size.

---

This is not done in fbcode so far. When we compile in fbcode, seems that all dependency libraries is compiled into shared library, not static library. For example, op tests depends on op implementation through shared library. In that case, the hidden symbols are not exposed and could cause link time failure.

In xplat, these dependencies are set to static libraries so it has no impact. Only when we explicitly build a shared library (for android), we hide the symbols and rely on op registry to store the impl.

---

This applies to internal build only for now. We will re-visit this for OSS later. It's a step needed to make use of selective build for building shared library (android use case mainly)

Reviewed By: dbort

Differential Revision: D56167833
  • Loading branch information
kirklandsign authored and facebook-github-bot committed Apr 16, 2024
1 parent c73bfc0 commit e411228
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 3 deletions.
2 changes: 1 addition & 1 deletion shim/xplat/executorch/build/env_interface.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ env = struct(
# @lint-ignore BUCKLINT: native and fb_native are explicitly forbidden in fbcode.
genrule = native.genrule,
is_oss = True,
is_xplat = False,
is_xplat = lambda: False,
patch_deps = _patch_deps,
patch_cxx_compiler_flags = _patch_cxx_compiler_flags,
patch_executorch_genrule_cmd = _patch_executorch_genrule_cmd,
Expand Down
14 changes: 12 additions & 2 deletions shim/xplat/executorch/kernels/portable/op_registration_util.bzl
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
load("@fbsource//xplat/executorch/build:runtime_wrapper.bzl", "runtime")
load("@fbsource//xplat/executorch/build:runtime_wrapper.bzl", "is_xplat", "runtime")
load("@fbsource//xplat/executorch/build:selects.bzl", "selects")

def op_target(name, deps = [], android_deps = [], _allow_third_party_deps = False, _aten_mode_deps = []):
Expand Down Expand Up @@ -122,7 +122,17 @@ def define_op_library(name, deps, android_deps, aten_target, _allow_third_party_
fbandroid_platform_deps = android_deps,
# kernels often have helpers with no prototypes just disabling the warning here as the headers
# are codegend and linked in later
compiler_flags = ["-Wno-missing-prototypes"],
compiler_flags = ["-Wno-missing-prototypes"] + (
# For shared library build, we don't want to expose symbols of
# kernel implementation (ex torch::executor::native::tanh_out)
# to library users. They should use kernels through registry only.
# With visibility=hidden, linker won't expose kernel impl symbols
# so it can prune unregistered kernels.
# Currently fbcode linkes all dependent libraries through shared
# library, and it blocks users like unit tests to use kernel
# implementation directly. So we enable this for xplat only.
["-fvisibility=hidden"] if is_xplat() else []
),
deps = [
"//executorch/runtime/kernel:kernel_includes" + aten_suffix,
] + deps,
Expand Down

0 comments on commit e411228

Please sign in to comment.