-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[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
Conversation
@llvm/pr-subscribers-libc Author: Schrodinger ZHU Yifan (SchrodingerZhu) ChangesBased on https://discourse.llvm.org/t/implement-memset-explicit/77312 Full diff: https://github.com/llvm/llvm-project/pull/83577.diff 16 Files Affected:
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
|
Please help check the aarch64 implementation. |
1187c14
to
48658ba
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.
Please update the docs to add this function as implemented (perhaps only for certain architectures).
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.
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.
I have updated the implementation using the initial proposal with changes:
|
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.
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 If you are saying that you want 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 |
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.
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.
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.
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?
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.
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:
- 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.
- link to it from this unit test
Over time, more use cases will be added to the issue which will help us reprioritize.
@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. |
See #84366 |
If there is no further objection, I will merge this as it is :D |
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!).
Agreed -- I think the current approach in this patch is good progress and we can hammer out incremental improvements in the future. Thank you! |
Another possibility would be to look at placing this kind of test into |
Based on https://discourse.llvm.org/t/implement-memset-explicit/77312
fixes #82817.