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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D71973#1817941 , @gbencze wrote:

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


This is a neat approach, but I think I still prefer asking the RecordLayout for 
this information if possible.




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() &&

gbencze wrote:
> 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? 
I do think this should be provided by `RecordLayout` (and yes, please as a 
separate patch -- feel free to add myself and Richard Smith as reviewers for 
it). Intuitively, I would expect to be able to query for whether a record has 
any padding within it whatsoever. For your needs, I would imagine an interface 
like `bool hasPaddingWithin(CharUnits InitialOffset, CharUnits FinalOffset)` 
would also help -- though I suspect we may not want to use `CharUnits` because 
of bit-fields that may have padding due to 0-sized bit-fields.


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.

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:30
+ uint64_t ComparedBits, uint64_t ) {
+  const auto IsNotEmptyBase = [](const CXXBaseSpecifier ) {
+return !Base.getType()->getAsCXXRecordDecl()->isEmpty();

Drop top-level `const` qualifiers on locals (it's not a style we typically 
use), elsewhere as well.



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() &&

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.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:130
+const Type *PointeeType = ArgType->getPointeeOrArrayElementType();
+assert(PointeeType != nullptr && "PointeeType should always be 
available.");
+

Can you add a test case like this:
```
struct S {
  operator void *() const;
};

S s;
memcmp(s, s, sizeof(s));
```
to ensure that this assertion doesn't trigger?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:133-134
+if (PointeeType->isRecordType()) {
+  const RecordDecl *RD = PointeeType->getAsRecordDecl()->getDefinition();
+  if (RD != nullptr) {
+if (const auto *CXXDecl = dyn_cast(RD)) {

Sink the declaration into the if conditional to limit its scope.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:140
+ "type %0; consider using a comparison operator instead")
+<< PointeeType->getAsTagDecl()->getQualifiedNameAsString();
+break;

You can pass in `PointeeType->getAsTagDecl()` -- the diagnostic printer will 
automatically get the correct name from any `NamedDecl`.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:148
+  "consider comparing the fields manually")
+  << PointeeType->getAsTagDecl()->getQualifiedNameAsString();
+  break;

Same here as above.


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.

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

LG from my side, besides the small nits.
Please let @aaron.ballman have a look as well.

thanks for the patch :)




Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:45
+
+  // check if comparing padding in base
+  if (hasPadding(Ctx, BaseRD, ComparedBits))

Nit: Please make this comment a full sentence with proper punctuation.



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
+`_.

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



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"

i believe the classification of the checks was changed today. did you rebase to 
master? hopefully it still applies clean.


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

[Jonas Toth via Phabricator via cfe-commits]

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

gbencze wrote:
> 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? 
Rather splitting. Duplication might be painfull in the future.



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

gbencze wrote:
> 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;
> };
> ```
Yes. Because it is only tested for the current archtiecture the buildbots will 
probably fail on it, because they test many architectures.

I think it is necessary to specify the arch in the RUN-line (`%t -- -- -target 
x86_64-unknown-unknown`) at the end of it.

And yes: Another test-file would be great to demonstrate that it works as 
expected.


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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:19
+
+static constexpr llvm::StringLiteral CallExprName = "call";
+

I think you can remove the constant and hardcode it in the matcher. that is 
common practice and because its used twice not a big issue (and shorter).



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:21
+
+static bool hasPadding(const clang::ASTContext , const RecordDecl *RD,
+   uint64_t ComparedBits);

You are in `clang` namespace, so you can ellide the `clang::` here and in the 
following functions/decls.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:27
+ const clang::ASTContext ) {
+  if (FD.isBitField())
+return FD.getBitWidthValue(Ctx);

Maybe `return FD.isBitField() ? FD.getBitWidthValue(Ctx) : 
Ctx.getTypeSize(FieldType);`? I find it cleaner, but its preference i guess.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:46
+  NonEmptyBaseIt->getType()->getAsCXXRecordDecl();
+  const uint64_t SizeOfBase = Ctx.getTypeSize(BaseRD->getTypeForDecl());
+  TotalSize += SizeOfBase;

value-types are not defined as `const` per coding guideline.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:62
+  for (const auto  : RD->fields()) {
+const uint64_t FieldOffset = Ctx.getFieldOffset(Field);
+assert(FieldOffset >= TotalSize);

plz no `const`



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:63
+const uint64_t FieldOffset = Ctx.getFieldOffset(Field);
+assert(FieldOffset >= TotalSize);
+

a short descriptive error message for this assertion would be great.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:66
+if (FieldOffset > TotalSize && TotalSize < ComparedBits)
+  // Padding before this field
+  return true;

Please make this comment a full sentence with punctuation. I think it should be 
above the `if`.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:72
+
+const uint64_t SizeOfField = getFieldSize(*Field, Field->getType(), Ctx);
+TotalSize += SizeOfField;

`const`



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:76
+if (Field->getType()->isRecordType()) {
+  // Check if comparing padding in nested record
+  if (hasPadding(Ctx, Field->getType()->getAsRecordDecl()->getDefinition(),

Please make this comment a full sentence with punctuation.

Both ifs can be merged to one with an `&&`?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:88
+   uint64_t ComparedBits) {
+  assert(RD && RD->isCompleteDefinition());
+  if (RD->isUnion())

Please provide an error message



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:98
+
+  // check for trailing padding
+  return TotalSize < Ctx.getTypeSize(RD->getTypeForDecl()) &&

Full sentence for comment.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:109
+CharUnits::fromQuantity(Result.Val.getInt().getExtValue()));
+  else
+return None;

No `else` after return. You can simply `return None;`



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:125
+  const auto *CE = Result.Nodes.getNodeAs(CallExprName);
+  assert(CE != nullptr);
+

Always true, otherwise matcher don't work / didn't fire. I think this assert is 
not required.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:128
+  const Expr *SizeExpr = CE->getArg(2);
+  assert(SizeExpr != nullptr);
+  const llvm::Optional ComparedBits =

please provide a short error message why this should always be true (i guess 
because memcmp is standardized and stuff, but when reading this code alone it 
might not be super obvious).



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:129
+  assert(SizeExpr != nullptr);
+  const llvm::Optional ComparedBits =
+  tryEvaluateSizeExpr(SizeExpr, Ctx);

value-type that is `const`, please remove the `const`.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:134
+const Expr 

[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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



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

gbencze wrote:
> 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? 
In 9.0 Release Notes aliases were placed after new checks.


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

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:38
+
+  if (const CXXRecordDecl *CXXRD = dyn_cast(RD)) {
+const auto NonEmptyBaseIt = llvm::find_if(CXXRD->bases(), IsNotEmptyBase);

You could use const auto * because type is specified in same statement.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:44
+
+  const auto *BaseRD = NonEmptyBaseIt->getType()->getAsCXXRecordDecl();
+  const uint64_t SizeOfBase = Ctx.getTypeSize(BaseRD->getTypeForDecl());

Please don't use auto when type is not specified in same statement or iterator.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:128
+  assert(SizeExpr != nullptr);
+  const auto ComparedBits = tryEvaluateSizeExpr(SizeExpr, Ctx);
+

Please don't use auto when type is not specified in same statement or iterator.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:139
+  if (RD != nullptr) {
+if (const CXXRecordDecl *CXXDecl = dyn_cast(RD)) {
+  if (!CXXDecl->isStandardLayout()) {

You could use const auto * because type is specified in same statement.



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

Please move into aliases section (in alphabetical order)


Repository:
  rG LLVM Github Monorepo

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 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];
+