[clang] Move warning about memset/memcpy to NonTriviallyCopyable type…#117387
Merged
serge-sans-paille merged 3 commits intollvm:mainfrom Nov 28, 2024
Merged
Conversation
…s to its own flag Namely -Wnontrivial-memcall, implied by -Wnontricial-memaccess This is a followup to llvm#111434
Member
|
@llvm/pr-subscribers-clang Author: None (serge-sans-paille) Changes…s to its own flag Namely -Wnontrivial-memcall, implied by -Wnontricial-memaccess This is a followup to #111434 Full diff: https://github.com/llvm/llvm-project/pull/117387.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8bd06fadfdc984..80b521001bbd2f 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -404,7 +404,7 @@ Modified Compiler Flags
to utilize these vector libraries. The behavior for all other vector function
libraries remains unchanged.
-- The ``-Wnontrivial-memaccess`` warning has been updated to also warn about
+- The ``-Wnontrivial-memcall`` warning has been updated to also warn about
passing non-trivially-copyable destrination parameter to ``memcpy``,
``memset`` and similar functions for which it is a documented undefined
behavior.
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index df9bf94b5d0398..e20c3f8bbb26a5 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -683,11 +683,13 @@ def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">;
def SizeofPointerMemaccess : DiagGroup<"sizeof-pointer-memaccess">;
def MemsetTransposedArgs : DiagGroup<"memset-transposed-args">;
def DynamicClassMemaccess : DiagGroup<"dynamic-class-memaccess">;
-def NonTrivialMemaccess : DiagGroup<"nontrivial-memaccess">;
+def NonTrivialMemcall : DiagGroup<"nontrivial-memcall">;
+def NonTrivialMemaccess : DiagGroup<"nontrivial-memaccess", [NonTrivialMemcall]>;
def SuspiciousBzero : DiagGroup<"suspicious-bzero">;
def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess",
[SizeofPointerMemaccess, DynamicClassMemaccess,
- NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>;
+ NonTrivialMemaccess, NonTrivialMemcall, MemsetTransposedArgs,
+ SuspiciousBzero]>;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
def GNUStaticFloatInit : DiagGroup<"gnu-static-float-init">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index eb05a6a77978af..cc35c2c58baad0 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -798,7 +798,7 @@ def warn_cstruct_memaccess : Warning<
def warn_cxxstruct_memaccess : Warning<
"first argument in call to "
"%0 is a pointer to non-trivially copyable type %1">,
- InGroup<NonTrivialMemaccess>;
+ InGroup<NonTrivialMemcall>;
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/test/SemaCXX/warn-memcall.cpp b/clang/test/SemaCXX/warn-memcall.cpp
new file mode 100644
index 00000000000000..fbc9f25a5f65bc
--- /dev/null
+++ b/clang/test/SemaCXX/warn-memcall.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wnontrivial-memcall %s
+
+extern "C" void *memcpy(void *s1, const void *s2, unsigned n);
+
+class TriviallyCopyable {};
+class NonTriviallyCopyable { NonTriviallyCopyable(const NonTriviallyCopyable&);};
+struct Incomplete;
+
+void test_memcpy(TriviallyCopyable* tc0, TriviallyCopyable* tc1,
+ 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));
+
+ // ~ OK
+ memcpy((void*)ntc0, ntc1, sizeof(*ntc0));
+
+ // OK
+ memcpy((void*)ntc0, (void*)ntc1, sizeof(*ntc0));
+}
|
zmodem
reviewed
Nov 25, 2024
clang/docs/ReleaseNotes.rst
Outdated
| libraries remains unchanged. | ||
|
|
||
| - The ``-Wnontrivial-memaccess`` warning has been updated to also warn about | ||
| - The ``-Wnontrivial-memcall`` warning has been updated to also warn about |
Collaborator
There was a problem hiding this comment.
Maybe mention here that it's part of -Wnontrivial-memaccess?
| def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess", | ||
| [SizeofPointerMemaccess, DynamicClassMemaccess, | ||
| NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>; | ||
| NonTrivialMemaccess, NonTrivialMemcall, MemsetTransposedArgs, |
Collaborator
There was a problem hiding this comment.
Hmm, do you need to include NonTrivialMemcall here explicitly? Wouldn't it be transitively included already, because NonTrivialMemaccess includes NonTrivialMemcall?
Collaborator
There was a problem hiding this comment.
It should be transitively included, I believe. (Test coverage would tell us for sure though!)
clang/test/SemaCXX/warn-memcall.cpp
Outdated
Comment on lines
+1
to
+27
| // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wnontrivial-memcall %s | ||
|
|
||
| extern "C" void *memcpy(void *s1, const void *s2, unsigned n); | ||
|
|
||
| class TriviallyCopyable {}; | ||
| class NonTriviallyCopyable { NonTriviallyCopyable(const NonTriviallyCopyable&);}; | ||
| struct Incomplete; | ||
|
|
||
| void test_memcpy(TriviallyCopyable* tc0, TriviallyCopyable* tc1, | ||
| 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)); | ||
|
|
||
| // ~ OK | ||
| memcpy((void*)ntc0, ntc1, sizeof(*ntc0)); | ||
|
|
||
| // OK | ||
| memcpy((void*)ntc0, (void*)ntc1, sizeof(*ntc0)); | ||
| } |
Collaborator
There was a problem hiding this comment.
Can't we just update clang/test/SemaCXX/warn-memaccess.cpp instead?
Collaborator
Author
There was a problem hiding this comment.
I wanted a test case that only used -Wnontrivial-memcall
Collaborator
There was a problem hiding this comment.
But it looks to me that clang/test/SemaCXX/warn-memaccess.cpp also only covers -Wnontrivial-memcall, i.e. it's just checking calls to bzero/memset/memmove/memcpy. Can you just change the flag there?
Collaborator
AaronBallman
left a comment
There was a problem hiding this comment.
Generally LGTM modulo comments from @zmodem
| def SuspiciousMemaccess : DiagGroup<"suspicious-memaccess", | ||
| [SizeofPointerMemaccess, DynamicClassMemaccess, | ||
| NonTrivialMemaccess, MemsetTransposedArgs, SuspiciousBzero]>; | ||
| NonTrivialMemaccess, NonTrivialMemcall, MemsetTransposedArgs, |
Collaborator
There was a problem hiding this comment.
It should be transitively included, I believe. (Test coverage would tell us for sure though!)
…le types to its own flag
…le types to its own flag
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/11206 Here is the relevant piece of the build log for the reference |
Contributor
I think you can ignore this failure. Apologies. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…s to its own flag
Namely -Wnontrivial-memcall, implied by -Wnontrivial-memaccess
This is a followup to #111434