[clang] Fix 71315698c9 in presence of incomplete types #114095

serge-sans-paille · 2024-10-29T17:35:44Z

Incomplete types are not considered trivially copyable by clang but we don't want to warn about invalid argument for memcpy / memset in that case because we cannot prove they are not Trivially Copyable.

llvmbot · 2024-10-29T17:36:31Z

@llvm/pr-subscribers-clang

Author: None (serge-sans-paille)

Changes

Incomplete types are not considered trivially copyable by clang but we don't want to warn about invalid argument for memcpy / memset in that case because we cannot prove they are not Trivially Copyable.

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

2 Files Affected:

(modified) clang/lib/Sema/SemaChecking.cpp (+4-3)
(modified) clang/test/SemaCXX/warn-memaccess.cpp (+21-4)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 3308b898a5b68f..577218ac4f0ab2 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -8900,7 +8900,8 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call,
           << Call->getCallee()->getSourceRange());
     else if (const auto *RT = PointeeTy->getAs<RecordType>()) {
 
-      bool IsTriviallyCopyableCXXRecord =
+      bool MayBeTriviallyCopyableCXXRecord =
+          RT->isIncompleteType() ||
           RT->desugar().isTriviallyCopyableType(Context);
 
       if ((BId == Builtin::BImemset || BId == Builtin::BIbzero) &&
@@ -8910,7 +8911,7 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call,
                                 << ArgIdx << FnName << PointeeTy << 0);
         SearchNonTrivialToInitializeField::diag(PointeeTy, Dest, *this);
       } else if ((BId == Builtin::BImemset || BId == Builtin::BIbzero) &&
-                 !IsTriviallyCopyableCXXRecord && ArgIdx == 0) {
+                 !MayBeTriviallyCopyableCXXRecord && ArgIdx == 0) {
         // FIXME: Limiting this warning to dest argument until we decide
         // whether it's valid for source argument too.
         DiagRuntimeBehavior(Dest->getExprLoc(), Dest,
@@ -8923,7 +8924,7 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call,
                                 << ArgIdx << FnName << PointeeTy << 1);
         SearchNonTrivialToCopyField::diag(PointeeTy, Dest, *this);
       } else if ((BId == Builtin::BImemcpy || BId == Builtin::BImemmove) &&
-                 !IsTriviallyCopyableCXXRecord && ArgIdx == 0) {
+                 !MayBeTriviallyCopyableCXXRecord && ArgIdx == 0) {
         // FIXME: Limiting this warning to dest argument until we decide
         // whether it's valid for source argument too.
         DiagRuntimeBehavior(Dest->getExprLoc(), Dest,
diff --git a/clang/test/SemaCXX/warn-memaccess.cpp b/clang/test/SemaCXX/warn-memaccess.cpp
index b4b7f6a6905b23..070b44891a91aa 100644
--- a/clang/test/SemaCXX/warn-memaccess.cpp
+++ b/clang/test/SemaCXX/warn-memaccess.cpp
@@ -7,12 +7,17 @@ extern "C" void *memcpy(void *s1, const void *s2, unsigned n);
 
 class TriviallyCopyable {};
 class NonTriviallyCopyable { NonTriviallyCopyable(const NonTriviallyCopyable&);};
+struct Incomplete;
 
 void test_bzero(TriviallyCopyable* tc,
-                 NonTriviallyCopyable *ntc) {
+                NonTriviallyCopyable *ntc,
+                Incomplete* i) {
   // OK
   bzero(tc, sizeof(*tc));
 
+  // OK
+  bzero(i, 10);
+
   // expected-warning@+2{{first argument in call to 'bzero' is a pointer to non-trivially copyable type 'NonTriviallyCopyable'}}
   // expected-note@+1{{explicitly cast the pointer to silence this warning}}
   bzero(ntc, sizeof(*ntc));
@@ -22,10 +27,14 @@ void test_bzero(TriviallyCopyable* tc,
 }
 
 void test_memset(TriviallyCopyable* tc,
-                 NonTriviallyCopyable *ntc) {
+                 NonTriviallyCopyable *ntc,
+                 Incomplete* i) {
   // OK
   memset(tc, 0, sizeof(*tc));
 
+  // OK
+  memset(i, 0, 10);
+
   // expected-warning@+2{{first argument in call to 'memset' is a pointer to non-trivially copyable type 'NonTriviallyCopyable'}}
   // expected-note@+1{{explicitly cast the pointer to silence this warning}}
   memset(ntc, 0, sizeof(*ntc));
@@ -36,10 +45,14 @@ void test_memset(TriviallyCopyable* tc,
 
 
 void test_memcpy(TriviallyCopyable* tc0, TriviallyCopyable* tc1,
-                 NonTriviallyCopyable *ntc0, NonTriviallyCopyable *ntc1) {
+                 NonTriviallyCopyable *ntc0, NonTriviallyCopyable *ntc1,
+                 Incomplete *i0, Incomplete *i1) {
   // OK
   memcpy(tc0, tc1, sizeof(*tc0));
 
+  // OK
+  memcpy(i0, i1, 10);
+
   // expected-warning@+2{{first argument in call to 'memcpy' is a pointer to non-trivially copyable type 'NonTriviallyCopyable'}}
   // expected-note@+1{{explicitly cast the pointer to silence this warning}}
   memcpy(ntc0, ntc1, sizeof(*ntc0));
@@ -52,10 +65,14 @@ void test_memcpy(TriviallyCopyable* tc0, TriviallyCopyable* tc1,
 }
 
 void test_memmove(TriviallyCopyable* tc0, TriviallyCopyable* tc1,
-                 NonTriviallyCopyable *ntc0, NonTriviallyCopyable *ntc1) {
+                  NonTriviallyCopyable *ntc0, NonTriviallyCopyable *ntc1,
+                  Incomplete *i0, Incomplete *i1) {
   // OK
   memmove(tc0, tc1, sizeof(*tc0));
 
+  // OK
+  memmove(i0, i1, 10);
+
   // expected-warning@+2{{first argument in call to 'memmove' is a pointer to non-trivially copyable type 'NonTriviallyCopyable'}}
   // expected-note@+1{{explicitly cast the pointer to silence this warning}}
   memmove(ntc0, ntc1, sizeof(*ntc0));

erichkeane · 2024-10-29T17:43:15Z

clang/lib/Sema/SemaChecking.cpp

@@ -8900,7 +8900,8 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call,
          << Call->getCallee()->getSourceRange());
    else if (const auto *RT = PointeeTy->getAs<RecordType>()) {

-      bool IsTriviallyCopyableCXXRecord =
+      bool MayBeTriviallyCopyableCXXRecord =
+          RT->isIncompleteType() ||


I guess this just makes this a 'not foolproof' diagnostic here in C++ mode, which is somewhat unfortunate. We could perhaps provide a second diagnostic (in a different group?) about using incomplete CXXRecordType types, but IDK how that use case works.

So you're basically talking about this case:

struct S; void foo(S *s1, S *s2) { memcpy(s1, s2, 10); } struct S { S(const S&); S& operator=(const S &); };

I'm not certain that's worth splitting off as its own diagnostic as memcpy, memset, etc all need you to pass in a size parameter and... anything the user puts there in case of an incomplete type is pretty suspect. Based on that, I think we may want the diagnostic for incomplete types, but that the diagnostic only makes sense in C++ regardless of whether the type is complete or not.

However, GCC silences the diagnostic for incomplete types: https://godbolt.org/z/bsPnrKf19 so I may be missing some use case where this construct is safer than I'm thinking.

According to https://godbolt.org/z/xcnb154de having fields that point to incomplete types does not make you non trivially copyable, so I'm not even sure it reduces the quality of the original warning.

According to https://godbolt.org/z/xcnb154de having fields that point to incomplete types does not make you non trivially copyable, so I'm not even sure it reduces the quality of the original warning.

Why WOULD it? Those are pointers, which are complete types.

So you're basically talking about this case:

struct S; void foo(S *s1, S *s2) { memcpy(s1, s2, 10); } struct S { S(const S&); S& operator=(const S &); };

I'm not certain that's worth splitting off as its own diagnostic as memcpy, memset, etc all need you to pass in a size parameter and... anything the user puts there in case of an incomplete type is pretty suspect. Based on that, I think we may want the diagnostic for incomplete types, but that the diagnostic only makes sense in C++ regardless of whether the type is complete or not.

However, GCC silences the diagnostic for incomplete types: https://godbolt.org/z/bsPnrKf19 so I may be missing some use case where this construct is safer than I'm thinking.

Yeah, something like that. It ends up causing someone to be able to 'side-step' this diagnostic inadvertently by using an incomplete type. So a separate warning of, "you're using an incomplete type here that might be non-trivially-copyable in the future ya know!" might be useful to some, though if we put it in the same group as this one, it might cause folks to throw the baby with the bath water.

More 'mulling' than having a request.

Trivial copyability is about whether you can copy the object itself (aka, its members), and you can't create an object of incomplete class type or which contains an object of incomplete class type. So a pointer to an incomplete type is fine in that regard; it's just a pointer, you're not dereferencing it. However, for this diagnostic, memcpy and friends are touching the bytes of the object (so it's a dereference, effectively), and passing a pointer to an incomplete type means those are touching bytes of an unknown type which may be completed later.

Edit: hah, it seems Erich posted something similar while I was typing my comment.

Should I mention that as a comment in the code?

Should I mention that as a comment in the code?

There were a few things, what do you mean by 'that'? Perhaps something that "note, this makes the diagnostic imperfect because... but shrug"?

Something along those lines?

FIXME: Do not consider incomplete type even though they may be completed later.

I can live with that.

Something along those lines?

FIXME: Do not consider incomplete type even though they may be completed later.

I'd prefer something more like:

FIXME: Do not consider incomplete types even though they may be completed later. GCC does not diagnose such code, but we may want to consider diagnosing it in the future, perhaps under a different, but related, diagnostic group.

so it's more clear why it's a fixme. But yeah, I can live with that.

AaronBallman

LGTM with commenting changes applied

Incomplete types are not considered trivially copyable by clang but we don't want to warn about invalid argument for memcpy / memset in that case because we cannot prove they are not Trivially Copyable.

serge-sans-paille · 2024-10-30T07:32:34Z

thanks for the discussions and the review 🙇

llvm-ci · 2024-10-30T08:08:37Z

LLVM Buildbot has detected a new failure on builder openmp-offload-libc-amdgpu-runtime running on omp-vega20-1 while building clang at step 10 "Add check check-offload".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/7747

Here is the relevant piece of the build log for the reference

Step 10 (Add check check-offload) failure: 1200 seconds without output running [b'ninja', b'-j 32', b'check-offload'], attempting to kill
...
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug53727.cpp (866 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug50022.cpp (867 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/test_libc.cpp (868 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug49779.cpp (869 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/wtime.c (870 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu :: offloading/bug49021.cpp (871 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu :: offloading/std_complex_arithmetic.cpp (872 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/complex_reduction.cpp (873 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/std_complex_arithmetic.cpp (874 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug49021.cpp (875 of 879)
command timed out: 1200 seconds without output running [b'ninja', b'-j 32', b'check-offload'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=1237.012473

Incomplete types are not considered trivially copyable by clang but we don't want to warn about invalid argument for memcpy / memset in that case because we cannot prove they are not Trivially Copyable.

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

serge-sans-paille mentioned this pull request Oct 29, 2024

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

Merged

serge-sans-paille requested review from AaronBallman and nathanchance October 29, 2024 17:41

erichkeane approved these changes Oct 29, 2024

View reviewed changes

serge-sans-paille force-pushed the feature/memaccess-incomplete branch from d5807d3 to 10c84d0 Compare October 29, 2024 18:03

AaronBallman approved these changes Oct 29, 2024

View reviewed changes

serge-sans-paille force-pushed the feature/memaccess-incomplete branch from 10c84d0 to e15fcb7 Compare October 29, 2024 19:48

[clang] Fix 7131569 in presence of incomplete types

0f7b4b2

Incomplete types are not considered trivially copyable by clang but we don't want to warn about invalid argument for memcpy / memset in that case because we cannot prove they are not Trivially Copyable.

serge-sans-paille force-pushed the feature/memaccess-incomplete branch from e15fcb7 to 0f7b4b2 Compare October 29, 2024 21:33

serge-sans-paille merged commit dc56a86 into llvm:main Oct 30, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang] Fix 71315698c9 in presence of incomplete types #114095

[clang] Fix 71315698c9 in presence of incomplete types #114095

Uh oh!

serge-sans-paille commented Oct 29, 2024

Uh oh!

llvmbot commented Oct 29, 2024

Uh oh!

erichkeane Oct 29, 2024

Uh oh!

AaronBallman Oct 29, 2024

Uh oh!

serge-sans-paille Oct 29, 2024

Uh oh!

erichkeane Oct 29, 2024

Uh oh!

AaronBallman Oct 29, 2024 •

edited

Loading

Uh oh!

serge-sans-paille Oct 29, 2024

Uh oh!

erichkeane Oct 29, 2024

Uh oh!

serge-sans-paille Oct 29, 2024

Uh oh!

erichkeane Oct 29, 2024

Uh oh!

AaronBallman Oct 29, 2024

Uh oh!

AaronBallman left a comment

Uh oh!

Uh oh!

serge-sans-paille commented Oct 30, 2024

Uh oh!

llvm-ci commented Oct 30, 2024

Uh oh!

Uh oh!

[clang] Fix 71315698c9 in presence of incomplete types #114095

[clang] Fix 71315698c9 in presence of incomplete types #114095

Uh oh!

Conversation

serge-sans-paille commented Oct 29, 2024

Uh oh!

llvmbot commented Oct 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronBallman Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serge-sans-paille commented Oct 30, 2024

Uh oh!

llvm-ci commented Oct 30, 2024

Uh oh!

Uh oh!

AaronBallman Oct 29, 2024 •

edited

Loading