-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[libc] enable stack protectors and frame pointers on default #86288
Conversation
@llvm/pr-subscribers-libc Author: Schrodinger ZHU Yifan (SchrodingerZhu) ChangesOS distros tend to enable them by default:
Full diff: https://github.com/llvm/llvm-project/pull/86288.diff 3 Files Affected:
diff --git a/libc/CMakeLists.txt b/libc/CMakeLists.txt
index a0d79858a896ad..0c1fdae04cf955 100644
--- a/libc/CMakeLists.txt
+++ b/libc/CMakeLists.txt
@@ -47,6 +47,10 @@ set(LIBC_NAMESPACE ${default_namespace}
CACHE STRING "The namespace to use to enclose internal implementations. Must start with '__llvm_libc'."
)
+# Codegen options.
+option(LLVM_LIBC_KEEP_FRAME_POINTER "Keep frame pointers in LLVM libc" ON)
+option(LLVM_LIBC_ENABLE_STACK_PROTECTOR "Enable stack protector for LLVM libc" ON)
+
if(LLVM_LIBC_FULL_BUILD OR LLVM_LIBC_GPU_BUILD)
if(NOT LIBC_HDRGEN_EXE)
# We need to set up hdrgen first since other targets depend on it.
diff --git a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
index 5bc0898298ce39..df7311f8ec6d9d 100644
--- a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
+++ b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
@@ -60,6 +60,15 @@ function(_get_common_compile_options output_var flags)
if (LIBC_CC_SUPPORTS_PATTERN_INIT)
list(APPEND compile_options "-ftrivial-auto-var-init=pattern")
endif()
+ if (LLVM_LIBC_KEEP_FRAME_POINTER)
+ list(APPEND compile_options "-fno-omit-frame-pointer")
+ if (LIBC_TARGET_ARCHITECTURE_IS_X86)
+ list(APPEND compile_options "-mno-omit-leaf-frame-pointer")
+ endif()
+ endif()
+ if (LLVM_LIBC_ENABLE_STACK_PROTECTOR)
+ list(APPEND compile_options "-fstack-protector-strong")
+ endif()
list(APPEND compile_options "-Wall")
list(APPEND compile_options "-Wextra")
# -DLIBC_WNO_ERROR=ON if you can't build cleanly with -Werror.
diff --git a/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl b/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl
index 7d815bc4a2299c..7dc12bade2605a 100644
--- a/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl
+++ b/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl
@@ -78,7 +78,6 @@ def libc_function(
its deps.
**kwargs: Other attributes relevant for a cc_library. For example, deps.
"""
-
# We use the explicit equals pattern here because append and += mutate the
# original list, where this creates a new list and stores it in deps.
copts = copts or []
@@ -87,7 +86,15 @@ def libc_function(
"-fno-builtin",
"-fno-lax-vector-conversions",
"-ftrivial-auto-var-init=pattern",
+ "-fno-omit-frame-pointer",
+ "-fstack-protector-strong",
]
+ # x86 targets have -mno-omit-leaf-frame-pointer.
+ platform_copts = selects.with_or({
+ PLATFORM_CPU_X86_64: ["-mno-omit-leaf-frame-pointer"],
+ "//conditions:default": []
+ })
+ copts = copts + platform_copts
# We compile the code twice, the first target is suffixed with ".__internal__" and contains the
# C++ functions in the "LIBC_NAMESPACE" namespace. This allows us to test the function in the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stack protectors are just good practice. +1
I believe our internal customer that's using bazel is using frame pointers generally, so this LGTM.
Do we document these cmake options anywhere? Might be nice to mention them somewhere, so that folks don't have to trawl our cmake to find them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine for now, though in future it might be best to move this option to config.json so it can be set per platform.
a9d65c6
to
da20563
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
OS distros tend to enable them by default: