[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2582-2584
+int64_t BitfieldSize = Field->getBitWidthValue(Context);
+if (BitfieldSize > FieldSizeInBits)
+  return llvm::None;

steakhal wrote:
> Why do you return `None` here?
These bits were just extracted from `structHasUniqueObjectRepresentations`, the 
underlying logic was not changed here. (originally in line 2615)
I believe this handles the case "If the width of a bit-field is larger than the 
width of the bit-field's type (or, in case of an enumeration type, of its 
underlying type), the extra bits are padding bits" from [class.bit]



Comment at: clang/lib/AST/ASTContext.cpp:2595-2598
+template 
+static llvm::Optional structSubobjectsHaveUniqueObjectRepresentations(
+const RangeT , int64_t CurOffsetInBits, const ASTContext 
,
+const clang::ASTRecordLayout ) {

steakhal wrote:
> Why is this a template?
> Can't you just take the `field_range`?
> 
> By the same token, `structSubobjectsHaveUniqueObjectRepresentations` returns 
> an `optional int`. I'm confused.
This function is also called with (non virtual) base class subobjects, not just 
fields.
A `None` return value indicates that the subobjects do not have unique object 
representations, otherwise the size of the subobjects is returned. This allows 
us to detect for example padding between a base class and the first field. 



Comment at: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp:42
+} // namespace TailPaddingReuse
+static_assert(__has_unique_object_representations(TailPaddingReuse::B));

steakhal wrote:
> Why is this outside of the namespace declaration?
Tbh I'm not sure. I can move it in the namespace if you think it'd be better 
there. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added a comment.

Ping @rsmith


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89651: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added a comment.

Thanks for the review! I'll push this as soon as the updated version of D89649 
 is also accepted.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89651/new/

https://reviews.llvm.org/D89651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added a comment.

Pinging @rsmith - could you please check the updates to this patch? Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89651: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze updated this revision to Diff 306679.
gbencze added a comment.
Herald added a subscriber: jfb.

Address comments:

- add new test case for `_Atomic` (with a FIXME comment)
- fix string literal formatting
- update docs


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89651/new/

https://reviews.llvm.org/D89651

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-exp42-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-flp37-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison-32bits.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
@@ -0,0 +1,231 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t \
+// RUN: -- -- -target x86_64-unknown-unknown
+
+namespace std {
+typedef __SIZE_TYPE__ size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);
+} // namespace std
+
+namespace sei_cert_example_oop57_cpp {
+class C {
+  int i;
+
+public:
+  virtual void f();
+};
+
+void f(C , C ) {
+  if (!std::memcmp(, , sizeof(C))) {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: comparing object representation of non-standard-layout type 'sei_cert_example_oop57_cpp::C'; consider using a comparison operator instead
+  }
+}
+} // namespace sei_cert_example_oop57_cpp
+
+namespace inner_padding_64bit_only {
+struct S {
+  int x;
+  int *y;
+};
+
+void test() {
+  S a, b;
+  std::memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of type 'inner_padding_64bit_only::S' which does not have a unique object representation; consider comparing the members of the object manually
+}
+} // namespace inner_padding_64bit_only
+
+namespace padding_in_base {
+class Base {
+  char c;
+  int i;
+};
+
+class Derived : public Base {};
+
+class Derived2 : public Derived {};
+
+void testDerived() {
+  Derived a, b;
+  std::memcmp(, , sizeof(Base));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of type 'padding_in_base::Derived' which does not have a unique object representation; consider comparing the members of the object manually
+  std::memcmp(, , sizeof(Derived));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of type 'padding_in_base::Derived' which does not have a unique object representation; consider comparing the members of the object manually
+}
+
+void testDerived2() {
+  Derived2 a, b;
+  std::memcmp(, , sizeof(Base));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of type 'padding_in_base::Derived2' which does not have a unique object representation; consider comparing the members of the object manually
+  std::memcmp(, , sizeof(Derived2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of type 'padding_in_base::Derived2' which does not have a unique object representation; consider comparing the members of the object manually
+}
+
+} // namespace padding_in_base
+
+namespace no_padding_in_base {
+class Base {
+  int a, b;
+};
+
+class Derived : public Base {};
+
+class Derived2 : public Derived {};
+
+void testDerived() {
+  Derived a, b;
+  std::memcmp(, , sizeof(Base));
+  std::memcmp(, , sizeof(Derived));
+}
+
+void testDerived2() {
+  Derived2 a, b;
+  std::memcmp(, , sizeof(char));
+  std::memcmp(, , sizeof(Base));
+  std::memcmp(, , sizeof(Derived2));
+}
+} // namespace no_padding_in_base
+
+namespace non_standard_layout {
+class C {
+private:
+  int x;
+
+public:
+  int y;
+};
+
+void test() {
+  C a, b;
+  std::memcmp(, , sizeof(C));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of non-standard-layout type 'non_standard_layout::C'; consider using a comparison operator instead
+}
+
+} // namespace non_standard_layout
+
+namespace static_ignored {
+struct S {
+  static char c;
+  int i;
+};
+
+void test() {
+  S a, b;
+  std::memcmp(, , sizeof(S));
+}
+} // namespace static_ignored
+
+namespace operator_void_ptr {
+struct S {
+  operator void *() const;
+};
+
+void test() {
+  S s;
+  std::memcmp(s, s, sizeof(s));
+}
+} // namespace operator_void_ptr

[PATCH] D89651: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze marked an inline comment as done.
gbencze added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:45
+
+  for (unsigned int i = 0; i < 2; ++i) {
+const Expr *ArgExpr = CE->getArg(i);

aaron.ballman wrote:
> The lint warning here is actually correct, which is a lovely change of pace.
Changed to `ArgIndex`



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:70
+  if (ComparedBits.hasValue() && *ComparedBits >= PointeeSize &&
+  !Ctx.hasUniqueObjectRepresentations(PointeeQualifiedType)) {
+diag(CE->getBeginLoc(),

aaron.ballman wrote:
> Note that this may produce false positives as the list of objects with unique 
> representations is not complete. For instance, it doesn't handle _Complex or 
> _Atomic types, etc.
Yes, this could definitely cause some false positives. I'm not sure how we 
could easily fix it thought, especially if they are in some nested record.
Do you think this is a serious issue?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:73
+ "comparing object representation of type %0 which does not have "
+ "unique object representations; consider comparing the values "
+ "manually")

aaron.ballman wrote:
> unique object representations -> a unique object representation
> 
> WDYT about:
> consider comparing the values manually -> consider comparing %select{the 
> values|the members of the object}0 manually
> 
> to make it more clear that these cases are different:
> ```
> memcmp(_float, _other_float, sizeof(float));
> memcmp(_struct, _other_struct, sizeof(some_struct));
> ```
> The use of "values" make it sound a bit like the user should be able to do 
> `if (some_struct == some_other_struct)` to me.
I wasn't entirely happy with the wording either; changed it according to your 
suggestions. 



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison-32bits.cpp:2
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t \
+// RUN: -- -- -target x86_64-unknown-unknown -m32
+

aaron.ballman wrote:
> Wouldn't picking a 32-bit target suffice instead of `-m32`? e.g., `-target 
> i386-unknown-unknown`
To be fully honest, I have no idea, I saw some other tests using `-m32` so I 
decided to copy that. 



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c:121
+  memcmp(, , sizeof(int)); // no-warning: not comparing entire object
+  memcmp(, , 2 * sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation 
of

aaron.ballman wrote:
> Just to make it obvious, I think a test like this would also be handy:
> ```
> struct Whatever {
>   int i[2];
>   char c;
> };
> 
> struct Whatever one, two;
> memcmp(, , 2 * sizeof(int)); // Shouldn't warn either
> ```
> which brings up a pathological case that I have no idea how it should behave:
> ```
> struct Whatever {
>   int i[2];
>   int : 0; // What now?!
> };
> 
> struct Whatever one, two;
> memcmp(, , 2 * sizeof(int)); // Warn? Don't Warn? Cry?
> ```
Added two new test cases for these: `Test_TrailingPadding2` and 
`Bitfield_TrailingUnnamed`. 
Purely empirically the latter one seems to have a size of `2*sizeof(int)`, so I 
assume there is no need to warn there. 



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp:229
+  // consider comparing the values manually
+}
+} // namespace alignment

aaron.ballman wrote:
> Another case we should be careful of is template instantiations:
> ```
> template 
> void func(const Ty *o1, const Ty *o2) {
>   memcmp(o1, o2, sizeof(Ty));
> }
> ```
> We don't want to diagnose that unless it's been instantiated with types that 
> are a problem.
Thanks, I added two new tests for these. I also made a minor fix in 
`registerMatchers` to make sure I don't try to evaluate a dependant expression. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89651/new/

https://reviews.llvm.org/D89651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89651: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze updated this revision to Diff 303643.
gbencze added a comment.

- Added some new test cases
- Fixed assertion in templates
- Changed loop variable name from `i` to `ArgIndex`
- Changed wording of warnings
- Changed CHECK-MESSAGES to be in a single line: turns out that only the first 
line is checked as a prefix if clang-tidy breaks it into multiple lines


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89651/new/

https://reviews.llvm.org/D89651

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-exp42-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-flp37-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison-32bits.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
@@ -0,0 +1,231 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t \
+// RUN: -- -- -target x86_64-unknown-unknown
+
+namespace std {
+typedef __SIZE_TYPE__ size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);
+} // namespace std
+
+namespace sei_cert_example_oop57_cpp {
+class C {
+  int i;
+
+public:
+  virtual void f();
+};
+
+void f(C , C ) {
+  if (!std::memcmp(, , sizeof(C))) {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: comparing object representation of non-standard-layout type 'sei_cert_example_oop57_cpp::C'; consider using a comparison operator instead
+  }
+}
+} // namespace sei_cert_example_oop57_cpp
+
+namespace inner_padding_64bit_only {
+struct S {
+  int x;
+  int *y;
+};
+
+void test() {
+  S a, b;
+  std::memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of type 'inner_padding_64bit_only::S' which does not have a unique object representation; consider comparing the members of the object manually
+}
+} // namespace inner_padding_64bit_only
+
+namespace padding_in_base {
+class Base {
+  char c;
+  int i;
+};
+
+class Derived : public Base {};
+
+class Derived2 : public Derived {};
+
+void testDerived() {
+  Derived a, b;
+  std::memcmp(, , sizeof(Base));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of type 'padding_in_base::Derived' which does not have a unique object representation; consider comparing the members of the object manually
+  std::memcmp(, , sizeof(Derived));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of type 'padding_in_base::Derived' which does not have a unique object representation; consider comparing the members of the object manually
+}
+
+void testDerived2() {
+  Derived2 a, b;
+  std::memcmp(, , sizeof(Base));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of type 'padding_in_base::Derived2' which does not have a unique object representation; consider comparing the members of the object manually
+  std::memcmp(, , sizeof(Derived2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of type 'padding_in_base::Derived2' which does not have a unique object representation; consider comparing the members of the object manually
+}
+
+} // namespace padding_in_base
+
+namespace no_padding_in_base {
+class Base {
+  int a, b;
+};
+
+class Derived : public Base {};
+
+class Derived2 : public Derived {};
+
+void testDerived() {
+  Derived a, b;
+  std::memcmp(, , sizeof(Base));
+  std::memcmp(, , sizeof(Derived));
+}
+
+void testDerived2() {
+  Derived2 a, b;
+  std::memcmp(, , sizeof(char));
+  std::memcmp(, , sizeof(Base));
+  std::memcmp(, , sizeof(Derived2));
+}
+} // namespace no_padding_in_base
+
+namespace non_standard_layout {
+class C {
+private:
+  int x;
+
+public:
+  int y;
+};
+
+void test() {
+  C a, b;
+  std::memcmp(, , sizeof(C));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of non-standard-layout type 'non_standard_layout::C'; consider using a comparison operator instead
+}
+
+} // namespace non_standard_layout
+
+namespace static_ignored {
+struct S {
+  static char c;
+  int i;
+};
+
+void test() {
+  S a, b;
+  std::memcmp(, , sizeof(S));
+}
+} // namespace static_ignored
+
+namespace operator_void_ptr {

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added inline comments.



Comment at: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify 
-std=c++2a %s
+//  expected-no-diagnostics

Just to be sure: is the specifying the triple here enough to ensure that this 
always uses the Itanium ABI? I believe MSVC currently ignores the 
`no_unique_address` attribute. Or do I need to add some more flags?
Alternatively, the static_asserts could be modified to check `sizeof(T) > 
[expected size with Itanium ABI] || __has_unique_object_representations(T)`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze updated this revision to Diff 303082.
gbencze added a comment.
Herald added a subscriber: mgrang.

Sorry for the slow update on this. 
I fixed the behavior when reusing tail padding as mentioned by @rsmith and took 
a shot at unifying the code paths for base classes and fields. Let me know your 
thoughts on the approach!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp

Index: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -std=c++2a %s
+//  expected-no-diagnostics
+
+struct Empty {};
+
+struct A {
+  [[no_unique_address]] Empty e;
+  char x;
+};
+
+static_assert(__has_unique_object_representations(A));
+
+struct B {
+  char x;
+  [[no_unique_address]] Empty e;
+};
+
+static_assert(__has_unique_object_representations(B));
+
+struct C {
+  char x;
+  [[no_unique_address]] Empty e1;
+  [[no_unique_address]] Empty e2;
+};
+
+static_assert(!__has_unique_object_representations(C));
+
+namespace TailPaddingReuse {
+struct A {
+private:
+  int a;
+
+public:
+  char b;
+};
+
+struct B {
+  [[no_unique_address]] A a;
+  char c[3];
+};
+} // namespace TailPaddingReuse
+static_assert(__has_unique_object_representations(TailPaddingReuse::B));
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -2549,16 +2549,66 @@
   return !RD->field_empty();
 }
 
-static bool isStructEmpty(QualType Ty) {
-  const RecordDecl *RD = Ty->castAs()->getDecl();
+static int64_t getSubobjectOffset(const FieldDecl *Field,
+  const ASTContext ,
+  const clang::ASTRecordLayout & /*Layout*/) {
+  return Context.getFieldOffset(Field);
+}
 
-  if (!RD->field_empty())
-return false;
+static int64_t getSubobjectOffset(const CXXRecordDecl *RD,
+  const ASTContext ,
+  const clang::ASTRecordLayout ) {
+  return Context.toBits(Layout.getBaseClassOffset(RD));
+}
 
-  if (const auto *ClassDecl = dyn_cast(RD))
-return ClassDecl->isEmpty();
+static llvm::Optional
+structHasUniqueObjectRepresentations(const ASTContext ,
+ const RecordDecl *RD);
 
-  return true;
+static llvm::Optional
+getSubobjectSizeInBits(const FieldDecl *Field, const ASTContext ) {
+  if (Field->getType()->isRecordType()) {
+const RecordDecl *RD = Field->getType()->getAsRecordDecl();
+if (!RD->isUnion())
+  return structHasUniqueObjectRepresentations(Context, RD);
+  }
+  if (!Field->getType()->isReferenceType() &&
+  !Context.hasUniqueObjectRepresentations(Field->getType()))
+return llvm::None;
+
+  int64_t FieldSizeInBits =
+  Context.toBits(Context.getTypeSizeInChars(Field->getType()));
+  if (Field->isBitField()) {
+int64_t BitfieldSize = Field->getBitWidthValue(Context);
+if (BitfieldSize > FieldSizeInBits)
+  return llvm::None;
+FieldSizeInBits = BitfieldSize;
+  }
+  return FieldSizeInBits;
+}
+
+static llvm::Optional
+getSubobjectSizeInBits(const CXXRecordDecl *RD, const ASTContext ) {
+  return structHasUniqueObjectRepresentations(Context, RD);
+}
+
+template 
+static llvm::Optional structSubobjectsHaveUniqueObjectRepresentations(
+const RangeT , int64_t CurOffsetInBits, const ASTContext ,
+const clang::ASTRecordLayout ) {
+  for (const auto *Subobject : Subobjects) {
+llvm::Optional SizeInBits =
+getSubobjectSizeInBits(Subobject, Context);
+if (!SizeInBits)
+  return llvm::None;
+if (*SizeInBits != 0) {
+  int64_t Offset = getSubobjectOffset(Subobject, Context, Layout);
+  if (Offset != CurOffsetInBits)
+return llvm::None;
+  CurOffsetInBits += *SizeInBits;
+}
+  }
+  return CurOffsetInBits;
 }
 
 static llvm::Optional
@@ -2572,58 +2622,32 @@
 if (ClassDecl->isDynamicClass())
   return llvm::None;
 
-SmallVector, 4> Bases;
+SmallVector Bases;
 for (const auto  : ClassDecl->bases()) {
   // Empty types can be inherited from, and non-empty types can potentially
   // have tail padding, so just make sure there isn't an error.
-  if (!isStructEmpty(Base.getType())) {
-llvm::Optional Size = structHasUniqueObjectRepresentations(
-Context, Base.getType()->castAs()->getDecl());
-if (!Size)
-  return llvm::None;
-Bases.emplace_back(Base.getType(), Size.getValue());
-  }
+  Bases.emplace_back(Base.getType()->getAsCXXRecordDecl ());
 }
 
-llvm::sort(Bases, [&](const std::pair ,
-  const 

[PATCH] D89651: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added a comment.

In D89651#2338266 , @njames93 wrote:

> Should point out there is already a check for cert-oop57-cpp, added in D72488 
> . Do these play nice with each other or 
> should they perhaps be merged or share code?

I would certainly expect some duplicate warnings with there two checks, but as 
far as I can tell that check does not currently warn on using `memcmp` with 
non-standard-layout types. 
I personally would leave the exp42 and flp37 parts of this check as separate, 
because those are useful in C as well. But maybe it'd be better to move the 
warning for non-standard-layout types from here to the existing one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89651/new/

https://reviews.llvm.org/D89651

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:2580-2581
   if (!isStructEmpty(Base.getType())) {
 llvm::Optional Size = structHasUniqueObjectRepresentations(
 Context, Base.getType()->castAs()->getDecl());
 if (!Size)

rsmith wrote:
> We need to do this for non-empty `[[no_unique_address]]` members of class 
> type too, to handle tail padding reuse in cases such as:
> 
> ```
> struct A {
> ~A();
> int a;
> char b;
> };
> struct B {
> [[no_unique_address]] A a;
> char c[3];
> };
> static_assert(sizeof(B) == 8, "");
> static_assert(__has_unique_object_representations(B), "");
> ```
You're right, I missed the case of reusing tail padding. I'll try to update 
this review to include this soon. 

But I don't think that this exact example is incorrect now as `A` is not 
trivially copyable due to the user defined destructor. 
It should return true when the class type member is trivially copyable but 
non-standard-layout though: 

```
struct A {
private:
int a;
public:
char b;
};
struct B {
[[no_unique_address]] A a;
char c[3];
};
static_assert(sizeof(B) == 8, "");
static_assert(__has_unique_object_representations(B), "");
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89649/new/

https://reviews.llvm.org/D89649

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89651: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze created this revision.
gbencze added reviewers: aaron.ballman, JonasToth, Charusso.
gbencze added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.
Herald added a project: clang.
gbencze requested review of this revision.

The check warns on suspicious calls to memcmp.
It currently checks for comparing types that do not have unique object 
representations or are non-standard-layout.
Based on
https://wiki.sei.cmu.edu/confluence/display/c/EXP42-C.+Do+not+compare+padding+data
https://wiki.sei.cmu.edu/confluence/display/c/FLP37-C.+Do+not+use+object+representations+to+compare+floating-point+values
and part of
https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP57-CPP.+Prefer+special+member+functions+and+overloaded+operators+to+C+Standard+Library+functions
Add alias cert-exp42-c and cert-flp37-c.

Some tests are currently failing at head, the check depends on D89649 
.
Originally started in D71973 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89651

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-exp42-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-flp37-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison-32bits.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
@@ -0,0 +1,230 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t \
+// RUN: -- -- -target x86_64-unknown-unknown
+
+namespace std {
+typedef __SIZE_TYPE__ size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);
+} // namespace std
+
+namespace sei_cert_example_oop57_cpp {
+class C {
+  int i;
+
+public:
+  virtual void f();
+};
+
+void f(C , C ) {
+  if (!std::memcmp(, , sizeof(C))) {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: comparing object representation
+// of non-standard-layout type 'sei_cert_example_oop57_cpp::C'; consider
+// using a comparison operator instead
+  }
+}
+} // namespace sei_cert_example_oop57_cpp
+
+namespace inner_padding_64bit_only {
+struct S {
+  int x;
+  int *y;
+};
+
+void test() {
+  S a, b;
+  std::memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of
+  // type 'inner_padding_64bit_only::S' which does not have unique object
+  // representations; consider comparing the values manually
+}
+} // namespace inner_padding_64bit_only
+
+namespace padding_in_base {
+class Base {
+  char c;
+  int i;
+};
+
+class Derived : public Base {};
+
+class Derived2 : public Derived {};
+
+void testDerived() {
+  Derived a, b;
+  std::memcmp(, , sizeof(Base));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of
+  // type 'padding_in_base::Derived' which does not have unique object
+  // representations; consider comparing the values manually
+  std::memcmp(, , sizeof(Derived));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of
+  // type 'padding_in_base::Derived' which does not have unique object
+  // representations; consider comparing the values manually
+}
+
+void testDerived2() {
+  Derived2 a, b;
+  std::memcmp(, , sizeof(Base));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of
+  // type 'padding_in_base::Derived2' which does not have unique object
+  // representations; consider comparing the values manually
+  std::memcmp(, , sizeof(Derived2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation of
+  // type 'padding_in_base::Derived2' which does not have unique object
+  // representations; consider comparing the values manually
+}
+
+} // namespace padding_in_base
+
+namespace no_padding_in_base {
+class Base {
+  int a, b;
+};
+
+class Derived : public Base {};
+
+class Derived2 : public Derived {};
+
+void testDerived() {
+  Derived a, b;
+  std::memcmp(, , sizeof(Base));
+  std::memcmp(, , sizeof(Derived));
+}
+
+void testDerived2() {
+  Derived2 a, b;
+  std::memcmp(, , sizeof(char));
+  std::memcmp(, , sizeof(Base));
+  std::memcmp(, , 

[PATCH] D89649: Fix __has_unique_object_representations with no_unique_address

[Gabor Bencze via Phabricator via cfe-commits]

gbencze created this revision.
gbencze added reviewers: rsmith, aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
gbencze requested review of this revision.

Fix incorrect behavior of __has_unique_object_representations when using the 
no_unique_address attribute.

Based on the bug report: https://bugs.llvm.org/show_bug.cgi?id=47722 



Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89649

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp


Index: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify 
-std=c++2a %s
+//  expected-no-diagnostics
+
+struct Empty {};
+
+struct A {
+  [[no_unique_address]] Empty e;
+  char x;
+};
+
+static_assert(__has_unique_object_representations(A));
+
+struct B {
+  char x;
+  [[no_unique_address]] Empty e;
+};
+
+static_assert(__has_unique_object_representations(B));
+
+struct C {
+  char x;
+  [[no_unique_address]] Empty e1;
+  [[no_unique_address]] Empty e2;
+};
+
+static_assert(!__has_unique_object_representations(C));
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -2602,9 +2602,12 @@
   }
 
   for (const auto *Field : RD->fields()) {
-if (!Field->getType()->isReferenceType() &&
-!Context.hasUniqueObjectRepresentations(Field->getType()))
-  return llvm::None;
+if (!Field->getType()->isReferenceType()) {
+  if (Field->isZeroSize(Context))
+continue;
+  else if (!Context.hasUniqueObjectRepresentations(Field->getType()))
+return llvm::None;
+}
 
 int64_t FieldSizeInBits =
 Context.toBits(Context.getTypeSizeInChars(Field->getType()));


Index: clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/has_unique_object_reps_no_unique_addr.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify -std=c++2a %s
+//  expected-no-diagnostics
+
+struct Empty {};
+
+struct A {
+  [[no_unique_address]] Empty e;
+  char x;
+};
+
+static_assert(__has_unique_object_representations(A));
+
+struct B {
+  char x;
+  [[no_unique_address]] Empty e;
+};
+
+static_assert(__has_unique_object_representations(B));
+
+struct C {
+  char x;
+  [[no_unique_address]] Empty e1;
+  [[no_unique_address]] Empty e2;
+};
+
+static_assert(!__has_unique_object_representations(C));
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -2602,9 +2602,12 @@
   }
 
   for (const auto *Field : RD->fields()) {
-if (!Field->getType()->isReferenceType() &&
-!Context.hasUniqueObjectRepresentations(Field->getType()))
-  return llvm::None;
+if (!Field->getType()->isReferenceType()) {
+  if (Field->isZeroSize(Context))
+continue;
+  else if (!Context.hasUniqueObjectRepresentations(Field->getType()))
+return llvm::None;
+}
 
 int64_t FieldSizeInBits =
 Context.toBits(Context.getTypeSizeInChars(Field->getType()));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added a comment.

ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71973/new/

https://reviews.llvm.org/D71973



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added a comment.

Another option that came to my mind is using a BitVector to (recursively) flag 
bits that are occupied by the fields. This solution would be slightly slower 
and uses more memory but is probably a lot easier to understand, maintain and 
more robust than the currently proposed implementation.  This would also catch 
a few additional cases as it could also look inside unions.

I stared to experiment with an implementation of this here 
.

Which direction do you think would be a better solution?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71973/new/

https://reviews.llvm.org/D71973



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:82-83
+
+static bool hasPadding(const ASTContext , const RecordDecl *RD,
+   uint64_t ComparedBits) {
+  assert(RD && RD->isCompleteDefinition() &&

aaron.ballman wrote:
> I am surprised that we have to do this much manual work, I would have 
> expected this to be something that `RecordLayout` would handle for us. I am a 
> bit worried about this manual work being split in a few places because we're 
> likely to get it wrong in at least one of them. For instance, this doesn't 
> seem to be taking into account things like `[[no_unique_address]]` or 
> alignment when considering the layout of fields.
> 
> I sort of wonder if we should try using the `RecordLayout` class to do this 
> work instead, even if that requires larger changes.
Thanks, I didn't realize the [[no_unique_address]] attribute existed but I 
fixed it (in this check for now) and also added some tests to cover it as well 
as alignas. 

If you think this information should be provided by RecordLayout I'm willing to 
work on that as well (though I guess those changes should be in a different 
patch). Do you have any ideas as to how it should expose the necessary 
information? 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71973/new/

https://reviews.llvm.org/D71973



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze updated this revision to Diff 237529.
gbencze added a comment.

Address (most of the) comments by @aaron.ballman

- remove top-level `const` on locals
- move declaration into `if`
- pass `TagDecl` to diag
- added test for `operator void *`
- fixed `[[no_unique_address]]`
  - remove assertion that checks for overlapping fields
  - in `hasPaddingBetweenFields`: only do recursive call and add field size to 
`TotalSize` if not `isZeroSize`
  - + added tests


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71973/new/

https://reviews.llvm.org/D71973

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-exp42-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison-32bits.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
@@ -0,0 +1,228 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t \
+// RUN: -- -- -target x86_64-unknown-unknown
+
+namespace std {
+typedef __SIZE_TYPE__ size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);
+} // namespace std
+
+namespace sei_cert_example_oop57_cpp {
+class C {
+  int i;
+
+public:
+  virtual void f();
+};
+
+void f(C , C ) {
+  if (!std::memcmp(, , sizeof(C))) {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: comparing object representation
+// of non-standard-layout type sei_cert_example_oop57_cpp::C; consider
+// using a comparison operator instead
+  }
+}
+} // namespace sei_cert_example_oop57_cpp
+
+namespace inner_padding_64bit_only {
+struct S {
+  int x;
+  int *y;
+};
+
+void test() {
+  S a, b;
+  std::memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // inner_padding_64bit_only::S; consider comparing the fields manually
+}
+} // namespace inner_padding_64bit_only
+
+namespace padding_in_base {
+class Base {
+  char c;
+  int i;
+};
+
+class Derived : public Base {};
+
+class Derived2 : public Derived {};
+
+void testDerived() {
+  Derived a, b;
+  std::memcmp(, , sizeof(char));
+  std::memcmp(, , sizeof(Base));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // padding_in_base::Derived; consider comparing the fields manually
+  std::memcmp(, , sizeof(Derived));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // padding_in_base::Derived; consider comparing the fields manually
+}
+
+void testDerived2() {
+  Derived2 a, b;
+  std::memcmp(, , sizeof(char));
+  std::memcmp(, , sizeof(Base));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // padding_in_base::Derived2; consider comparing the fields manually
+  std::memcmp(, , sizeof(Derived2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // padding_in_base::Derived2; consider comparing the fields manually
+}
+
+} // namespace padding_in_base
+
+namespace no_padding_in_base {
+class Base {
+  int a, b;
+};
+
+class Derived : public Base {};
+
+class Derived2 : public Derived {};
+
+void testDerived() {
+  Derived a, b;
+  std::memcmp(, , sizeof(Base));
+  std::memcmp(, , sizeof(Derived));
+}
+
+void testDerived2() {
+  Derived2 a, b;
+  std::memcmp(, , sizeof(char));
+  std::memcmp(, , sizeof(Base));
+  std::memcmp(, , sizeof(Derived2));
+}
+} // namespace no_padding_in_base
+
+namespace non_standard_layout {
+class C {
+private:
+  int x;
+
+public:
+  int y;
+};
+
+void test() {
+  C a, b;
+  std::memcmp(, , sizeof(C));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation
+  // of non-standard-layout type non_standard_layout::C; consider using a
+  // comparison operator instead
+}
+
+} // namespace non_standard_layout
+
+namespace static_ignored {
+struct S {
+  static char c;
+  int i;
+};
+
+void test() {
+  S a, b;
+  std::memcmp(, , sizeof(S));
+}
+} // namespace static_ignored
+
+namespace operator_void_ptr {
+struct S {
+  operator void *() const;
+};
+
+void test() {
+  S s;
+  std::memcmp(s, s, sizeof(s));
+}
+} // namespace operator_void_ptr
+
+namespace empty_struct {
+struct S {};
+
+void test() {
+  S a, b;
+  

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added a comment.

ping @aaron.ballman - any thoughts on the patch?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71973/new/

https://reviews.llvm.org/D71973



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze marked 4 inline comments as done.
gbencze added a comment.

Thanks for all the feedback @JonasToth  :)




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst:10
+This check corresponds to the CERT C Coding Standard rule
+`EXP42-C. Do not compare padding data
+`_.

JonasToth wrote:
> Maybe this link is not proper, because of the newline. could you please check 
> if the documentation builds? (you need sphinx for that and enable it in 
> cmake.)
It seems to work fine



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:75
`bugprone-suspicious-enum-usage `_,
+   `bugprone-suspicious-memory-comparison 
`_,
`bugprone-suspicious-memset-usage 
`_, "Yes"

JonasToth wrote:
> i believe the classification of the checks was changed today. did you rebase 
> to master? hopefully it still applies clean.
I did rebase today and removed the classification for these two by hand


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71973/new/

https://reviews.llvm.org/D71973



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze updated this revision to Diff 235965.
gbencze added a comment.

Punctuation in comment


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71973/new/

https://reviews.llvm.org/D71973

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-exp42-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison-32bits.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t \
+// RUN: -- -- -target x86_64-unknown-unknown
+
+namespace std {
+typedef __SIZE_TYPE__ size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);
+} // namespace std
+
+namespace sei_cert_example_oop57_cpp {
+class C {
+  int i;
+
+public:
+  virtual void f();
+};
+
+void f(C , C ) {
+  if (!std::memcmp(, , sizeof(C))) {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: comparing object representation
+// of non-standard-layout type sei_cert_example_oop57_cpp::C; consider
+// using a comparison operator instead
+  }
+}
+} // namespace sei_cert_example_oop57_cpp
+
+namespace inner_padding_64bit_only {
+struct S {
+  int x;
+  int *y;
+};
+
+void test() {
+  S a, b;
+  std::memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // inner_padding_64bit_only::S; consider comparing the fields manually
+}
+} // namespace inner_padding_64bit_only
+
+namespace padding_in_base {
+class Base {
+  char c;
+  int i;
+};
+
+class Derived : public Base {};
+
+class Derived2 : public Derived {};
+
+void testDerived() {
+  Derived a, b;
+  std::memcmp(, , sizeof(char));
+  std::memcmp(, , sizeof(Base));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // padding_in_base::Derived; consider comparing the fields manually
+  std::memcmp(, , sizeof(Derived));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // padding_in_base::Derived; consider comparing the fields manually
+}
+
+void testDerived2() {
+  Derived2 a, b;
+  std::memcmp(, , sizeof(char));
+  std::memcmp(, , sizeof(Base));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // padding_in_base::Derived2; consider comparing the fields manually
+  std::memcmp(, , sizeof(Derived2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // padding_in_base::Derived2; consider comparing the fields manually
+}
+
+} // namespace padding_in_base
+
+namespace no_padding_in_base {
+class Base {
+  int a, b;
+};
+
+class Derived : public Base {};
+
+class Derived2 : public Derived {};
+
+void testDerived() {
+  Derived a, b;
+  std::memcmp(, , sizeof(Base));
+  std::memcmp(, , sizeof(Derived));
+}
+
+void testDerived2() {
+  Derived2 a, b;
+  std::memcmp(, , sizeof(char));
+  std::memcmp(, , sizeof(Base));
+  std::memcmp(, , sizeof(Derived2));
+}
+} // namespace no_padding_in_base
+
+namespace non_standard_layout {
+class C {
+private:
+  int x;
+
+public:
+  int y;
+};
+
+void test() {
+  C a, b;
+  std::memcmp(, , sizeof(C));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation
+  // of non-standard-layout type non_standard_layout::C; consider using a
+  // comparison operator instead
+}
+
+} // namespace non_standard_layout
+
+namespace static_ignored {
+struct S {
+  static char c;
+  int i;
+};
+
+void test() {
+  S a, b;
+  std::memcmp(, , sizeof(S));
+}
+
+} // namespace static_ignored
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c
@@ -0,0 +1,224 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t \
+// RUN: -- -- -target x86_64-unknown-unknown -std=c99
+
+typedef __SIZE_TYPE__ size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);
+
+// Examples from cert rule exp42-c
+
+struct S {
+  char c;
+  int i;
+  char buffer[13];
+};
+
+void noncompliant(const 

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze updated this revision to Diff 235915.
gbencze added a comment.

Tests: Split C/C++ tests and add 32/64bit specific test.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71973/new/

https://reviews.llvm.org/D71973

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-exp42-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison-32bits.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t \
+// RUN: -- -- -target x86_64-unknown-unknown
+
+namespace std {
+typedef __SIZE_TYPE__ size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);
+} // namespace std
+
+namespace sei_cert_example_oop57_cpp {
+class C {
+  int i;
+
+public:
+  virtual void f();
+};
+
+void f(C , C ) {
+  if (!std::memcmp(, , sizeof(C))) {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: comparing object representation
+// of non-standard-layout type sei_cert_example_oop57_cpp::C; consider
+// using a comparison operator instead
+  }
+}
+} // namespace sei_cert_example_oop57_cpp
+
+namespace inner_padding_64bit_only {
+struct S {
+  int x;
+  int *y;
+};
+
+void test() {
+  S a, b;
+  std::memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // inner_padding_64bit_only::S; consider comparing the fields manually
+}
+} // namespace inner_padding_64bit_only
+
+namespace padding_in_base {
+class Base {
+  char c;
+  int i;
+};
+
+class Derived : public Base {};
+
+class Derived2 : public Derived {};
+
+void testDerived() {
+  Derived a, b;
+  std::memcmp(, , sizeof(char));
+  std::memcmp(, , sizeof(Base));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // padding_in_base::Derived; consider comparing the fields manually
+  std::memcmp(, , sizeof(Derived));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // padding_in_base::Derived; consider comparing the fields manually
+}
+
+void testDerived2() {
+  Derived2 a, b;
+  std::memcmp(, , sizeof(char));
+  std::memcmp(, , sizeof(Base));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // padding_in_base::Derived2; consider comparing the fields manually
+  std::memcmp(, , sizeof(Derived2));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // padding_in_base::Derived2; consider comparing the fields manually
+}
+
+} // namespace padding_in_base
+
+namespace no_padding_in_base {
+class Base {
+  int a, b;
+};
+
+class Derived : public Base {};
+
+class Derived2 : public Derived {};
+
+void testDerived() {
+  Derived a, b;
+  std::memcmp(, , sizeof(Base));
+  std::memcmp(, , sizeof(Derived));
+}
+
+void testDerived2() {
+  Derived2 a, b;
+  std::memcmp(, , sizeof(char));
+  std::memcmp(, , sizeof(Base));
+  std::memcmp(, , sizeof(Derived2));
+}
+} // namespace no_padding_in_base
+
+namespace non_standard_layout {
+class C {
+private:
+  int x;
+
+public:
+  int y;
+};
+
+void test() {
+  C a, b;
+  std::memcmp(, , sizeof(C));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing object representation
+  // of non-standard-layout type non_standard_layout::C; consider using a
+  // comparison operator instead
+}
+
+} // namespace non_standard_layout
+
+namespace static_ignored {
+struct S {
+  static char c;
+  int i;
+};
+
+void test() {
+  S a, b;
+  std::memcmp(, , sizeof(S));
+}
+
+} // namespace static_ignored
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c
@@ -0,0 +1,224 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t \
+// RUN: -- -- -target x86_64-unknown-unknown -std=c99
+
+typedef __SIZE_TYPE__ size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);
+
+// Examples from cert rule exp42-c
+
+struct S {
+  char c;
+  int i;
+  char 

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze marked 18 inline comments as done.
gbencze added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp:1
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t
+

JonasToth wrote:
> We should explicitly test, that the check runs under C-code, either with two 
> run-lines or with separate test files (my preference).
Should I duplicate the tests that are legal C or try to split it up so that 
only c++ specific code is tested in this one? 



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp:3
+
+typedef unsigned long long size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);

JonasToth wrote:
> I think the test could be sensitive to cpu-architectures.
> If you could bring up a case that is e.g. problematic on X86, but not on 
> anything else (powerpc, or arm or anything, 64bit vs 32bit) and demonstrate 
> with tests that such cases are caught as well this would be great. I see no 
> reason it wouldn't.
> 
> That would maybe justify another alias into `portability`, as this is an 
> issue in that case.
I may have misunderstood you but the check currently only warns if you're 
comparing padding on the current compilation target. 
Or do you mean adding another test file that is compiled e.g. with -m32 and 
testing if calling memcmp on the following doesn't warn there but does on 64bit?
```
struct S {
int x;
int* y;
};
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71973/new/

https://reviews.llvm.org/D71973



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze updated this revision to Diff 235900.
gbencze added a comment.

Coding guide and better diagnostic message for padding comparison


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71973/new/

https://reviews.llvm.org/D71973

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-exp42-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
@@ -0,0 +1,373 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t
+
+typedef __SIZE_TYPE__ size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);
+
+namespace std {
+int memcmp(const void *lhs, const void *rhs, size_t count);
+}
+
+namespace sei_cert_example_exp42_c {
+struct s {
+  char c;
+  int i;
+  char buffer[13];
+};
+
+void noncompliant(const struct s *left, const struct s *right) {
+  if ((left && right) && (0 == memcmp(left, right, sizeof(struct s {
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: comparing padding data in type
+// sei_cert_example_exp42_c::s; consider comparing the fields manually
+  }
+}
+
+void compliant(const struct s *left, const struct s *right) {
+  if ((left && right) && (left->c == right->c) && (left->i == right->i) &&
+  (0 == memcmp(left->buffer, right->buffer, 13))) {
+  }
+}
+} // namespace sei_cert_example_exp42_c
+
+namespace sei_cert_example_exp42_c_ex1 {
+#pragma pack(push, 1)
+struct s {
+  char c;
+  int i;
+  char buffer[13];
+};
+#pragma pack(pop)
+
+void compare(const struct s *left, const struct s *right) {
+  if ((left && right) && (0 == memcmp(left, right, sizeof(struct s {
+// no-warning
+  }
+}
+} // namespace sei_cert_example_exp42_c_ex1
+
+namespace sei_cert_example_oop57_cpp {
+class C {
+  int i;
+
+public:
+  virtual void f();
+};
+
+void f(C , C ) {
+  if (!std::memcmp(, , sizeof(C))) {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: comparing object representation
+// of non-standard-layout type sei_cert_example_oop57_cpp::C; consider
+// using a comparison operator instead
+  }
+}
+} // namespace sei_cert_example_oop57_cpp
+
+namespace trailing_padding {
+struct S {
+  int i;
+  char c;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // trailing_padding::S; consider comparing the fields manually
+  memcmp(, , sizeof(int));
+  memcmp(, , sizeof(int) + sizeof(char));
+  memcmp(, , 2 * sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // trailing_padding::S; consider comparing the fields manually
+}
+} // namespace trailing_padding
+
+namespace inner_padding {
+struct S {
+  char c;
+  int i;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // inner_padding::S; consider comparing the fields manually
+  memcmp(, , sizeof(char) + sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // inner_padding::S; consider comparing the fields manually
+  memcmp(, , sizeof(char));
+  memcmp(, , 2 * sizeof(char));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // inner_padding::S; consider comparing the fields manually
+}
+} // namespace inner_padding
+
+namespace bitfield {
+struct S {
+  int x : 10;
+  int y : 6;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // bitfield::S; consider comparing the fields manually
+  memcmp(, , 2); // no-warning: no padding in first 2 bytes
+}
+} // namespace bitfield
+
+namespace bitfield_2 {
+struct S {
+  int x : 10;
+  int y : 7;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // bitfield_2::S; consider comparing the fields manually
+  memcmp(, , 2);
+  memcmp(, , 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding data in type
+  // bitfield_2::S; consider comparing the fields manually
+}
+} // namespace bitfield_2
+
+namespace bitfield_3 {
+struct S {
+  int x : 2;
+  int : 0;
+  int y : 6;
+};
+
+void test() {
+  S a, b;
+  

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze updated this revision to Diff 235564.
gbencze added a comment.

ReleaseNotes: move alias after new checks


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71973/new/

https://reviews.llvm.org/D71973

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-exp42-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
@@ -0,0 +1,352 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t
+
+typedef unsigned long long size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);
+
+namespace std {
+int memcmp(const void *lhs, const void *rhs, size_t count);
+}
+
+namespace sei_cert_example_exp42_c {
+struct s {
+  char c;
+  int i;
+  char buffer[13];
+};
+
+void noncompliant(const struct s *left, const struct s *right) {
+  if ((left && right) && (0 == memcmp(left, right, sizeof(struct s {
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: comparing padding bytes
+  }
+}
+
+void compliant(const struct s *left, const struct s *right) {
+  if ((left && right) && (left->c == right->c) && (left->i == right->i) &&
+  (0 == memcmp(left->buffer, right->buffer, 13))) {
+  }
+}
+} // namespace sei_cert_example_exp42_c
+
+namespace sei_cert_example_exp42_c_ex1 {
+#pragma pack(push, 1)
+struct s {
+  char c;
+  int i;
+  char buffer[13];
+};
+#pragma pack(pop)
+
+void compare(const struct s *left, const struct s *right) {
+  if ((left && right) && (0 == memcmp(left, right, sizeof(struct s {
+// no-warning
+  }
+}
+} // namespace sei_cert_example_exp42_c_ex1
+
+namespace sei_cert_example_oop57_cpp {
+class C {
+  int i;
+
+public:
+  virtual void f();
+};
+
+void f(C , C ) {
+  if (!std::memcmp(, , sizeof(C))) {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: comparing object representation
+// of non-standard-layout type 'sei_cert_example_oop57_cpp::C'; consider
+// using a comparison operator instead
+  }
+}
+} // namespace sei_cert_example_oop57_cpp
+
+namespace trailing_padding {
+struct S {
+  int i;
+  char c;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , sizeof(int));
+  memcmp(, , sizeof(int) + sizeof(char));
+  memcmp(, , 2 * sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace trailing_padding
+
+namespace inner_padding {
+struct S {
+  char c;
+  int i;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , sizeof(char) + sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , sizeof(char));
+  memcmp(, , 2 * sizeof(char));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace inner_padding
+
+namespace bitfield {
+struct S {
+  int x : 10;
+  int y : 6;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , 2); // no-warning: no padding in first 2 bytes
+}
+} // namespace bitfield
+
+namespace bitfield_2 {
+struct S {
+  int x : 10;
+  int y : 7;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , 2);
+  memcmp(, , 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace bitfield_2
+
+namespace bitfield_3 {
+struct S {
+  int x : 2;
+  int : 0;
+  int y : 6;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace bitfield_3
+
+namespace array_no_padding {
+struct S {
+  int x;
+  int y;
+};
+
+void test() {
+  S a[3];
+  S b[3];
+  memcmp(a, b, 3 * sizeof(S));
+}
+} // namespace array_no_padding
+
+namespace array_with_padding {
+struct S {
+  int x;
+  char y;
+};
+
+void test() {
+  S a[3];
+  S b[3];
+  memcmp(a, b, 3 * sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace array_with_padding
+
+namespace unknown_count {
+struct S {
+  char c;
+  int i;
+};
+
+void test(size_t c) {
+  S a;
+  S b;
+  

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze marked 5 inline comments as done.
gbencze added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:103
+
+- New alias :doc:`cert-exp42-c
+  ` to

Eugene.Zelenko wrote:
> Please move into aliases section (in alphabetical order)
I'm not quite sure where these are supposed to be. I can only find one other 
alias (cert-pos44-c) in the release notes, should I put it immediately before 
that? 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71973/new/

https://reviews.llvm.org/D71973



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze updated this revision to Diff 235526.
gbencze added a comment.

Update style based on comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71973/new/

https://reviews.llvm.org/D71973

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-exp42-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
@@ -0,0 +1,352 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t
+
+typedef unsigned long long size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);
+
+namespace std {
+int memcmp(const void *lhs, const void *rhs, size_t count);
+}
+
+namespace sei_cert_example_exp42_c {
+struct s {
+  char c;
+  int i;
+  char buffer[13];
+};
+
+void noncompliant(const struct s *left, const struct s *right) {
+  if ((left && right) && (0 == memcmp(left, right, sizeof(struct s {
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: comparing padding bytes
+  }
+}
+
+void compliant(const struct s *left, const struct s *right) {
+  if ((left && right) && (left->c == right->c) && (left->i == right->i) &&
+  (0 == memcmp(left->buffer, right->buffer, 13))) {
+  }
+}
+} // namespace sei_cert_example_exp42_c
+
+namespace sei_cert_example_exp42_c_ex1 {
+#pragma pack(push, 1)
+struct s {
+  char c;
+  int i;
+  char buffer[13];
+};
+#pragma pack(pop)
+
+void compare(const struct s *left, const struct s *right) {
+  if ((left && right) && (0 == memcmp(left, right, sizeof(struct s {
+// no-warning
+  }
+}
+} // namespace sei_cert_example_exp42_c_ex1
+
+namespace sei_cert_example_oop57_cpp {
+class C {
+  int i;
+
+public:
+  virtual void f();
+};
+
+void f(C , C ) {
+  if (!std::memcmp(, , sizeof(C))) {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: comparing object representation
+// of non-standard-layout type 'sei_cert_example_oop57_cpp::C'; consider
+// using a comparison operator instead
+  }
+}
+} // namespace sei_cert_example_oop57_cpp
+
+namespace trailing_padding {
+struct S {
+  int i;
+  char c;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , sizeof(int));
+  memcmp(, , sizeof(int) + sizeof(char));
+  memcmp(, , 2 * sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace trailing_padding
+
+namespace inner_padding {
+struct S {
+  char c;
+  int i;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , sizeof(char) + sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , sizeof(char));
+  memcmp(, , 2 * sizeof(char));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace inner_padding
+
+namespace bitfield {
+struct S {
+  int x : 10;
+  int y : 6;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , 2); // no-warning: no padding in first 2 bytes
+}
+} // namespace bitfield
+
+namespace bitfield_2 {
+struct S {
+  int x : 10;
+  int y : 7;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , 2);
+  memcmp(, , 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace bitfield_2
+
+namespace bitfield_3 {
+struct S {
+  int x : 2;
+  int : 0;
+  int y : 6;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace bitfield_3
+
+namespace array_no_padding {
+struct S {
+  int x;
+  int y;
+};
+
+void test() {
+  S a[3];
+  S b[3];
+  memcmp(a, b, 3 * sizeof(S));
+}
+} // namespace array_no_padding
+
+namespace array_with_padding {
+struct S {
+  int x;
+  char y;
+};
+
+void test() {
+  S a[3];
+  S b[3];
+  memcmp(a, b, 3 * sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace array_with_padding
+
+namespace unknown_count {
+struct S {
+  char c;
+  int i;
+};
+
+void test(size_t c) {
+  S a;
+  S b;
+  memcmp(, , 

[PATCH] D71973: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze created this revision.
gbencze added reviewers: aaron.ballman, alexfh, JonasToth, Charusso.
gbencze added projects: clang-tools-extra, clang.
Herald added subscribers: cfe-commits, xazax.hun, whisperity, mgorny.

The check warns on suspicious calls to memcmp.
It currently checks for comparing padding and non-standard-layout types.
Based on
https://wiki.sei.cmu.edu/confluence/display/c/EXP42-C.+Do+not+compare+padding+data
and part of
https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP57-CPP.+Prefer+special+member+functions+and+overloaded+operators+to+C+Standard+Library+functions
Add alias cert-exp42-c.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71973

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-exp42-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.cpp
@@ -0,0 +1,352 @@
+// RUN: %check_clang_tidy %s bugprone-suspicious-memory-comparison %t
+
+typedef unsigned long long size_t;
+int memcmp(const void *lhs, const void *rhs, size_t count);
+
+namespace std {
+int memcmp(const void *lhs, const void *rhs, size_t count);
+}
+
+namespace sei_cert_example_exp42_c {
+struct s {
+  char c;
+  int i;
+  char buffer[13];
+};
+
+void noncompliant(const struct s *left, const struct s *right) {
+  if ((left && right) && (0 == memcmp(left, right, sizeof(struct s {
+// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: comparing padding bytes
+  }
+}
+
+void compliant(const struct s *left, const struct s *right) {
+  if ((left && right) && (left->c == right->c) && (left->i == right->i) &&
+  (0 == memcmp(left->buffer, right->buffer, 13))) {
+  }
+}
+} // namespace sei_cert_example_exp42_c
+
+namespace sei_cert_example_exp42_c_ex1 {
+#pragma pack(push, 1)
+struct s {
+  char c;
+  int i;
+  char buffer[13];
+};
+#pragma pack(pop)
+
+void compare(const struct s *left, const struct s *right) {
+  if ((left && right) && (0 == memcmp(left, right, sizeof(struct s {
+// no-warning
+  }
+}
+} // namespace sei_cert_example_exp42_c_ex1
+
+namespace sei_cert_example_oop57_cpp {
+class C {
+  int i;
+
+public:
+  virtual void f();
+};
+
+void f(C , C ) {
+  if (!std::memcmp(, , sizeof(C))) {
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: comparing object representation
+// of non-standard-layout type 'sei_cert_example_oop57_cpp::C'; consider
+// using a comparison operator instead
+  }
+}
+} // namespace sei_cert_example_oop57_cpp
+
+namespace trailing_padding {
+struct S {
+  int i;
+  char c;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , sizeof(int));
+  memcmp(, , sizeof(int) + sizeof(char));
+  memcmp(, , 2 * sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace trailing_padding
+
+namespace inner_padding {
+struct S {
+  char c;
+  int i;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , sizeof(char) + sizeof(int));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , sizeof(char));
+  memcmp(, , 2 * sizeof(char));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace inner_padding
+
+namespace bitfield {
+struct S {
+  int x : 10;
+  int y : 6;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , 2); // no-warning: no padding in first 2 bytes
+}
+} // namespace bitfield
+
+namespace bitfield_2 {
+struct S {
+  int x : 10;
+  int y : 7;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , sizeof(S));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+  memcmp(, , 2);
+  memcmp(, , 3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace bitfield_2
+
+namespace bitfield_3 {
+struct S {
+  int x : 2;
+  int : 0;
+  int y : 6;
+};
+
+void test() {
+  S a, b;
+  memcmp(, , 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: comparing padding bytes
+}
+} // namespace bitfield_3
+
+namespace array_no_padding {
+struct S {
+  int x;
+  int y;
+};
+
+void test() {
+  S a[3];
+  S b[3];
+  

[PATCH] D70052: [clang-tidy] Add misc-mutating-copy check

[Gabor Bencze via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbbc9f6c2ef07: [clang-tidy] Add cert-oop58-cpp check The 
check warns when (a member of) the… (authored by gbencze).

Changed prior to commit:
  https://reviews.llvm.org/D70052?vs=229925=233966#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70052/new/

https://reviews.llvm.org/D70052

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp
  clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-oop58-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp
@@ -0,0 +1,149 @@
+// RUN: %check_clang_tidy %s cert-oop58-cpp %t
+
+// Example test cases from CERT rule
+// https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP58-CPP.+Copy+operations+must+not+mutate+the+source+object
+namespace test_mutating_noncompliant_example {
+class A {
+  mutable int m;
+
+public:
+  A() : m(0) {}
+  explicit A(int m) : m(m) {}
+
+  A(const A ) : m(other.m) {
+other.m = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+  }
+
+  A =(const A ) {
+if ( != this) {
+  m = other.m;
+  other.m = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: mutating copied object
+}
+return *this;
+  }
+
+  int get_m() const { return m; }
+};
+} // namespace test_mutating_noncompliant_example
+
+namespace test_mutating_compliant_example {
+class B {
+  int m;
+
+public:
+  B() : m(0) {}
+  explicit B(int m) : m(m) {}
+
+  B(const B ) : m(other.m) {}
+  B(B &) : m(other.m) {
+other.m = 0; //no-warning: mutation allowed in move constructor
+  }
+
+  B =(const B ) {
+if ( != this) {
+  m = other.m;
+}
+return *this;
+  }
+
+  B =(B &) {
+m = other.m;
+other.m = 0; //no-warning: mutation allowed in move assignment operator
+return *this;
+  }
+
+  int get_m() const { return m; }
+};
+} // namespace test_mutating_compliant_example
+
+namespace test_mutating_pointer {
+class C {
+  C *ptr;
+  int value;
+
+  C();
+  C(C ) {
+other = {};
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.ptr = nullptr;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.value = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+
+// no-warning: mutating a pointee is allowed
+other.ptr->value = 0;
+*other.ptr = {};
+  }
+};
+} // namespace test_mutating_pointer
+
+namespace test_mutating_indirect_member {
+struct S {
+  int x;
+};
+
+class D {
+  S s;
+  D(D ) {
+other.s = {};
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.s.x = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+  }
+};
+} // namespace test_mutating_indirect_member
+
+namespace test_mutating_other_object {
+class E {
+  E();
+  E(E ) {
+E tmp;
+// no-warning: mutating an object that is not the source is allowed
+tmp = {};
+  }
+};
+} // namespace test_mutating_other_object
+
+namespace test_mutating_member_function {
+class F {
+  int a;
+
+public:
+  void bad_func() { a = 12; }
+  void fine_func() const;
+  void fine_func_2(int x) { x = 5; }
+  void questionable_func();
+
+  F(F ) : a(other.a) {
+this->bad_func(); // no-warning: mutating this is allowed
+
+other.bad_func();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: call mutates copied object
+
+other.fine_func();
+other.fine_func_2(42);
+other.questionable_func();
+  }
+};
+} // namespace test_mutating_member_function
+
+namespace test_mutating_function_on_nested_object {
+struct S {
+  int x;
+  void mutate(int y) {
+x = y;
+  }
+};
+
+class G {
+  S s;
+  G(G ) {
+s.mutate(0); // no-warning: mutating this is allowed
+
+other.s.mutate(0);
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: call mutates copied object
+  }
+};
+} // namespace test_mutating_function_on_nested_object
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -107,6 +107,7 @@
cert-msc51-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) 
+   cert-oop58-cpp
cert-pos44-c (redirects to bugprone-bad-signal-to-kill-thread) 
clang-analyzer-core.CallAndMessage (redirects to 

[PATCH] D70052: [clang-tidy] Add misc-mutating-copy check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze marked 5 inline comments as done.
gbencze added a comment.

Mark comments as Done.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70052/new/

https://reviews.llvm.org/D70052



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70052: [clang-tidy] Add misc-mutating-copy check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added a comment.

In D70052#1749730 , @JonasToth wrote:

> There is a `ExprMutAnalyzer` that is able to find mutation of expressions in 
> general (even though it is kinda experimental still). Maybe that should be 
> utilized somehow? I think the current implementation does not cover when the 
> address is taken and mutation happens through pointers/references in free 
> standing functions, does it?
>
> On the other hand it makes the check more complicated, slower. Additionally 
> the most cases are catched with this version, i guess.


You're right, the current version does not cover mutations through pointers and 
references. I'm not sure how common these would be, but `ExprMutAnalyzer` seems 
like a great option if we wanted to add support for those cases as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70052/new/

https://reviews.llvm.org/D70052



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70052: [clang-tidy] Add misc-mutating-copy check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze updated this revision to Diff 229925.
gbencze added a comment.

Formatting changes (curly braces, newline at EOF)
Remove incorrect flag from test


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70052/new/

https://reviews.llvm.org/D70052

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp
  clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-oop58-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp
@@ -0,0 +1,149 @@
+// RUN: %check_clang_tidy %s cert-oop58-cpp %t
+
+// Example test cases from CERT rule
+// https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP58-CPP.+Copy+operations+must+not+mutate+the+source+object
+namespace test_mutating_noncompliant_example {
+class A {
+  mutable int m;
+
+public:
+  A() : m(0) {}
+  explicit A(int m) : m(m) {}
+
+  A(const A ) : m(other.m) {
+other.m = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+  }
+
+  A =(const A ) {
+if ( != this) {
+  m = other.m;
+  other.m = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: mutating copied object
+}
+return *this;
+  }
+
+  int get_m() const { return m; }
+};
+} // namespace test_mutating_noncompliant_example
+
+namespace test_mutating_compliant_example {
+class B {
+  int m;
+
+public:
+  B() : m(0) {}
+  explicit B(int m) : m(m) {}
+
+  B(const B ) : m(other.m) {}
+  B(B &) : m(other.m) {
+other.m = 0; //no-warning: mutation allowed in move constructor
+  }
+
+  B =(const B ) {
+if ( != this) {
+  m = other.m;
+}
+return *this;
+  }
+
+  B =(B &) {
+m = other.m;
+other.m = 0; //no-warning: mutation allowed in move assignment operator
+return *this;
+  }
+
+  int get_m() const { return m; }
+};
+} // namespace test_mutating_compliant_example
+
+namespace test_mutating_pointer {
+class C {
+  C *ptr;
+  int value;
+
+  C();
+  C(C ) {
+other = {};
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.ptr = nullptr;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.value = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+
+// no-warning: mutating a pointee is allowed
+other.ptr->value = 0;
+*other.ptr = {};
+  }
+};
+} // namespace test_mutating_pointer
+
+namespace test_mutating_indirect_member {
+struct S {
+  int x;
+};
+
+class D {
+  S s;
+  D(D ) {
+other.s = {};
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.s.x = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+  }
+};
+} // namespace test_mutating_indirect_member
+
+namespace test_mutating_other_object {
+class E {
+  E();
+  E(E ) {
+E tmp;
+// no-warning: mutating an object that is not the source is allowed
+tmp = {};
+  }
+};
+} // namespace test_mutating_other_object
+
+namespace test_mutating_member_function {
+class F {
+  int a;
+
+public:
+  void bad_func() { a = 12; }
+  void fine_func() const;
+  void fine_func_2(int x) { x = 5; }
+  void questionable_func();
+
+  F(F ) : a(other.a) {
+this->bad_func(); // no-warning: mutating this is allowed
+
+other.bad_func();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: call mutates copied object
+
+other.fine_func();
+other.fine_func_2(42);
+other.questionable_func();
+  }
+};
+} // namespace test_mutating_member_function
+
+namespace test_mutating_function_on_nested_object {
+struct S {
+  int x;
+  void mutate(int y) {
+x = y;
+  }
+};
+
+class G {
+  S s;
+  G(G ) {
+s.mutate(0); // no-warning: mutating this is allowed
+
+other.s.mutate(0);
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: call mutates copied object
+  }
+};
+} // namespace test_mutating_function_on_nested_object
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -105,6 +105,7 @@
cert-msc51-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) 
+   cert-oop58-cpp
clang-analyzer-core.CallAndMessage (redirects to https://clang.llvm.org/docs/analyzer/checkers) 
clang-analyzer-core.DivideZero (redirects to https://clang.llvm.org/docs/analyzer/checkers) 
clang-analyzer-core.DynamicTypePropagation
Index: 

[PATCH] D70052: [clang-tidy] Add misc-mutating-copy check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze updated this revision to Diff 229720.
gbencze added a comment.

Moved check to CERT module. 
Added warnings for calling mutating member functions.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70052/new/

https://reviews.llvm.org/D70052

Files:
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/clang-tidy/cert/CMakeLists.txt
  clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.cpp
  clang-tools-extra/clang-tidy/cert/MutatingCopyCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-oop58-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-oop58-cpp.cpp
@@ -0,0 +1,149 @@
+// RUN: %check_clang_tidy %s cert-oop58-cpp %t -std=c++11-or-later
+
+// Example test cases from CERT rule
+// https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP58-CPP.+Copy+operations+must+not+mutate+the+source+object
+namespace test_mutating_noncompliant_example {
+class A {
+  mutable int m;
+
+public:
+  A() : m(0) {}
+  explicit A(int m) : m(m) {}
+
+  A(const A ) : m(other.m) {
+other.m = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+  }
+
+  A =(const A ) {
+if ( != this) {
+  m = other.m;
+  other.m = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: mutating copied object
+}
+return *this;
+  }
+
+  int get_m() const { return m; }
+};
+} // namespace test_mutating_noncompliant_example
+
+namespace test_mutating_compliant_example {
+class B {
+  int m;
+
+public:
+  B() : m(0) {}
+  explicit B(int m) : m(m) {}
+
+  B(const B ) : m(other.m) {}
+  B(B &) : m(other.m) {
+other.m = 0; //no-warning: mutation allowed in move constructor
+  }
+
+  B =(const B ) {
+if ( != this) {
+  m = other.m;
+}
+return *this;
+  }
+
+  B =(B &) {
+m = other.m;
+other.m = 0; //no-warning: mutation allowed in move assignment operator
+return *this;
+  }
+
+  int get_m() const { return m; }
+};
+} // namespace test_mutating_compliant_example
+
+namespace test_mutating_pointer {
+class C {
+  C *ptr;
+  int value;
+
+  C();
+  C(C ) {
+other = {};
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.ptr = nullptr;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.value = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+
+// no-warning: mutating a pointee is allowed
+other.ptr->value = 0;
+*other.ptr = {};
+  }
+};
+} // namespace test_mutating_pointer
+
+namespace test_mutating_indirect_member {
+struct S {
+  int x;
+};
+
+class D {
+  S s;
+  D(D ) {
+other.s = {};
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.s.x = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+  }
+};
+} // namespace test_mutating_indirect_member
+
+namespace test_mutating_other_object {
+class E {
+  E();
+  E(E ) {
+E tmp;
+// no-warning: mutating an object that is not the source is allowed
+tmp = {};
+  }
+};
+} // namespace test_mutating_other_object
+
+namespace test_mutating_member_function {
+class F {
+  int a;
+
+public:
+  void bad_func() { a = 12; }
+  void fine_func() const;
+  void fine_func_2(int x) { x = 5; }
+  void questionable_func();
+
+  F(F ) : a(other.a) {
+this->bad_func(); // no-warning: mutating this is allowed
+
+other.bad_func();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: call mutates copied object
+
+other.fine_func();
+other.fine_func_2(42);
+other.questionable_func();
+  }
+};
+} // namespace test_mutating_member_function
+
+namespace test_mutating_function_on_nested_object {
+struct S {
+  int x;
+  void mutate(int y) {
+x = y;
+  }
+};
+
+class G {
+  S s;
+  G(G ) {
+s.mutate(0); // no-warning: mutating this is allowed
+
+other.s.mutate(0);
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: call mutates copied object
+  }
+};
+} // namespace test_mutating_function_on_nested_object
\ No newline at end of file
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -105,6 +105,7 @@
cert-msc51-cpp
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) 
+   cert-oop58-cpp
clang-analyzer-core.CallAndMessage (redirects to https://clang.llvm.org/docs/analyzer/checkers) 
clang-analyzer-core.DivideZero (redirects to https://clang.llvm.org/docs/analyzer/checkers) 
clang-analyzer-core.DynamicTypePropagation
Index: 

[PATCH] D70052: [clang-tidy] Add misc-mutating-copy check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze updated this revision to Diff 228758.
gbencze added a comment.

Update documentation and release notes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70052/new/

https://reviews.llvm.org/D70052

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/MutatingCopyCheck.cpp
  clang-tools-extra/clang-tidy/misc/MutatingCopyCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-mutating-copy.rst
  clang-tools-extra/test/clang-tidy/misc-mutating-copy.cpp

Index: clang-tools-extra/test/clang-tidy/misc-mutating-copy.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/misc-mutating-copy.cpp
@@ -0,0 +1,107 @@
+// RUN: %check_clang_tidy %s misc-mutating-copy %t -std=c++11-or-later
+
+// Example test cases from CERT rule
+// https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP58-CPP.+Copy+operations+must+not+mutate+the+source+object
+namespace test_mutating_noncompliant_example {
+class A {
+  mutable int m;
+
+public:
+  A() : m(0) {}
+  explicit A(int m) : m(m) {}
+
+  A(const A ) : m(other.m) {
+other.m = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+  }
+
+  A =(const A ) {
+if ( != this) {
+  m = other.m;
+  other.m = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: mutating copied object
+}
+return *this;
+  }
+
+  int get_m() const { return m; }
+};
+} // namespace test_mutating_noncompliant_example
+
+namespace test_mutating_compliant_example {
+class B {
+  int m;
+
+public:
+  B() : m(0) {}
+  explicit B(int m) : m(m) {}
+
+  B(const B ) : m(other.m) {}
+  B(B &) : m(other.m) {
+other.m = 0; //no-warning: mutation allowed in move constructor
+  }
+
+  B =(const B ) {
+if ( != this) {
+  m = other.m;
+}
+return *this;
+  }
+
+  B =(B &) {
+m = other.m;
+other.m = 0; //no-warning: mutation allowed in move assignment operator
+return *this;
+  }
+
+  int get_m() const { return m; }
+};
+} // namespace test_mutating_compliant_example
+
+namespace test_mutating_pointer {
+class C {
+  C *ptr;
+  int value;
+
+  C();
+  C(C ) {
+other = {};
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.ptr = nullptr;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.value = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+
+// no-warning: mutating a pointee is allowed
+other.ptr->value = 0;
+*other.ptr = {};
+  }
+};
+} // namespace test_mutating_pointer
+
+namespace test_mutating_indirect_member {
+struct S {
+  int x;
+};
+
+class D {
+  S s;
+  D(D ) {
+other.s = {};
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.s.x = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+  }
+};
+} // namespace test_mutating_indirect_member
+
+namespace test_mutating_other_object {
+class E {
+  E();
+  E(E ) {
+E tmp;
+// no-warning: mutating an object that is not the source is allowed
+tmp = {};
+  }
+};
+} // namespace test_mutating_other_object
Index: clang-tools-extra/docs/clang-tidy/checks/misc-mutating-copy.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc-mutating-copy.rst
@@ -0,0 +1,11 @@
+.. title:: clang-tidy - misc-mutating-copy
+
+misc-mutating-copy
+==
+
+Finds assignments to the copied object and its direct or indirect members
+in copy constructors and copy assignment operators.
+
+This check corresponds to the CERT C Coding Standard rule
+`OOP58-CPP. Copy operations must not mutate the source object
+`_.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -283,6 +283,7 @@
llvm-twine-local
misc-definitions-in-headers
misc-misplaced-const
+   misc-mutating-copy
misc-new-delete-overloads
misc-non-copyable-objects
misc-non-private-member-variables-in-classes
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -120,6 +120,12 @@
   Finds historical use of ``unsigned`` to hold vregs and physregs and rewrites
   them to use ``Register``
 
+- New :doc:`misc-mutating-copy
+  ` check.
+
+  Finds assignments to the copied object and its direct or indirect members
+  in copy constructors and copy assignment operators.
+
 

[PATCH] D70052: [clang-tidy] Add misc-mutating-copy check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added a comment.

In D70052#1740235 , @Eugene.Zelenko 
wrote:

> If this is CERT rule, why check is not placed in relevant module?


To be honest I was hoping for some feedback on this as I wasn't sure what the 
best place for this check would be. Quite a few CERT rules seem to be 
implemented in other modules and have cert aliases. 
Do you think this check should be moved there or should I just add an alias?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70052/new/

https://reviews.llvm.org/D70052



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70052: [clang-tidy] Add misc-mutating-copy check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added a comment.

In D70052#1740142 , @mgehre wrote:

> Did you consider to warn on copy constructors/assignment operators take a 
> their arguments by non-`const` reference, and suggest the user to turn them 
> into const references? This seems like a more useful (and easier) check to me.
>  The link above contains `Ideally, the copy operator should have an idiomatic 
> signature. For copy constructors, that is T(const T&); and for copy 
> assignment operators, that is T& operator=(const T&);.`.
>
> The only remaining case are then `mutable` members which are quite rare.


I haven't really considered it as the non-compliant example has the idiomatic 
signature, but it probably would be a good addition to the check. 
misc-unconventional-assign-operator already seems to do this for the assignment 
operator.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70052/new/

https://reviews.llvm.org/D70052



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70052: [clang-tidy] Add misc-mutating-copy check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze added a comment.

In D70052#1740075 , @mgehre wrote:

> Have you run your check over the LLVM/clang source base and seen true 
> positives/false positives?


I tested it on the Xerces and Bitcoin codebases and did not get any warnings.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70052/new/

https://reviews.llvm.org/D70052



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70052: [clang-tidy] Add misc-mutating-copy check

[Gabor Bencze via Phabricator via cfe-commits]

gbencze created this revision.
gbencze added reviewers: aaron.ballman, alexfh, JonasToth, Charusso.
gbencze added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, mgehre, xazax.hun, mgorny.
Herald added a project: clang.

The check warns when (a member of) the copied object is assigned to in a 
copy constructor or copy assignment operator. Based on 
https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP58-CPP.+Copy+operations+must+not+mutate+the+source+object


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D70052

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/MutatingCopyCheck.cpp
  clang-tools-extra/clang-tidy/misc/MutatingCopyCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-mutating-copy.rst
  clang-tools-extra/test/clang-tidy/misc-mutating-copy.cpp

Index: clang-tools-extra/test/clang-tidy/misc-mutating-copy.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/misc-mutating-copy.cpp
@@ -0,0 +1,107 @@
+// RUN: %check_clang_tidy %s misc-mutating-copy %t -std=c++11-or-later
+
+// Example test cases from CERT rule
+// https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP58-CPP.+Copy+operations+must+not+mutate+the+source+object
+namespace test_mutating_noncompliant_example {
+class A {
+  mutable int m;
+
+public:
+  A() : m(0) {}
+  explicit A(int m) : m(m) {}
+
+  A(const A ) : m(other.m) {
+other.m = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+  }
+
+  A =(const A ) {
+if ( != this) {
+  m = other.m;
+  other.m = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: mutating copied object
+}
+return *this;
+  }
+
+  int get_m() const { return m; }
+};
+} // namespace test_mutating_noncompliant_example
+
+namespace test_mutating_compliant_example {
+class B {
+  int m;
+
+public:
+  B() : m(0) {}
+  explicit B(int m) : m(m) {}
+
+  B(const B ) : m(other.m) {}
+  B(B &) : m(other.m) {
+other.m = 0; //no-warning: mutation allowed in move constructor
+  }
+
+  B =(const B ) {
+if ( != this) {
+  m = other.m;
+}
+return *this;
+  }
+
+  B =(B &) {
+m = other.m;
+other.m = 0; //no-warning: mutation allowed in move assignment operator
+return *this;
+  }
+
+  int get_m() const { return m; }
+};
+} // namespace test_mutating_compliant_example
+
+namespace test_mutating_pointer {
+class C {
+  C *ptr;
+  int value;
+
+  C();
+  C(C ) {
+other = {};
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.ptr = nullptr;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.value = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+
+// no-warning: mutating a pointee is allowed
+other.ptr->value = 0;
+*other.ptr = {};
+  }
+};
+} // namespace test_mutating_pointer
+
+namespace test_mutating_indirect_member {
+struct S {
+  int x;
+};
+
+class D {
+  S s;
+  D(D ) {
+other.s = {};
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+other.s.x = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: mutating copied object
+  }
+};
+} // namespace test_mutating_indirect_member
+
+namespace test_mutating_other_object {
+class E {
+  E();
+  E(E ) {
+E tmp;
+// no-warning: mutating an object that is not the source is allowed
+tmp = {};
+  }
+};
+} // namespace test_mutating_other_object
Index: clang-tools-extra/docs/clang-tidy/checks/misc-mutating-copy.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc-mutating-copy.rst
@@ -0,0 +1,11 @@
+.. title:: clang-tidy - misc-mutating-copy
+
+misc-mutating-copy
+==
+
+Finds assignments to and to direct or indirect members of the copied object 
+in copy constructors and copy assignment operators.
+
+This check corresponds to the CERT C Coding Standard rule
+`OOP58-CPP. Copy operations must not mutate the source object
+`_.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -283,6 +283,7 @@
llvm-twine-local
misc-definitions-in-headers
misc-misplaced-const
+   misc-mutating-copy
misc-new-delete-overloads
misc-non-copyable-objects
misc-non-private-member-variables-in-classes
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++