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.

Sign up for GitHub

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
Merged

[libc][c23] add memset_explicit #83577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
48658ba
[libc][c23] add memset_explicit
SchrodingerZhu Mar 1, 2024
bb457ac
add noinline attr
SchrodingerZhu Mar 1, 2024
aa12ab7
address reviews
SchrodingerZhu Mar 6, 2024
c4d3ce2
remove extra code
SchrodingerZhu Mar 6, 2024
ff5fb31
undo aarch64 changes
SchrodingerZhu Mar 6, 2024
364bb4b
remove template arg
SchrodingerZhu Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions libc/config/linux/aarch64/entrypoints.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions libc/config/linux/x86_64/entrypoints.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions libc/spec/stdc.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down
11 changes: 11 additions & 0 deletions libc/src/string/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,17 @@ 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
)

# Helper to define a function with multiple implementations
# - Computes flags to satisfy required/rejected features and arch,
# - Declares an entry point,
Expand Down
25 changes: 25 additions & 0 deletions libc/src/string/memset_explicit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
//===-- 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/inline_memset.h"

namespace LIBC_NAMESPACE {

[[gnu::noinline]] LLVM_LIBC_FUNCTION(void *, memset_explicit,
(void *dst, int value, size_t count)) {
// Use the inline memset function to set the memory.
inline_memset(dst, static_cast<uint8_t>(value), count);
// avoid dead store elimination
// The asm itself should also be sufficient to behave as a compiler barrier.
asm("" : : "r"(dst) : "memory");
return dst;
}

} // namespace LIBC_NAMESPACE
20 changes: 20 additions & 0 deletions libc/src/string/memset_explicit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 {

[[gnu::noinline]] void *memset_explicit(void *ptr, int value, size_t count);

} // namespace LIBC_NAMESPACE

#endif // LLVM_LIBC_SRC_STRING_MEMSET_EXPLICIT_H
10 changes: 10 additions & 0 deletions libc/test/src/string/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
31 changes: 31 additions & 0 deletions libc/test/src/string/memset_explicit_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
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 •
edited
Loading

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.


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