Page MenuHomePhabricator

[Clang-tidy] check for malloc, realloc and free calls
ClosedPublic

Authored by JonasToth on Oct 31 2016, 2:45 PM.

Details

Summary

This checker flags the use of C-style memory management functionality and notes about modern alternatives.
In an earlier revision it tried to autofix some kind of patterns, but that was a bad idea. Since memory management can be so widespread in a program, manual updating is most likely necessary.
Maybe for special cases, there could be later additions to this basic checker.

This is the first checker I wrote and I never did something with clang (only compiling programs). So whenever I missed conventions or did plain retarded stuff, feel free to point it out! I am willing to fix them and write a better checker.

I hope the patch does work, I never did this either. On a testapply in my repository it did, but I am pretty unconfident in my patching skills :)

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
aaron.ballman added inline comments.Nov 10 2016, 7:54 AM
clang-tidy/modernize/NoMallocCheck.cpp
22 ↗(On Diff #77379)

Here, and elsewhere, all comments should be grammatically correct (capitalization and punctuation). Look for missing full stops at the end of a sentence, and make sure the sentence starts with a capital letter.

27 ↗(On Diff #77379)

You should not register the AST matchers unless in C++ mode (I don't think the check makes sense for C).

JonasToth updated this revision to Diff 78624.Nov 19 2016, 1:52 AM
JonasToth edited edge metadata.

Iam sorry if this is not so great, but i think i wanted to much with the automatic fixing.
As pointed out in the Code Review, there a issues with the automatic fixing and it most likely wont be of any value for real project. I tested it on a closed source project (70k lines) and it was terrible :D

The checker is now way simpler, just warning about malloc, calloc, realloc and free. Suggesting RAII, Containers or Smartpointers in a note. The diagnostics is now way shorter, and i think easier to work with.
I moved the check into cppcoreguidelines as well. Sry for the wasted time on the first revisions, but i think this one is a better check now.

Special cases like C-strings could be added later, and automatic fixing could be used later. But i think i want a little more experience with the Checker and writing a clang-tidy check first, before i start sth like that.

alexfh requested changes to this revision.Nov 30 2016, 8:19 AM
alexfh edited edge metadata.

Jonas, I suppose, reviewers are waiting until you address all outstanding comments (and mark them "Done") before the next round of review.

clang-tidy/modernize/NoMallocCheck.cpp
145 ↗(On Diff #77379)

"diagnostics are not meant to be grammatically correct" might be too strong ;) They are not supposed to be full sentences (no leading capitalization or trailing punctuation), but otherwise they should be grammatically correct ;)

This revision now requires changes to proceed.Nov 30 2016, 8:19 AM
JonasToth marked an inline comment as done.Nov 30 2016, 8:25 AM
JonasToth marked 20 inline comments as done.Nov 30 2016, 8:29 AM

I did mark all comments as done.
Most issues and codelines dont exist anymore, since the checker is now way simpler (just flagging c memorymanagement functions) and giving a note what could be used otherwise.

JonasToth marked 2 inline comments as done.Nov 30 2016, 8:33 AM
JonasToth updated this object.Nov 30 2016, 8:35 AM
JonasToth edited edge metadata.
alexfh added inline comments.Dec 1 2016, 7:05 AM
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
68–70

A note is used when there's a need to point to a different source location. There's not much sense in just splitting out a part of a warning to a note on the same location.

clang-tidy/modernize/NoMallocCheck.cpp
145 ↗(On Diff #77379)

This hasn't been addressed as far as I can see.

JonasToth added inline comments.Dec 1 2016, 8:09 AM
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
68–70

should i add the suggestion to the warning or remove it completly?

alexfh added inline comments.Dec 1 2016, 9:08 AM
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
68–70

The suggestion is fine, just merge it with the warning. Please also make sure the text is consistent with the other cppcoreguidelines checks (I mean the parts in the square brackets, I'm not sure if other checks follow the same style).

JonasToth updated this revision to Diff 79926.Dec 1 2016, 9:28 AM
JonasToth edited edge metadata.

Update the diagnostics, only one warning is produced containing the suggestion, no notes anymore.

Furthermore there is an update in 2 comments.

JonasToth marked 3 inline comments as done.Dec 1 2016, 9:29 AM

adjusted the diagnostics, i also removed the reference to the specific part of the cppcoreguidelines, making it consistent with other checkers.

JonasToth marked an inline comment as done.Dec 12 2016, 2:52 AM
alexfh requested changes to this revision.Dec 12 2016, 8:58 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
55

The handle.* methods now just add more bulk and bring no benefit. With a single statement in each function they might as well be inlined and the checkmethod would be still quite compact:

CallExpr Call = nullptr;
StringRef Recommendation;
if ((Call = Result.Nodes.getNodeAs<CallExpr>("aquisition")))
  Recommendation = "consider a container or smartpointer";
else if ((Call = ...))
  ...;

assert(Call && "Unhandled binding in the matcher);

diag(Call->getLocStart(),
     "do not allocate memory manually; %0")
  << Recommendation
  << SourceRange(...);
57

"smart pointer" is two words.

58

Please use parentheses instead of curly braces for constructor calls (SourceRange(x, y)), it's more common in LLVM code.

This revision now requires changes to proceed.Dec 12 2016, 8:58 AM
JonasToth marked 3 inline comments as done.Dec 12 2016, 10:08 AM

removed the function calls, a little doc tidy as well

JonasToth updated this revision to Diff 81103.Dec 12 2016, 10:10 AM
JonasToth edited edge metadata.

fixes last review comments from alex

alexfh requested changes to this revision.Dec 12 2016, 11:20 PM
alexfh edited edge metadata.

One important thing is missing. Please run this check on a large enough codebase (LLVM + Clang is a good choice for testing most of kinds of checks usually) and include a summary of results in the patch description. The most important things are: clang-tidy doesn't crash, results (or if there are too many, then a sufficiently large random sample - say, 100) look good on manual inspection.

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
49

nit: a smart pointer (an article is missing).

50

Please remove empty lines before else

This revision now requires changes to proceed.Dec 12 2016, 11:20 PM
JonasToth updated this revision to Diff 81193.Dec 13 2016, 1:23 AM
JonasToth edited edge metadata.
JonasToth marked 2 inline comments as done.

last fixes in the code, from alex

I did run clang-tidy with $(./run-clang-tidy.py -checks=-*,cppcoreguidelines-no-malloc). The resulting output is in the appended file.
Since all warnings came from header files (llvms custom containers?) there are always the same warnings for all source files.
Every function (calloc, malloc, realloc and free) was seen, i hope the output is sufficient.

Whats not nice is, that there is no underlining (in test neither), but i could not find out what is wrong, since i supply a SourceRange in the diag()-call.

malcolm.parsons added inline comments.
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
30

C memory management functions are also present in the std namespace.

docs/clang-tidy/checks/cppcoreguidelines-no-malloc.rst
11

Do you mean #r10-avoid-malloc-and-free?

27

Missing variable.

JonasToth marked an inline comment as done.Dec 13 2016, 2:25 AM
JonasToth added inline comments.
docs/clang-tidy/checks/cppcoreguidelines-no-malloc.rst
11

yes, but i think the wider view is more appropriate. RAII is a general strategy, and not specific to malloc etc.

27

that slipped through. i fix it.

JonasToth marked an inline comment as done.Dec 13 2016, 2:26 AM
JonasToth added inline comments.
clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
30

in the clang codebase, std::malloc got catched as well. see the tidy_output_no_malloc.txt in one of my previous comments.

JonasToth updated this revision to Diff 81204.Dec 13 2016, 2:28 AM
JonasToth edited edge metadata.

fix missing variable in doc

JonasToth marked an inline comment as done.Dec 13 2016, 2:29 AM
docs/clang-tidy/checks/cppcoreguidelines-no-malloc.rst
5

A few = missing.

11

This checker is specific to R.10.
The documentation for all the other cppcoreguidelines checks mention which rule they're enforcing.

JonasToth updated this revision to Diff 81208.Dec 13 2016, 3:02 AM

adjusted doc.

JonasToth marked 4 inline comments as done.Dec 13 2016, 3:02 AM
JonasToth added inline comments.
docs/clang-tidy/checks/cppcoreguidelines-no-malloc.rst
11

alright. i will adjust.

JonasToth marked 2 inline comments as done.Dec 13 2016, 3:03 AM
alexfh accepted this revision.Dec 13 2016, 6:18 AM
alexfh edited edge metadata.

LG, but please wait until Malcolm

Whats not nice is, that there is no underlining (in test neither), but i could not find out what is wrong,
since i supply a SourceRange in the diag()-call.

Please leave the range anyway. I suspect clang-tidy doesn't handle ranges in diagnostics correctly, but it's easier to fix this, if there is an example already.

This revision is now accepted and ready to land.Dec 13 2016, 6:18 AM

Hit "submit" too early.

LG, but please wait until Malcolm is happy with the change as well.

alexfh removed a subscriber: malcolm.parsons.
malcolm.parsons accepted this revision.Dec 13 2016, 6:51 AM
malcolm.parsons edited edge metadata.

LGTM.

JonasToth marked 2 inline comments as done.Dec 13 2016, 7:30 AM

Jonas, do you need someone to commit the patch for you?

tbh. i dont know anything how the clang workflow works :)

do i need to ask someone to commit, or what be my next step?

Jonas, do you need someone to commit the patch for you?

i think so, yes. can i trigger somehow automatically, that this will go into the code base somehow?

alexfh closed this revision.Dec 13 2016, 9:12 AM

Committed in r289546.

rnk added a subscriber: rnk.Dec 13 2016, 12:35 PM

nit: All of the new files had UTF-8 BOMs, which I don't think we usually add.

test/clang-tidy/cppcoreguidelines-no-malloc.cpp
3

This test did not pass on 32-bit Windows. I will change it to:

using size_t = __SIZE_TYPE__;

I tried this check on my company's codebase.
It misses cases where malloc is used through a function pointer.
Should clang-tidy warn about making a function pointer to these functions?

I tried this check on my company's codebase.
It misses cases where malloc is used through a function pointer.
Should clang-tidy warn about making a function pointer to these functions?

Sounds like a good idea to me. I can't really imagine a scenario where you'd want to enable this check and not be warned about it.

alright, i will try to get the functionpointers as well.
what do you think about configuration of the allocating functions? e.g. for aligned memory you must use OS-specific allocation functions.
should the check catch custom allocation functions as well?

is there a way to make a list of allocation, reallocation, deallocation functions that the check shall look for (+aliases to these)?

JonasToth marked an inline comment as done.Dec 14 2016, 2:34 AM

what do you think about configuration of the allocating functions? e.g. for aligned memory you must use OS-specific allocation functions.
should the check catch custom allocation functions as well?

Sounds like a good idea to me.

what do you think about configuration of the allocating functions? e.g. for aligned memory you must use OS-specific allocation functions.
should the check catch custom allocation functions as well?

Sounds like a good idea to me.

I think this check should also warn about strdup(), strndup() and wcsdup().

kimgr added a subscriber: kimgr.Dec 27 2016, 3:42 AM

Minor spelling nits inline

clang-tidy/cppcoreguidelines/NoMallocCheck.cpp
30

Spelling nit: acquisition

But I wonder if there's a simpler word you can use for this, e.g. "alloc"

47

"aquisition" as above