[clang-tidy] Extend cert-oop57-cpp to check non-zero memset values

[gamesh411]

Summary

Clang Tidy check cert-oop57-cpp now checks for arbitrary-valued integer
literals in memset expressions containing non-trivially
default-constructible instances. Previously it only checked 0 values.

Note that the first non-compliant example of
https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP57-CPP.+Prefer+special+member+functions+and+overloaded+operators+to+C+Standard+Library+functions requires this.

A comparative analysis of OS projects is currently running to see the impact of this change.

[steakhal]

Looks reasonable to me.

[gamesh411]

{F23163926}
There is no change in the results as far as these OS projects are concerned.

[njames93]

Please upload diff with full context.
Can you add a note about this in the release notes.

As a side point I'm not sure this change really follows what the rule is trying to enforce. The rule is about not using std::memset to reinitialise objects that aren't trivial. Having said that limiting it to only 0 does seem a little restrictive,

[gamesh411]

Added a release note
Also generated the full context (arcanist could validate the site certificate, that's why I had to resort to manual diff creation. Was there a certificate change on the reviews.llmv.org site maybe?)

[steakhal]

In D126186#3530967, @gamesh411 wrote:

Was there a certificate change on the reviews.llmv.org site maybe?)

Yes, AFAIK.

Please redo the measurement, it doesn't look right.

[njames93]

This diff looks like it's rooted on the clang-tools-extra directory which is why the pre-merge bot is failing to build.

[aaron.ballman]

In D126186#3530938, @njames93 wrote:

As a side point I'm not sure this change really follows what the rule is trying to enforce. The rule is about not using std::memset to reinitialise objects that aren't trivial. Having said that limiting it to only 0 does seem a little restrictive,

I think this change makes sense for the rule as it's written, per "Do not use std::memset() to initialize an object of nontrivial class type as it may not properly initialize the value representation of the object." The noncompliant example was using zero initialization, but there's nothing inherently special about 0 except for how commonly used it is as a default value.

[gamesh411]

Add full diff with arcanist

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

208

Please sort entries in section alphabetically.

209

Please fix double space and enclose memset into double back-ticks.

[gamesh411]

fix Release Notes

[gamesh411]

Remove literal checking from the matcher for memset as well

There is no change in the result set on open source projects even without
restricting the matches to literals.

IMO this is more in line with the rule as it's written as @aaron.ballman mentioned.

Updated the ReleaseNotes to reflect this.

[aaron.ballman]

LGTM!

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

160

Please use double back-ticks for language constructs.

[gamesh411]

Thanks for the quick review!
Fixed the double backtick in the release notes as well.

clang-tools-extra/clang-tidy/cert/NonTrivialTypesLibcMemoryCallsCheck.cpp

return expr(unaryOperator(
         hasOperatorName("&"),
         hasUnaryOperand(declRefExpr(
             allOf(hasType(cxxRecordDecl(Constraint)),
                   hasType(Bind ? qualType().bind("Record") : qualType()))))));
   };
   auto IsRecordSizeOf =
       expr(sizeOfExpr(hasArgumentOfType(equalsBoundNode("Record"))));
   auto ArgChecker = [&](Matcher<CXXRecordDecl> RecordConstraint,
-                        BindableMatcher<Stmt> SecondArg) {
+                        BindableMatcher<Stmt> SecondArg = expr()) {
     return allOf(argumentCountIs(3),
                  hasArgument(0, IsStructPointer(RecordConstraint, true)),
                  hasArgument(1, SecondArg), hasArgument(2, IsRecordSizeOf));
   };
 
   Finder->addMatcher(
       callExpr(callee(namedDecl(hasAnyName(
                    utils::options::parseListPair(BuiltinMemSet, MemSetNames)))),
-               ArgChecker(unless(isTriviallyDefaultConstructible()),
-                          expr(integerLiteral(equals(0)))))
+               ArgChecker(unless(isTriviallyDefaultConstructible())))
           .bind("lazyConstruct"),
       this);
   Finder->addMatcher(
       callExpr(callee(namedDecl(hasAnyName(
                    utils::options::parseListPair(BuiltinMemCpy, MemCpyNames)))),
                ArgChecker(unless(isTriviallyCopyable()), IsStructPointer()))
           .bind("lazyCopy"),
       this);

clang-tools-extra/docs/ReleaseNotes.rst

 
 - Fixed some false positives in :doc:`bugprone-infinite-loop
   <clang-tidy/checks/bugprone-infinite-loop>` involving dependent expressions.
 
 - Fixed a crash in :doc:`bugprone-sizeof-expression
   <clang-tidy/checks/bugprone-sizeof-expression>` when `sizeof(...)` is
   compared against a `__int128_t`.
 
+- Made :doc:`cert-oop57-cpp <clang-tidy/checks/cert-oop57-cpp>` more sensitive
+  by checking for an arbitrary expression in the second argument of `memset`.
+
 - Improved :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines-prefer-member-initializer>` check.
 
   Fixed an issue when there was already an initializer in the constructor and
   the check would try to create another initializer for the same member.
 
 - Fixed a crash in :doc:`llvmlibc-callee-namespace
   <clang-tidy/checks/llvmlibc-callee-namespace>` when executing for C++ code

 - Fixed a false positive in :doc:`readability-non-const-parameter
   <clang-tidy/checks/readability-non-const-parameter>` when the parameter is
   referenced by an lvalue.
 
 - Expanded :doc:`readability-simplify-boolean-expr
   <clang-tidy/checks/readability-simplify-boolean-expr>` to simplify expressions
   using DeMorgan's Theorem.
 
 - Fixed a crash in :doc:`performance-unnecessary-value-param
   <clang-tidy/checks/readability-suspicious-call-argument>` when the specialization
   template has an unnecessary value paramter. Removed the fix for a template.
 
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 

clang-tools-extra/test/clang-tidy/checkers/cert-oop57-cpp.cpp

void baz(const NonTrivial &Other) {
   NonTrivial Data;
   mymemset(&Data, 0, sizeof(Data));
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling 'mymemset' on a non-trivially default constructible class is undefined
   mymemcpy(&Data, &Other, sizeof(Data));
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling 'mymemcpy' on a non-trivially copyable class is undefined
   mymemcmp(&Data, &Other, sizeof(Data));
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: consider using comparison operators instead of calling 'mymemcmp'
 }
+
+void nonNullSetValue() {
+  NonTrivial Data;
+  // Check non-null-valued second argument.
+  std::memset(&Data, 1, sizeof(Data));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling 'memset' on a non-trivially default constructible class is undefined
+}
+
+void nonLiteralSetValue(char C) {
+  NonTrivial Data;
+  // Check non-literal second argument.
+  std::memset(&Data, C, sizeof(Data));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling 'memset' on a non-trivially default constructible class is undefined
+}