This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add a checker for zero-length memset.
ClosedPublic

Authored by bkramer on Jul 16 2014, 6:29 AM.

Details

Summary

If there's memset(x, y, 0) in the code it's most likely a mistake. The
checker suggests a fix-it to swap 'y' and '0'.

I think this has the potential to be promoted into a general clang warning
after some testing in clang-tidy.

Diff Detail

Repository
rL LLVM

Event Timeline

bkramer updated this revision to Diff 11502.Jul 16 2014, 6:29 AM
bkramer retitled this revision from to [clang-tidy] Add a checker for zero-length memset..
bkramer updated this object.
bkramer added reviewers: alexfh, djasper.
bkramer added a subscriber: Unknown Object (MLST).
djasper added inline comments.Jul 16 2014, 6:39 AM
clang-tidy/google/MemsetZeroLengthCheck.cpp
32–33 ↗(On Diff #11502)

Intuitively, I would have written something like:

unless(hasArgument(1, expr(anyOf(integerLiteral(), characterLiteral())))

Also, I wonder whether we should do this in the callback or inside the callback. In the callback, we could check whether the argument trivially computes to 0 and thereby not only find literal zeros. I am thinking about cases like:

int default_value = 0;
memset(a, 10, default_value);

This is merely a suggestion, though, not sure whether it would lead to false positives.

65 ↗(On Diff #11502)

I generally dislike declaration statements defining more than one variable. Here, at lest make it consisten with RHSString and LHSString.

68 ↗(On Diff #11502)

Early exit?

if (LHSString.empty() || RHSString.empty())
  return;
clang-tidy/google/MemsetZeroLengthCheck.h
21–22 ↗(On Diff #11502)

The sentence doesn't really parse. How about:

This is most likely unintended and the length and value arguments are swapped.

alexfh edited edge metadata.Jul 16 2014, 6:44 AM

Other standard functions may suffer from the same problem, e.g. memchr. I think, it makes sense to handle all of them in the same check.

clang-tidy/google/MemsetZeroLengthCheck.cpp
28 ↗(On Diff #11502)

Ignore literals in the middle argument.

Why?

bkramer updated this revision to Diff 11506.Jul 16 2014, 7:14 AM
bkramer edited edge metadata.
  • Make the checker more aggressive by also looking at values that always evaluate to zero.
  • Remove the restriction on literals in the middle argument. cpplint did this to reduce false positives from testing code, I don't buy that excuse.
  • Cosmetic cleanup.
djasper accepted this revision.Jul 16 2014, 7:37 AM
djasper edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Jul 16 2014, 7:37 AM
bkramer closed this revision.Jul 16 2014, 7:39 AM
bkramer updated this revision to Diff 11514.

Closed by commit rL213155 (authored by d0k).

alexfh added inline comments.Jul 16 2014, 7:40 AM
clang-tidy/google/MemsetZeroLengthCheck.cpp
60 ↗(On Diff #11506)

Maybe we should emit a warning, that this call is a no-op?

64 ↗(On Diff #11506)

I'd say Value1 < 0 is also not when we want to swap the arguments.

bkramer added inline comments.Jul 16 2014, 7:52 AM
clang-tidy/google/MemsetZeroLengthCheck.cpp
60 ↗(On Diff #11506)

Fixed.

64 ↗(On Diff #11506)

That seems like an incredibly weak heuristic. Zero-sized memsets should not occur in real code :(