From c84fa42ed8bc7f8cc6a0960e1f105e7cc1d40292 Mon Sep 17 00:00:00 2001 From: Hansong Zhang Date: Tue, 16 Apr 2024 15:42:18 -0700 Subject: [PATCH] Set kernel default visibility to hidden (#3060) 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 --- shim/xplat/executorch/build/env_interface.bzl | 2 +- .../kernels/portable/op_registration_util.bzl | 14 ++++++++++++-- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/shim/xplat/executorch/build/env_interface.bzl b/shim/xplat/executorch/build/env_interface.bzl index 6d82cc4e4b5..03e4ef7830d 100644 --- a/shim/xplat/executorch/build/env_interface.bzl +++ b/shim/xplat/executorch/build/env_interface.bzl @@ -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, diff --git a/shim/xplat/executorch/kernels/portable/op_registration_util.bzl b/shim/xplat/executorch/kernels/portable/op_registration_util.bzl index 5f9508fceb6..32a6da5260f 100644 --- a/shim/xplat/executorch/kernels/portable/op_registration_util.bzl +++ b/shim/xplat/executorch/kernels/portable/op_registration_util.bzl @@ -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 = []): @@ -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,