Skip to content

[libc][c23] add memset_explicit #83577

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

Merged
merged 6 commits into from
Mar 7, 2024

Conversation

SchrodingerZhu
Copy link
Contributor

@SchrodingerZhu SchrodingerZhu commented Mar 1, 2024

@llvmbot
Copy link
Member

llvmbot commented Mar 1, 2024

@llvm/pr-subscribers-libc

Author: Schrodinger ZHU Yifan (SchrodingerZhu)

Changes

Based on https://discourse.llvm.org/t/implement-memset-explicit/77312


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

16 Files Affected:

  • (modified) libc/cmake/modules/LLVMLibCCheckCpuFeatures.cmake (+24-7)
  • (added) libc/cmake/modules/cpu_features/check_CLFLUSHOPT.cpp (+6)
  • (modified) libc/config/linux/aarch64/entrypoints.txt (+1)
  • (modified) libc/config/linux/x86_64/entrypoints.txt (+1)
  • (modified) libc/spec/stdc.td (+5)
  • (modified) libc/src/string/CMakeLists.txt (+12)
  • (modified) libc/src/string/memory_utils/CMakeLists.txt (+20)
  • (added) libc/src/string/memory_utils/aarch64/flush_cacheline.h (+29)
  • (modified) libc/src/string/memory_utils/aarch64/inline_memset.h (+2-1)
  • (added) libc/src/string/memory_utils/flush_cache.h (+57)
  • (modified) libc/src/string/memory_utils/inline_memset.h (+8)
  • (added) libc/src/string/memory_utils/x86_64/flush_cacheline.h (+27)
  • (added) libc/src/string/memset_explicit.cpp (+27)
  • (added) libc/src/string/memset_explicit.h (+20)
  • (modified) libc/test/src/string/CMakeLists.txt (+10)
  • (added) libc/test/src/string/memset_explicit_test.cpp (+31)
diff --git a/libc/cmake/modules/LLVMLibCCheckCpuFeatures.cmake b/libc/cmake/modules/LLVMLibCCheckCpuFeatures.cmake
index 73b249374a0667..0225237cd7fa33 100644
--- a/libc/cmake/modules/LLVMLibCCheckCpuFeatures.cmake
+++ b/libc/cmake/modules/LLVMLibCCheckCpuFeatures.cmake
@@ -6,9 +6,11 @@
 set(ALL_CPU_FEATURES "")
 
 if(${LIBC_TARGET_ARCHITECTURE_IS_X86})
-  set(ALL_CPU_FEATURES SSE2 SSE4_2 AVX AVX2 AVX512F AVX512BW FMA)
+  set(ALL_CPU_FEATURES SSE2 SSE4_2 AVX AVX2 AVX512F AVX512BW FMA CLFLUSHOPT)
+  set(CPU_FEATURES_DETECT_REQUIRES_RUN "CLFLUSHOPT")
   set(LIBC_COMPILE_OPTIONS_NATIVE -march=native)
 elseif(${LIBC_TARGET_ARCHITECTURE_IS_AARCH64})
+  set(CPU_FEATURES_DETECT_REQUIRES_RUN "")
   set(LIBC_COMPILE_OPTIONS_NATIVE -mcpu=native)
 endif()
 
@@ -53,12 +55,27 @@ else()
   # Try compile a C file to check if flag is supported.
   set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
   foreach(feature IN LISTS ALL_CPU_FEATURES)
-    try_compile(
-      has_feature
-      ${CMAKE_CURRENT_BINARY_DIR}/cpu_features
-      SOURCES ${LIBC_SOURCE_DIR}/cmake/modules/cpu_features/check_${feature}.cpp
-      COMPILE_DEFINITIONS -I${LIBC_SOURCE_DIR} ${LIBC_COMPILE_OPTIONS_NATIVE}
-    )
+    if (${feature} IN_LIST CPU_FEATURES_DETECT_REQUIRES_RUN)
+      try_run(
+        return_code
+        can_compile
+        ${CMAKE_CURRENT_BINARY_DIR}/cpu_features
+        ${LIBC_SOURCE_DIR}/cmake/modules/cpu_features/check_${feature}.cpp
+        COMPILE_DEFINITIONS -I${LIBC_SOURCE_DIR} ${LIBC_COMPILE_OPTIONS_NATIVE}
+      )
+      if (can_compile AND return_code EQUAL 0)
+        set(has_feature TRUE)
+      else()
+        set(has_feature FALSE)
+      endif()
+    else()
+      try_compile(
+        has_feature
+        ${CMAKE_CURRENT_BINARY_DIR}/cpu_features
+        SOURCES ${LIBC_SOURCE_DIR}/cmake/modules/cpu_features/check_${feature}.cpp
+        COMPILE_DEFINITIONS -I${LIBC_SOURCE_DIR} ${LIBC_COMPILE_OPTIONS_NATIVE}
+      )
+    endif()
     if(has_feature)
       list(APPEND AVAILABLE_CPU_FEATURES ${feature})
     endif()
diff --git a/libc/cmake/modules/cpu_features/check_CLFLUSHOPT.cpp b/libc/cmake/modules/cpu_features/check_CLFLUSHOPT.cpp
new file mode 100644
index 00000000000000..2063cd82846016
--- /dev/null
+++ b/libc/cmake/modules/cpu_features/check_CLFLUSHOPT.cpp
@@ -0,0 +1,6 @@
+void test(char *ptr) { asm volatile("clflushopt %0" : "+m"(*ptr)::"memory"); }
+
+int main(int argc, char **argv) {
+  test(argv[0]);
+  return 0;
+}
\ No newline at end of file
diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index 06832a41221dd8..c32773f67cda53 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -51,6 +51,7 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.string.mempcpy
     libc.src.string.memrchr
     libc.src.string.memset
+    libc.src.string.memset_explicit
     libc.src.string.rindex
     libc.src.string.stpcpy
     libc.src.string.stpncpy
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index bc10512d942fa7..fef6a92d06aff1 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -51,6 +51,7 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.string.mempcpy
     libc.src.string.memrchr
     libc.src.string.memset
+    libc.src.string.memset_explicit
     libc.src.string.rindex
     libc.src.string.stpcpy
     libc.src.string.stpncpy
diff --git a/libc/spec/stdc.td b/libc/spec/stdc.td
index 94ac62966f3ba5..c8f26eb1e07e0d 100644
--- a/libc/spec/stdc.td
+++ b/libc/spec/stdc.td
@@ -234,6 +234,11 @@ def StdC : StandardSpec<"stdc"> {
               RetValSpec<VoidPtr>,
               [ArgSpec<VoidPtr>, ArgSpec<IntType>, ArgSpec<SizeTType>]
           >,
+          FunctionSpec<
+              "memset_explicit",
+              RetValSpec<VoidPtr>,
+              [ArgSpec<VoidPtr>, ArgSpec<IntType>, ArgSpec<SizeTType>]
+          >,
           FunctionSpec<
               "strcpy",
               RetValSpec<CharPtr>,
diff --git a/libc/src/string/CMakeLists.txt b/libc/src/string/CMakeLists.txt
index 1c893280e8a3c2..c169f9e99278e6 100644
--- a/libc/src/string/CMakeLists.txt
+++ b/libc/src/string/CMakeLists.txt
@@ -441,6 +441,18 @@ add_entrypoint_object(
     .memory_utils.inline_memcpy
 )
 
+add_entrypoint_object(
+  memset_explicit
+  SRCS
+    memset_explicit.cpp
+  HDRS
+    memset_explicit.h
+  DEPENDS
+    .string_utils
+    .memory_utils.inline_memset
+    .memory_utils.flush_cache
+)
+
 # Helper to define a function with multiple implementations
 # - Computes flags to satisfy required/rejected features and arch,
 # - Declares an entry point,
diff --git a/libc/src/string/memory_utils/CMakeLists.txt b/libc/src/string/memory_utils/CMakeLists.txt
index 08c0b0d34d5030..78b47a93bef73d 100644
--- a/libc/src/string/memory_utils/CMakeLists.txt
+++ b/libc/src/string/memory_utils/CMakeLists.txt
@@ -7,6 +7,7 @@ add_header_library(
     aarch64/inline_memcpy.h
     aarch64/inline_memmove.h
     aarch64/inline_memset.h
+    aarch64/flush_cacheline.h
     generic/aligned_access.h
     generic/byte_per_byte.h
     inline_bcmp.h
@@ -30,6 +31,7 @@ add_header_library(
     x86_64/inline_memcpy.h
     x86_64/inline_memmove.h
     x86_64/inline_memset.h
+    x86_64/flush_cacheline.h
   DEPENDS
     libc.src.__support.common
     libc.src.__support.CPP.bit
@@ -97,3 +99,21 @@ add_header_library(
   HDRS
     inline_memmem.h
 )
+
+if (CLFLUSHOPT IN_LIST LIBC_CPU_FEATURES)
+  set(clflushopt_option "-DLIBC_TARGET_CPU_HAS_CLFLUSHOPT")
+  message(STATUS "Using clflushopt for cacheline flushing")
+else()
+  set(clflushopt_option "")
+endif()
+
+add_header_library(
+  flush_cache
+  HDRS
+    flush_cache.h
+  COMPILE_OPTIONS
+    ${clflushopt_option}
+  DEPENDS
+    .memory_utils
+    libc.src.__support.CPP.atomic
+)
diff --git a/libc/src/string/memory_utils/aarch64/flush_cacheline.h b/libc/src/string/memory_utils/aarch64/flush_cacheline.h
new file mode 100644
index 00000000000000..5aaa58796bc437
--- /dev/null
+++ b/libc/src/string/memory_utils/aarch64/flush_cacheline.h
@@ -0,0 +1,29 @@
+//===-- Flush Cacheline for AArch64 -----------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_LIBC_SRC_STRING_MEMORY_UTILS_AARCH64_FLUSH_CACHELINE_H
+#define LLVM_LIBC_SRC_STRING_MEMORY_UTILS_AARCH64_FLUSH_CACHELINE_H
+
+#include "src/__support/common.h"
+#include <stddef.h> // size_t
+namespace LIBC_NAMESPACE {
+
+LIBC_INLINE size_t cacheline_size() {
+  // Use the same way as in compiler-rt
+  size_t ctr_el0;
+  asm volatile("mrs %0, ctr_el0" : "=r"(ctr_el0));
+  return 4 << ((ctr_el0 >> 16) & 15);
+}
+
+LIBC_INLINE void flush_cacheline_async(volatile char *addr) {
+  // flush to external memory and invalidate the cache line
+  asm volatile("dc civac, %0" : : "r"(addr) : "memory");
+}
+
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC_STRING_MEMORY_UTILS_AARCH64_FLUSH_CACHELINE_H
diff --git a/libc/src/string/memory_utils/aarch64/inline_memset.h b/libc/src/string/memory_utils/aarch64/inline_memset.h
index 91512acce6fc07..79d7aab278da44 100644
--- a/libc/src/string/memory_utils/aarch64/inline_memset.h
+++ b/libc/src/string/memory_utils/aarch64/inline_memset.h
@@ -17,6 +17,7 @@
 
 namespace LIBC_NAMESPACE {
 
+template <bool OPAQUE_VALUE = false>
 [[maybe_unused]] LIBC_INLINE static void
 inline_memset_aarch64(Ptr dst, uint8_t value, size_t count) {
   static_assert(aarch64::kNeon, "aarch64 supports vector types");
@@ -45,7 +46,7 @@ inline_memset_aarch64(Ptr dst, uint8_t value, size_t count) {
     generic::Memset<uint256_t>::tail(dst, value, count);
     return;
   }
-  if (count >= 448 && value == 0 && aarch64::neon::hasZva()) {
+  if (!OPAQUE_VALUE && count >= 448 && value == 0 && aarch64::neon::hasZva()) {
     generic::Memset<uint512_t>::block(dst, 0);
     align_to_next_boundary<64>(dst, count);
     return aarch64::neon::BzeroCacheLine::loop_and_tail(dst, 0, count);
diff --git a/libc/src/string/memory_utils/flush_cache.h b/libc/src/string/memory_utils/flush_cache.h
new file mode 100644
index 00000000000000..5bb2055752d9ba
--- /dev/null
+++ b/libc/src/string/memory_utils/flush_cache.h
@@ -0,0 +1,57 @@
+//===-- Dispatch cache flushing -------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STRING_MEMORY_UTILS_FLUSH_CACHE_H
+#define LLVM_LIBC_SRC_STRING_MEMORY_UTILS_FLUSH_CACHE_H
+
+#include "src/__support/CPP/atomic.h"
+#include "src/__support/common.h"
+#include "src/__support/macros/properties/architectures.h" // LIBC_TARGET_ARCH_IS_
+
+#include <stddef.h> // size_t
+#include <stdint.h> // uintptr_t
+
+#ifdef LIBC_TARGET_ARCH_IS_X86
+#include "src/string/memory_utils/x86_64/flush_cacheline.h"
+#define LIBC_HAS_FLUSH_CACHELINE_ASYNC 1
+#elif defined(LIBC_TARGET_ARCH_IS_AARCH64)
+#include "src/string/memory_utils/aarch64/flush_cacheline.h"
+#define LIBC_HAS_FLUSH_CACHELINE_ASYNC 1
+#else
+#define LIBC_HAS_FLUSH_CACHELINE_ASYNC 0
+#endif
+
+namespace LIBC_NAMESPACE {
+
+LIBC_INLINE void flush_cache(volatile void *start, size_t size) {
+#if LIBC_HAS_FLUSH_CACHELINE_ASYNC
+  size_t line_size = cacheline_size();
+  uintptr_t addr = reinterpret_cast<uintptr_t>(start);
+  uintptr_t offset = addr % line_size;
+  // shift start to the left and align size to the right
+  // we want to cover the whole range of memory that needs to be flushed
+  size += offset;
+  size += line_size - (size % line_size);
+  addr -= offset;
+  // flush cache line async may be reordered. We need to put barriers.
+  cpp::atomic_thread_fence(cpp::MemoryOrder::SEQ_CST);
+  for (size_t i = 0; i < size; i += line_size)
+    flush_cacheline_async(reinterpret_cast<volatile char *>(addr + i));
+  cpp::atomic_thread_fence(cpp::MemoryOrder::SEQ_CST);
+#else
+  // we do not have specific instructions to flush the cache
+  // fallback to use a full memory barrier instead.
+  // Notice, however, memory fence might not flush the cache on many
+  // architectures.
+  cpp::atomic_thread_fence(cpp::MemoryOrder::SEQ_CST);
+#endif
+}
+
+} // namespace LIBC_NAMESPACE
+#undef LIBC_HAS_FLUSH_CACHELINE_ASYNC
+#endif // LLVM_LIBC_SRC_STRING_MEMORY_UTILS_FLUSH_CACHE_H
diff --git a/libc/src/string/memory_utils/inline_memset.h b/libc/src/string/memory_utils/inline_memset.h
index 1c07c1ca4bffc0..0dd698a325bb12 100644
--- a/libc/src/string/memory_utils/inline_memset.h
+++ b/libc/src/string/memory_utils/inline_memset.h
@@ -36,8 +36,16 @@
 
 namespace LIBC_NAMESPACE {
 
+template <bool OPAQUE_VALUE = false>
 LIBC_INLINE static void inline_memset(void *dst, uint8_t value, size_t count) {
+#if LIBC_TARGET_ARCH_IS_AARCH64
+  // The AArch64 implementation has an additional template parameter. It
+  // may uses dc zva to zero memory.
+  LIBC_SRC_STRING_MEMORY_UTILS_MEMSET<OPAQUE_VALUE>(reinterpret_cast<Ptr>(dst),
+                                                    value, count);
+#else
   LIBC_SRC_STRING_MEMORY_UTILS_MEMSET(reinterpret_cast<Ptr>(dst), value, count);
+#endif
 }
 
 } // namespace LIBC_NAMESPACE
diff --git a/libc/src/string/memory_utils/x86_64/flush_cacheline.h b/libc/src/string/memory_utils/x86_64/flush_cacheline.h
new file mode 100644
index 00000000000000..db9229f69ba314
--- /dev/null
+++ b/libc/src/string/memory_utils/x86_64/flush_cacheline.h
@@ -0,0 +1,27 @@
+//===-- Flush Cacheline for x86_64 ------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#ifndef LLVM_LIBC_SRC_STRING_MEMORY_UTILS_X86_64_FLUSH_CACHELINE_H
+#define LLVM_LIBC_SRC_STRING_MEMORY_UTILS_X86_64_FLUSH_CACHELINE_H
+
+#include "src/__support/common.h"
+#include <stddef.h> // size_t
+namespace LIBC_NAMESPACE {
+
+LIBC_INLINE constexpr size_t cacheline_size() { return 64; }
+
+LIBC_INLINE void flush_cacheline_async(volatile char *addr) {
+#if defined(LIBC_TARGET_CPU_HAS_CLFLUSHOPT)
+  asm volatile("clflushopt %0" : "+m"(*addr)::"memory");
+#else
+  __builtin_ia32_clflush(const_cast<const char *>(addr));
+#endif
+}
+
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC_STRING_MEMORY_UTILS_X86_64_FLUSH_CACHELINE_H
diff --git a/libc/src/string/memset_explicit.cpp b/libc/src/string/memset_explicit.cpp
new file mode 100644
index 00000000000000..0e58cb16c181f7
--- /dev/null
+++ b/libc/src/string/memset_explicit.cpp
@@ -0,0 +1,27 @@
+//===-- Implementation of memset_explicit ---------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/string/memset_explicit.h"
+#include "src/__support/common.h"
+#include "src/string/memory_utils/flush_cache.h"
+#include "src/string/memory_utils/inline_memset.h"
+
+namespace LIBC_NAMESPACE {
+
+LLVM_LIBC_FUNCTION(void *, memset_explicit,
+                   (void *dst, int value, size_t count)) {
+  // Use the inline memset function to set the memory.
+  inline_memset<true>(dst, static_cast<uint8_t>(value), count);
+
+  // Flush the cache line.
+  flush_cache(dst, count);
+
+  return dst;
+}
+
+} // namespace LIBC_NAMESPACE
diff --git a/libc/src/string/memset_explicit.h b/libc/src/string/memset_explicit.h
new file mode 100644
index 00000000000000..c47880dbff1854
--- /dev/null
+++ b/libc/src/string/memset_explicit.h
@@ -0,0 +1,20 @@
+//===-- Implementation header for memset_explicit ---------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STRING_MEMSET_EXPLICIT_H
+#define LLVM_LIBC_SRC_STRING_MEMSET_EXPLICIT_H
+
+#include <stddef.h> // size_t
+
+namespace LIBC_NAMESPACE {
+
+void *memset_explicit(void *ptr, int value, size_t count);
+
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC_STRING_MEMSET_EXPLICIT_H
diff --git a/libc/test/src/string/CMakeLists.txt b/libc/test/src/string/CMakeLists.txt
index 6088289532d771..c1caec5fd912c8 100644
--- a/libc/test/src/string/CMakeLists.txt
+++ b/libc/test/src/string/CMakeLists.txt
@@ -418,6 +418,16 @@ add_libc_test(
     libc.src.string.strxfrm
 )
 
+add_libc_test(
+  memset_explicit_test
+  SUITE
+    libc-string-tests
+  SRCS
+    memset_explicit_test.cpp
+  DEPENDS
+    libc.src.string.memset_explicit
+)
+
 # Tests all implementations that can run on the target CPU.
 function(add_libc_multi_impl_test name)
   get_property(fq_implementations GLOBAL PROPERTY ${name}_implementations)
diff --git a/libc/test/src/string/memset_explicit_test.cpp b/libc/test/src/string/memset_explicit_test.cpp
new file mode 100644
index 00000000000000..bb5111bd639e3a
--- /dev/null
+++ b/libc/test/src/string/memset_explicit_test.cpp
@@ -0,0 +1,31 @@
+//===-- Unittests for memset_explicit -------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "memory_utils/memory_check_utils.h"
+#include "src/string/memset_explicit.h"
+#include "test/UnitTest/Test.h"
+
+namespace LIBC_NAMESPACE {
+
+// Apply the same tests as memset
+
+static inline void Adaptor(cpp::span<char> p1, uint8_t value, size_t size) {
+  LIBC_NAMESPACE::memset_explicit(p1.begin(), value, size);
+}
+
+TEST(LlvmLibcmemsetExplicitTest, SizeSweep) {
+  static constexpr size_t kMaxSize = 400;
+  Buffer DstBuffer(kMaxSize);
+  for (size_t size = 0; size < kMaxSize; ++size) {
+    const char value = size % 10;
+    auto dst = DstBuffer.span().subspan(0, size);
+    ASSERT_TRUE((CheckMemset<Adaptor>(dst, value, size)));
+  }
+}
+
+} // namespace LIBC_NAMESPACE

@SchrodingerZhu
Copy link
Contributor Author

Please help check the aarch64 implementation.

Copy link
Member

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

Please update the docs to add this function as implemented (perhaps only for certain architectures).

@nickdesaulniers
Copy link
Member

Copy link
Member

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

Please add to the commit message that this fixes #82817 so that the issue will auto close when merged.

As discussed in https://discourse.llvm.org/t/implement-memset-explicit/77312/5, the current implementation adds significant complexity which is not mandated, specified, or referenced in the standard. As such, I'd prefer to see and initial PR that's just the memset + compiler barrier. Once we have that basic implementation merged, we can start adding architecture specific mitigations behind a cmake flag, in a distinct PR.

I would like to have a cmake flag to opt into the arch-specific additional mitigations. From there, we can then later add a generic "hardened libc" cmake flag that turns on all of these individual configurations to improve security.

@SchrodingerZhu
Copy link
Contributor Author

I have updated the implementation using the initial proposal with changes:

  1. use the barrier from bionic to avoid DSE
  2. avoid using ZVA on AArch64

Copy link
Member

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

Until the standard says we must memset differently then I'm not willing to accept the complexity of doing something different in our memset to support memset_explicit.

@AaronBallman
Copy link
Collaborator

Until the standard says we must memset differently then I'm not willing to accept the complexity of doing something different in our memset to support memset_explicit.

If you are saying that you want memset_explicit to behave like memset with a compiler barrier in the initial patch and then do additional security hardening in follow-up work, that sounds reasonable to me.

If you are saying that you want memset_explicit to behave like memset until the standard changes: as the community's WG14 rep, I don't think this is a reasonable stance to take. I explained in the RFC why your belief that the standard should be prescriptive here is not feasible. IMO, this is not in the best interests of users who want this functionality, and I'd like you to reconsider your position or at least provide significant justification for why you think a security related feature should be implemented in a way that's almost indistinguishable from malicious conformance. (Note, specific concerns like "I don't think we should do because " are absolutely fine by me. But "we won't make this any different from memset until the standard changes" is what I'm pushing back on, if that's what you're actually pushing for.)

Btw, if you'd like, I'm happy to hop in a call to discuss further so we can get on the same page more quickly.


namespace LIBC_NAMESPACE {

// Apply the same tests as memset
Copy link
Collaborator

Choose a reason for hiding this comment

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

That seems inadequate. The idea is that memset_explicit works in situations where memset does not, so that's the functionality that really needs testing. Probably you need a lit test that shows memset is optimized away but memset_explicit is not. Or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, we do not have llvm-lit for libc. Therefore, it is not a trivial thing to me to examine compiler behavior at this stage. Any suggestion?

Copy link
Member

@nickdesaulniers nickdesaulniers Mar 7, 2024

Choose a reason for hiding this comment

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

AFAIK, llvm-libc doesn't yet have testing frameworks in place to do what @pogo59 is suggesting. It may be useful to have at some point in the future, but I would say for right now, I struggle to think of additional cases where that would be useful. I'm sure that view will change over time and use cases for doing so will only grow, so we may end up eventually integrating some unit tests that make use of llvm-lit in the future. Because of the minimal need at the moment, I would say it's lower priority than say implementation work. When we have such testing framework in place, we should revisit verifying this invariant.

Perhaps for now:

  1. file a todo in llvm's issue tracker mentioning that we might want to pursue using llvm-lit to do some binary analysis testing of llvmlibc.
  2. link to it from this unit test

Over time, more use cases will be added to the issue which will help us reprioritize.

@nickdesaulniers
Copy link
Member

@AaronBallman and I hopped on a call and had a fruitful discussion. Despite having slight disagreements on various aspects, we are still friends (lest external followers of the discussion think there's larger disagreements here).

I think ultimately we will be able to provide feedback to WG14 that may one day result in modifications to the standard with regard to the language around this function in particular. But even if nothing comes of that or that feedback never gets written, I think we can provide additional QoL improvements, whether that be specific mitigations for well known attack vectors, or a "harden llvmlibc mode." I would like to have separate space to discuss and review those mitigations than in this initial PR as the current iteration does appear to me to satisfy the original intent of the proposal without significant added complexity.

@SchrodingerZhu
Copy link
Contributor Author

See #84366

@SchrodingerZhu
Copy link
Contributor Author

SchrodingerZhu commented Mar 7, 2024

If there is no further objection, I will merge this as it is :D

@AaronBallman
Copy link
Collaborator

@AaronBallman and I hopped on a call and had a fruitful discussion. Despite having slight disagreements on various aspects, we are still friends (lest external followers of the discussion think there's larger disagreements here).

100% agreed, thank you for hopping on the call with me, that was super productive (and it's always fun to get to chat, too!).

I think ultimately we will be able to provide feedback to WG14 that may one day result in modifications to the standard with regard to the language around this function in particular. But even if nothing comes of that or that feedback never gets written, I think we can provide additional QoL improvements, whether that be specific mitigations for well known attack vectors, or a "harden llvmlibc mode." I would like to have separate space to discuss and review those mitigations than in this initial PR as the current iteration does appear to me to satisfy the original intent of the proposal without significant added complexity.

Agreed -- I think the current approach in this patch is good progress and we can hammer out incremental improvements in the future. Thank you!

@SchrodingerZhu SchrodingerZhu merged commit 57a3373 into llvm:main Mar 7, 2024
@pogo59
Copy link
Collaborator

pogo59 commented Mar 7, 2024

Another possibility would be to look at placing this kind of test into cross-project-tests rather than in libc, as the test is looking at how clang responds to a libc implementation tactic.

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.

[libc][c23] implement memset_explicit
5 participants