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

[Balázs Benics via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3373e845398b: [clang-tidy] Add 
bugprone-suspicious-memory-comparison check (authored by gbencze, committed by 
steakhal).

Changed prior to commit:
  https://reviews.llvm.org/D89651?vs=306679=368824#toc

Repository:
  rG LLVM Github Monorepo

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,233 @@
+// 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 

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.
Herald added a subscriber: nullptr.cpp.

I really love this.
I'm gonna have a look at the blocking patch.




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
+

gbencze wrote:
> 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. 
Why don't we pick `i386-unknown-linux` instead?
That would limit the scope of this test to Ititanium ABI.


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 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] D89651: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, thank you for the new check!


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 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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:76
+ "values|the members of the object}1 "
+ "manually")
+<< PointeeQualifiedType << (PointeeType->isRecordType() ? 1 : 0);

It looks like this part of the string literal can be moved up a line.



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

gbencze wrote:
> 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?
I don't think it's a serious issue except for possibly the `_Atomic` case in C 
code. It may be worth testing that scenario explicitly and putting a FIXME 
comment on the test case.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst:13
+See also:
+`OOP57-CPP. Prefer special member functions and overloaded operators to C 
Standard Library functions
+`_

May also want to mention EXP62-CPP which is similarly related to OOP57-CPP -- 
but the docs should be clear that this isn't a check for those rules, just that 
it has some partial overlap.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst:16
+
+**Case 2: Type with no unique object representations**
+

Type -> Types
representations -> representation



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memory-comparison.rst:18
+
+Objects with the same value may not have the same object representaions.
+This may be caused by padding or floating-point types.

representaions -> representation



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

gbencze wrote:
> 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. 
Thanks for the new test cases, and I think that emergent behavior is reasonable.


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 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] D89651: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D89651#2342367 , @gbencze wrote:

> 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.

The thrust of the idea behind OOP57-CPP is that you shouldn't use C functions 
to do work that would normally be done via a C++ overloaded operator. e.g., 
don't call `memcmp()` on two structs, define the appropriate set of relational 
and equality operators for the type instead. It's required that you do this for 
non-trivial types, but it's aspirational to do it for all types. I don't think 
the proposed check should relate to OOP57-CPP all that much -- I see it more 
closely relating to EXP62-CPP instead: 
https://wiki.sei.cmu.edu/confluence/display/cplusplus/EXP62-CPP.+Do+not+access+the+bits+of+an+object+representation+that+are+not+part+of+the+object%27s+value+representation
 except that this check is currently only about comparisons and not accessing, 
so it's not quite perfect coverage.




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);

The lint warning here is actually correct, which is a lovely change of pace.



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

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.



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")

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.



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
+

Wouldn't picking a 32-bit target suffice instead of `-m32`? e.g., `-target 
i386-unknown-unknown`



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

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?
```



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-suspicious-memory-comparison.c:198
+}
+
+struct PaddingAfterUnion {

How about a test where everything lines up perfectly for bit-fields?
```
struct Whatever {
  int i : 10;
  int j : 10;
  int k : 10;
  int l : 2;
};
_Static_assert(sizeof(struct Whatever) == sizeof(int));
```



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

Another case we should be careful of is template instantiations:
```
template 
void func(const Ty *o1, 

[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] D89651: [clang-tidy] Add bugprone-suspicious-memory-comparison check

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

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?


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] 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(, ,