Skip to content
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] default enable -ftrivial-auto-var-init=pattern #78776

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

nickdesaulniers
Copy link
Member

Usage of uninitialized memory is a top memory safety issue in C++ codebases.
Help mitigate this somewhat by default initialize stack allocations to a
pattern (0xAA repeating).

Clang has received optimizations to sink these into control flow paths that
access such values to minimize the overhead of these added initializations.

If there's a measurable slowdown, we can add
-ftrivial-auto-var-init-max-size= for some value N bytes if we have any
large stack allocations, or add attribute uninitialized to any variable
declarations.

Unsupported until GCC 12.1 / Clang 8.

Increases file size of libc.a from a full build by +8.79Ki (+0.2%).

Usage of uninitialized memory is a top memory safety issue in C++ codebases.
Help mitigate this somewhat by default initialize stack allocations to a
pattern (0xAA repeating).

Clang has received optimizations to sink these into control flow paths that
access such values to minimize the overhead of these added initializations.

If there's a measurable slowdown, we can add
-ftrivial-auto-var-init-max-size=<N> for some value N bytes if we have any
large stack allocations, or add attribute uninitialized to any variable
declarations.

Unsupported until GCC 12.1 / Clang 8.

Increases file size of libc.a from a full build by +8.79Ki (+0.2%).
@llvmbot llvmbot added the libc label Jan 19, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 19, 2024

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

Usage of uninitialized memory is a top memory safety issue in C++ codebases.
Help mitigate this somewhat by default initialize stack allocations to a
pattern (0xAA repeating).

Clang has received optimizations to sink these into control flow paths that
access such values to minimize the overhead of these added initializations.

If there's a measurable slowdown, we can add
-ftrivial-auto-var-init-max-size=<N> for some value N bytes if we have any
large stack allocations, or add attribute uninitialized to any variable
declarations.

Unsupported until GCC 12.1 / Clang 8.

Increases file size of libc.a from a full build by +8.79Ki (+0.2%).


Full diff: https://github.com/llvm/llvm-project/pull/78776.diff

2 Files Affected:

  • (modified) libc/cmake/modules/LLVMLibCObjectRules.cmake (+5)
  • (modified) utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl (+6-1)
diff --git a/libc/cmake/modules/LLVMLibCObjectRules.cmake b/libc/cmake/modules/LLVMLibCObjectRules.cmake
index 6eba17ae9201c8..46f5ed5cd7465f 100644
--- a/libc/cmake/modules/LLVMLibCObjectRules.cmake
+++ b/libc/cmake/modules/LLVMLibCObjectRules.cmake
@@ -41,6 +41,11 @@ function(_get_common_compile_options output_var flags)
     list(APPEND compile_options "-fno-unwind-tables")
     list(APPEND compile_options "-fno-asynchronous-unwind-tables")
     list(APPEND compile_options "-fno-rtti")
+    # clang-8+, gcc-12+
+    check_cxx_compiler_flag("-ftrivial-auto-var-init=pattern" LIBC_CC_SUPPORTS_PATTERN_INIT)
+    if (LIBC_CC_SUPPORTS_PATTERN_INIT)
+      list(APPEND compile_options "-ftrivial-auto-var-init=pattern")
+    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 cf27001be9dfed..a28fa51a39f99d 100644
--- a/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl
+++ b/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl
@@ -81,7 +81,12 @@ def libc_function(
     # 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 []
-    copts = copts + ["-O3", "-fno-builtin", "-fno-lax-vector-conversions"]
+    copts = copts + [
+      "-O3",
+      "-fno-builtin",
+      "-fno-lax-vector-conversions",
+      "-ftrivial-auto-var-init=pattern"
+    ]
 
     # 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

@nickdesaulniers
Copy link
Member Author

FWIW, here's some measurements from bloaty: https://gist.github.com/nickdesaulniers/1092b3748b6810ae97053973c40b19a2.

The two cases of relatively large increases in code size:

  1. __llvm_libc_18_0_0_git::StrToNumResult<__llvm_libc_18_0_0_git::internal::ExpandedFloat<long double> > __llvm_libc_18_0_0_git::internal::hexadecimal_string_to_float<long double>(char const*, char, __llvm_libc_18_0_0_git::internal::RoundDirection)
  2. __llvm_libc_18_0_0_git::internal::FloatConvertReturn<long double> __llvm_libc_18_0_0_git::internal::decimal_exp_to_float<long double>(__llvm_libc_18_0_0_git::internal::ExpandedFloat<long double>, char const*, bool, __llvm_libc_18_0_0_git::internal::RoundDirection)

These look like their no longer being inlined due to increases in code size, perhaps:

  1. __llvm_libc_18_0_0_git::construct_thread_name_file_path(__llvm_libc_18_0_0_git::cpp::StringStream&, int)
  2. __llvm_libc_18_0_0_git::printf_core::FloatWriter::write_middle_block(unsigned int)

@@ -41,6 +41,11 @@ function(_get_common_compile_options output_var flags)
list(APPEND compile_options "-fno-unwind-tables")
list(APPEND compile_options "-fno-asynchronous-unwind-tables")
list(APPEND compile_options "-fno-rtti")
# clang-8+, gcc-12+
check_cxx_compiler_flag("-ftrivial-auto-var-init=pattern" LIBC_CC_SUPPORTS_PATTERN_INIT)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this check outside of this function, as it is called quite a lot? Either to the top of the file, or to somewhere in CheckCompilerFeatures.cmake.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 862dc3c

@nickdesaulniers
Copy link
Member Author

nickdesaulniers commented Jan 19, 2024

diff --git a/libc/src/__support/str_to_float.h b/libc/src/__support/str_to_float.h
index 8aeb3d2cea03..e8f0cb613b07 100644
--- a/libc/src/__support/str_to_float.h
+++ b/libc/src/__support/str_to_float.h
@@ -30,8 +30,8 @@ namespace LIBC_NAMESPACE {
 namespace internal {
 
 template <class T> struct ExpandedFloat {
-  typename fputil::FPBits<T>::StorageType mantissa;
-  int32_t exponent;
+  typename fputil::FPBits<T>::StorageType mantissa = {};
+  int32_t exponent = 0;
 };
 
 template <class T> struct FloatConvertReturn {

knocks out the second difference.

@nickdesaulniers nickdesaulniers merged commit 1d5c16d into llvm:main Jan 22, 2024
4 checks passed
@nickdesaulniers nickdesaulniers deleted the pattern_init branch January 22, 2024 22:55
nickdesaulniers added a commit to nickdesaulniers/llvm-project that referenced this pull request Apr 5, 2024
This would consistently fail for me locally, to the point where I could not run
`ninja libc-unit-tests` without `ninja libc_setjmp_unittests` failing.

Turns out that since I enabled -ftrivial-auto-var-init=pattern in
commit 1d5c16d ("[libc] default enable -ftrivial-auto-var-init=pattern (llvm#78776)")
this has been a problem. Our x86_64 setjmp definition disabled -Wuninitialized,
so we wound up clobbering these registers and instead backing up
0xAAAAAAAAAAAAAAAA rather than the actual register value.
nickdesaulniers added a commit to nickdesaulniers/llvm-project that referenced this pull request Apr 9, 2024
This fixes libc_setjmp_unittests for me.

This would consistently fail for me locally, to the point where I could not run
ninja libc-unit-tests without ninja libc_setjmp_unittests failing.

Turns out that since I enabled -ftrivial-auto-var-init=pattern in
commit 1d5c16d ("[libc] default enable -ftrivial-auto-var-init=pattern (llvm#78776)")
this has been a problem. Our x86_64 setjmp definition disabled -Wuninitialized,
so we wound up clobbering these registers and instead backing up
0xAAAAAAAAAAAAAAAA rather than the actual register value.

Adding out of line asm to llvm-libc is opening pandora's box. This should not
be merged without discussion and buy in from maintainers. We should have a
policy in place for _when_ it's acceptable to use out of line asm or not.

Link: llvm#87837
Link: llvm#88054
Link: https://discourse.llvm.org/t/hand-written-in-assembly-in-libc-setjmp-longjmp/73249
nickdesaulniers added a commit that referenced this pull request May 20, 2024
This would consistently fail for me locally, to the point where I could not run
ninja libc-unit-tests without ninja libc_setjmp_unittests failing.

Turns out that since I enabled -ftrivial-auto-var-init=pattern in
commit 1d5c16d ("[libc] default enable -ftrivial-auto-var-init=pattern (#78776)")
this has been a problem. Our x86_64 setjmp definition disabled -Wuninitialized,
so we wound up clobbering these registers and instead backing up
0xAAAAAAAAAAAAAAAA rather than the actual register value.

The implemenation should be rewritten entirely. I've proposed three different
ways to do so (linked below). Until we decide which way to go, at least disable
this hardening feature for this function for now so that the unit tests go back
to green.

Link: #87837
Link: #88054
Link: #88157
Fixes: #91164
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants