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

Jonas Toth via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 30 05:54:11 PST 2019


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 &Ctx, 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 &Ctx) {
+  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 &Field : 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<CallExpr>(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<int64_t> 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<int64_t> 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 *ArgExpr = CE->getArg(i);
+    const QualType ArgType = ArgExpr->IgnoreImplicit()->getType();
+    const Type *const PointeeType = ArgType->getPointeeOrArrayElementType();
----------------
`const`, next line the `* const` is uncommon in llvm-code. I think you should remove the pointer-consting. The other pointers are not `const` either.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:136
+    const Type *const PointeeType = ArgType->getPointeeOrArrayElementType();
+    assert(PointeeType != nullptr);
+
----------------
error message.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousMemoryComparisonCheck.cpp:152
+        if (ComparedBits && hasPadding(Ctx, RD, *ComparedBits)) {
+          diag(CE->getBeginLoc(), "comparing padding bytes");
+          break;
----------------
i think this diagnostic could be a bit more expressive, similar to the one above.
Unexperienced programmer might not understand the issue or even know what padding bytes are.


================
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
+
----------------
We should explicitly test, that the check runs under C-code, either with two run-lines or with separate test files (my preference).


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


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

https://reviews.llvm.org/D71973





More information about the cfe-commits mailing list