[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 <https://reviews.llvm.org/D89651#2342367>, @gbencze wrote:

> In D89651#2338266 <https://reviews.llvm.org/D89651#2338266>, @njames93 wrote:
>
>> Should point out there is already a check for cert-oop57-cpp, added in D72488 <https://reviews.llvm.org/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(&some_float, &some_other_float, sizeof(float));
memcmp(&some_struct, &some_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(&a, &b, sizeof(int)); // no-warning: not comparing entire object
+  memcmp(&a, &b, 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(&one, &two, 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(&one, &two, 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 <typename Ty>
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89651