[clang] Warn about memset/memcpy to NonTriviallyCopyable types #111434

serge-sans-paille · 2024-10-07T20:55:12Z

This implements a warning that's similar to what GCC does in that context: both memcpy and memset require their first and second operand to be trivially copyable, let's warn if that's not the case.

llvmbot · 2024-10-07T20:55:46Z

@llvm/pr-subscribers-pgo
@llvm/pr-subscribers-libcxx

@llvm/pr-subscribers-clang

Author: None (serge-sans-paille)

Changes

This implements a warning that's similar to what GCC does in that context: both memcpy and memset require their first and second operand to be trivially copyable, let's warn if that's not the case.

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

3 Files Affected:

(modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+4)
(modified) clang/lib/Sema/SemaChecking.cpp (+24)
(modified) clang/test/SemaCXX/constexpr-string.cpp (+4)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 583475327c5227..d9bff4a559b3b7 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -790,6 +790,10 @@ def warn_cstruct_memaccess : Warning<
   "%1 call is a pointer to record %2 that is not trivial to "
   "%select{primitive-default-initialize|primitive-copy}3">,
   InGroup<NonTrivialMemaccess>;
+def warn_cxxstruct_memaccess : Warning<
+  "%select{destination for|source of|first operand of|second operand of}0 this "
+  "%1 call is a pointer to record %2 that is not trivially-copyable">,
+  InGroup<NonTrivialMemaccess>;
 def note_nontrivial_field : Note<
   "field is non-trivial to %select{copy|default-initialize}0">;
 def err_non_trivial_c_union_in_invalid_context : Error<
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 2bcb930acdcb57..46dda34d0ac8f3 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -8899,18 +8899,42 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call,
           << ArgIdx << FnName << PointeeTy
           << Call->getCallee()->getSourceRange());
     else if (const auto *RT = PointeeTy->getAs<RecordType>()) {
+
+      auto IsTriviallyCopyableCXXRecord = [](auto const *RT) {
+        auto const *D = RT->getDecl();
+        if (!D)
+          return true;
+        auto const *RD = dyn_cast<CXXRecordDecl>(D);
+        if (!RD)
+          return true;
+        RD = RD->getDefinition();
+        if (!RD)
+          return true;
+        return RD->isTriviallyCopyable();
+      };
+
       if ((BId == Builtin::BImemset || BId == Builtin::BIbzero) &&
           RT->getDecl()->isNonTrivialToPrimitiveDefaultInitialize()) {
         DiagRuntimeBehavior(Dest->getExprLoc(), Dest,
                             PDiag(diag::warn_cstruct_memaccess)
                                 << ArgIdx << FnName << PointeeTy << 0);
         SearchNonTrivialToInitializeField::diag(PointeeTy, Dest, *this);
+      } else if ((BId == Builtin::BImemset || BId == Builtin::BIbzero) &&
+                 !IsTriviallyCopyableCXXRecord(RT)) {
+        DiagRuntimeBehavior(Dest->getExprLoc(), Dest,
+                            PDiag(diag::warn_cxxstruct_memaccess)
+                                << ArgIdx << FnName << PointeeTy);
       } else if ((BId == Builtin::BImemcpy || BId == Builtin::BImemmove) &&
                  RT->getDecl()->isNonTrivialToPrimitiveCopy()) {
         DiagRuntimeBehavior(Dest->getExprLoc(), Dest,
                             PDiag(diag::warn_cstruct_memaccess)
                                 << ArgIdx << FnName << PointeeTy << 1);
         SearchNonTrivialToCopyField::diag(PointeeTy, Dest, *this);
+      } else if ((BId == Builtin::BImemcpy || BId == Builtin::BImemmove) &&
+                 !IsTriviallyCopyableCXXRecord(RT)) {
+        DiagRuntimeBehavior(Dest->getExprLoc(), Dest,
+                            PDiag(diag::warn_cxxstruct_memaccess)
+                                << ArgIdx << FnName << PointeeTy);
       } else {
         continue;
       }
diff --git a/clang/test/SemaCXX/constexpr-string.cpp b/clang/test/SemaCXX/constexpr-string.cpp
index c456740ef7551f..26e2e138ef34e0 100644
--- a/clang/test/SemaCXX/constexpr-string.cpp
+++ b/clang/test/SemaCXX/constexpr-string.cpp
@@ -603,12 +603,16 @@ namespace MemcpyEtc {
   };
   constexpr bool test_nontrivial_memcpy() { // expected-error {{never produces a constant}}
     NonTrivial arr[3] = {};
+    // expected-warning@+2 {{source of this '__builtin_memcpy' call is a pointer to record 'NonTrivial' that is not trivially-copyable}}
+    // expected-note@+1 {{explicitly cast the pointer to silence this warning}}
     __builtin_memcpy(arr, arr + 1, sizeof(NonTrivial)); // expected-note 2{{non-trivially-copyable}}
     return true;
   }
   static_assert(test_nontrivial_memcpy()); // expected-error {{constant}} expected-note {{in call}}
   constexpr bool test_nontrivial_memmove() { // expected-error {{never produces a constant}}
     NonTrivial arr[3] = {};
+    // expected-warning@+2 {{source of this '__builtin_memcpy' call is a pointer to record 'NonTrivial' that is not trivially-copyable}}
+    // expected-note@+1 {{explicitly cast the pointer to silence this warning}}
     __builtin_memcpy(arr, arr + 1, sizeof(NonTrivial)); // expected-note 2{{non-trivially-copyable}}
     return true;
   }

serge-sans-paille · 2024-10-08T09:45:47Z

This change is triggering warnings at several point in the code base, I'll fix that too.

serge-sans-paille · 2024-10-08T12:47:45Z

patches to make sure llvm, libc++ and compiler-rt don't trigger that extra warning

serge-sans-paille · 2024-10-08T19:10:39Z

cc @ldionne got the compiler-rt part.

serge-sans-paille · 2024-10-08T19:41:44Z

Some of the libcxx builds fail, they are marked as "build stopped: interrupted by user."... I can't find the origin

philnik777 · 2024-10-09T08:21:02Z

libcxx/include/__iterator/aliasing_iterator.h

@@ -102,7 +102,7 @@ struct __aliasing_iterator_wrapper {

    _LIBCPP_HIDE_FROM_ABI _Alias operator*() const _NOEXCEPT {
      _Alias __val;
-      __builtin_memcpy(&__val, std::__to_address(__base_), sizeof(value_type));
+      __builtin_memcpy(&__val, reinterpret_cast<void*>(std::__to_address(__base_)), sizeof(value_type));


IMO this is a false-positive. This inspects the non-trivial object and implicitly starts the lifetime of _Alias, which is perfectly defined behaviour.

The cast should probably be static_cast<void*> also

ack for the false positive. I'll move that to a static_cast which should silent the warning.

carlosgalvezp · 2024-10-09T08:52:33Z

Please note: GCC is more strict; for std::memset, it requires the type to be trivial, not just trivially copyable:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107361

I want to remember it was also more strict for std::memcpy, but I haven't been able to reproduce it.

I haven't looked at the patch in detail but for consistency with GCC it should also support suppressing the warning by explictly casting the pointers to void* (maybe it's already in place).

serge-sans-paille · 2024-10-09T12:13:21Z

Please note: GCC is more strict; for std::memset, it requires the type to be trivial, not just trivially copyable:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107361

ACK. Not a standard requirement though, is it?

I want to remember it was also more strict for std::memcpy, but I haven't been able to reproduce it.

I haven't looked at the patch in detail but for consistency with GCC it should also support suppressing the warning by explictly casting the pointers to void* (maybe it's already in place).

Yeah, that's part of current logic clang in clang too (thus the various cast added in this PR)

carlosgalvezp · 2024-10-09T12:39:38Z

ACK. Not a standard requirement though, is it?

Correct, it's only UB on non-trivially-copyable. My point was more about if we want to be consistent with GCC or not. I don't have any strong opinion on that.

philnik777 · 2024-10-09T13:19:17Z

ACK. Not a standard requirement though, is it?

Correct, it's only UB on non-trivially-copyable. My point was more about if we want to be consistent with GCC or not. I don't have any strong opinion on that.

I can see an argument that a user probably is doing something wrong if they memset a non-trivial type, especially since compilers are perfectly capable of optimizing zero-initializing loops into memsets. For memcpy/memmove we should definitely only diagnose non-trivially copyable cases, since it's quite a common optimization to memcpy trivially copyable objects.

AaronBallman

Thank you for this! It's missing a lot of test coverage that should be added to clang/test/SemaCXX and you should also add a release note to clang/docs/ReleaseNotes.rst so users know about the new diagnostic.

AaronBallman · 2024-10-10T13:23:37Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

+  "%select{destination for|source of|first operand of|second operand of}0 this "
+  "%1 call is a pointer to record %2 that is not trivially-copyable">,


Suggested change

"%select{destination for|source of|first operand of|second operand of}0 this "

"%1 call is a pointer to record %2 that is not trivially-copyable">,

"%select{destination for|source of|first operand of|second operand of}0 call to "

"%1 is a pointer to non-trivially copyable type %2">,

Slight rewording

AaronBallman · 2024-10-10T13:32:38Z

clang/lib/Sema/SemaChecking.cpp

@@ -8899,18 +8899,42 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call,
          << ArgIdx << FnName << PointeeTy
          << Call->getCallee()->getSourceRange());
    else if (const auto *RT = PointeeTy->getAs<RecordType>()) {
+
+      auto IsTriviallyCopyableCXXRecord = [](auto const *RT) {


Suggested change

auto IsTriviallyCopyableCXXRecord = [](auto const *RT) {

auto IsTriviallyCopyableCXXRecord = [](const RecordType *RT) {

Do not use auto when there's only one type this can be called with (I just spent a lot of time writing comments that I had to delete because this can only accept a RecordType).

Actually, I don't think we even need the lambda. You could precalculate this as a local variable:

bool IsTriviallyCopyableCXXRecord = false; if (const auto *RD = RT->getAsCXXRecordDecl()) IsTriviallyCopyableCXXRecord = RD->isTriviallyCopyable();

should suffice. (The call should return false if the record has no definition because that's an incomplete type but that's a good test case to ensure we have.)

Actually, @erichkeane just reminded me that we have QualType::isTriviallyCopyableType() -- that's an even better interface to use IMO.

AaronBallman · 2024-10-10T13:33:52Z

clang/lib/Sema/SemaChecking.cpp

@@ -8899,18 +8899,42 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call,
          << ArgIdx << FnName << PointeeTy
          << Call->getCallee()->getSourceRange());
    else if (const auto *RT = PointeeTy->getAs<RecordType>()) {
+
+      auto IsTriviallyCopyableCXXRecord = [](auto const *RT) {
+        auto const *D = RT->getDecl();


Suggested change

auto const *D = RT->getDecl();

const Decl *D = RT->getDecl();

Please only use auto when the type is spelled out in the initialization. Also, we use const Type and not Type const as the prevailing style, so you should stick with that.

AaronBallman · 2024-10-10T13:34:16Z

clang/lib/Sema/SemaChecking.cpp

+        auto const *D = RT->getDecl();
+        if (!D)
+          return true;
+        auto const *RD = dyn_cast<CXXRecordDecl>(D);


Suggested change

auto const *RD = dyn_cast<CXXRecordDecl>(D);

const auto *RD = dyn_cast<CXXRecordDecl>(D);

philnik777 · 2024-10-10T13:50:42Z

libcxx/include/__iterator/aliasing_iterator.h

@@ -102,7 +102,7 @@ struct __aliasing_iterator_wrapper {

    _LIBCPP_HIDE_FROM_ABI _Alias operator*() const _NOEXCEPT {
      _Alias __val;
-      __builtin_memcpy(&__val, std::__to_address(__base_), sizeof(value_type));
+      __builtin_memcpy(&__val, static_cast<const void*>(std::__to_address(__base_)), sizeof(value_type));


Just to make it visible again: IMO this is a false-positive, since this is inspecting a non-trivial type, which is perfectly defined behaviour.

There's one thing I don't understand: my understanding is that passing a pointer to a non-trivially copyable type to memcpy first or second argument is UB and _BaseIter maybe a non-trivially copyable type, so what happens then?

This implements a warning that's similar to what GCC does in that context: both memcpy and memset require their first and second operand to be trivially copyable, let's warn if that's not the case.

…ble data

serge-sans-paille · 2024-10-12T06:27:50Z

@AaronBallman doc, tests and style updated. Thanks for the review!

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 7, 2024

serge-sans-paille requested a review from AaronBallman October 7, 2024 20:55

serge-sans-paille requested a review from a team as a code owner October 8, 2024 12:47

llvmbot added compiler-rt libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. PGO Profile Guided Optimizations llvm:adt labels Oct 8, 2024

serge-sans-paille force-pushed the feature/trivially-copyable-memcpy branch from 3f44807 to bc3b844 Compare October 8, 2024 16:30

philnik777 reviewed Oct 9, 2024

View reviewed changes

serge-sans-paille force-pushed the feature/trivially-copyable-memcpy branch from bc3b844 to c9a013f Compare October 9, 2024 12:06

serge-sans-paille force-pushed the feature/trivially-copyable-memcpy branch from c9a013f to 92f14e6 Compare October 9, 2024 13:12

AaronBallman reviewed Oct 10, 2024

View reviewed changes

philnik777 reviewed Oct 10, 2024

View reviewed changes

serge-sans-paille force-pushed the feature/trivially-copyable-memcpy branch from 92f14e6 to 9cd993d Compare October 11, 2024 06:48

serge-sans-paille added 2 commits October 11, 2024 11:37

[clang] Warn about memset/memcpy to NonTriviallyCopyable types

df45524

This implements a warning that's similar to what GCC does in that context: both memcpy and memset require their first and second operand to be trivially copyable, let's warn if that's not the case.

[compiler-rt] Silent warning related to memcpy of non trivially-copia…

f5180fc

…ble data

serge-sans-paille force-pushed the feature/trivially-copyable-memcpy branch from 9cd993d to 67de33b Compare October 11, 2024 09:37

serge-sans-paille added 2 commits October 11, 2024 23:09

[libc++] Silent warning related to memcpy of non trivially-copyable data

b651491

[ADT] Silent warning related to memcpy of non trivially-copyable data

31be3da

serge-sans-paille force-pushed the feature/trivially-copyable-memcpy branch from 67de33b to 31be3da Compare October 11, 2024 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] Warn about memset/memcpy to NonTriviallyCopyable types #111434

[clang] Warn about memset/memcpy to NonTriviallyCopyable types #111434

serge-sans-paille commented Oct 7, 2024

llvmbot commented Oct 7, 2024 •

edited

Loading

serge-sans-paille commented Oct 8, 2024

serge-sans-paille commented Oct 8, 2024

serge-sans-paille commented Oct 8, 2024

serge-sans-paille commented Oct 8, 2024

philnik777 Oct 9, 2024

carlosgalvezp Oct 9, 2024

serge-sans-paille Oct 9, 2024

carlosgalvezp commented Oct 9, 2024 •

edited

Loading

serge-sans-paille commented Oct 9, 2024

carlosgalvezp commented Oct 9, 2024

philnik777 commented Oct 9, 2024

AaronBallman left a comment

AaronBallman Oct 10, 2024

AaronBallman Oct 10, 2024

AaronBallman Oct 10, 2024

AaronBallman Oct 10, 2024

AaronBallman Oct 10, 2024

philnik777 Oct 10, 2024

serge-sans-paille Oct 10, 2024

serge-sans-paille commented Oct 12, 2024

		"%select{destination for\|source of\|first operand of\|second operand of}0 this "
		"%1 call is a pointer to record %2 that is not trivially-copyable">,

	auto IsTriviallyCopyableCXXRecord = [](auto const *RT) {
	auto IsTriviallyCopyableCXXRecord = [](const RecordType *RT) {

	auto const *D = RT->getDecl();
	const Decl *D = RT->getDecl();

	auto const *RD = dyn_cast<CXXRecordDecl>(D);
	const auto *RD = dyn_cast<CXXRecordDecl>(D);

[clang] Warn about memset/memcpy to NonTriviallyCopyable types #111434

Are you sure you want to change the base?

[clang] Warn about memset/memcpy to NonTriviallyCopyable types #111434

Conversation

serge-sans-paille commented Oct 7, 2024

llvmbot commented Oct 7, 2024 • edited Loading

serge-sans-paille commented Oct 8, 2024

serge-sans-paille commented Oct 8, 2024

serge-sans-paille commented Oct 8, 2024

serge-sans-paille commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosgalvezp commented Oct 9, 2024 • edited Loading

serge-sans-paille commented Oct 9, 2024

carlosgalvezp commented Oct 9, 2024

philnik777 commented Oct 9, 2024

AaronBallman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serge-sans-paille commented Oct 12, 2024

llvmbot commented Oct 7, 2024 •

edited

Loading

carlosgalvezp commented Oct 9, 2024 •

edited

Loading