[clang-tidy] introduce legacy resource functions to 'cppcoreguidelines-owning-memory'

Summary:
This patch introduces support for legacy C-style resource functions that must obey
the 'owner<>' semantics.

- added legacy creators like malloc,fopen,...
- added legacy consumers like free,fclose,...

This helps codes that mostly benefit from owner:
Legacy, C-Style code that isn't feasable to port directly to RAII but needs a step in between
to identify actual resource management and just using the resources.

Reviewers: aaron.ballman, alexfh, hokein

Reviewed By: aaron.ballman

Subscribers: nemanjai, JDevlieghere, xazax.hun, kbarton

Differential Revision: https://reviews.llvm.org/D38396

llvm-svn: 316092
This commit is contained in:
Jonas Toth 2017-10-18 16:14:15 +00:00
parent 13ce95b77f
commit c9aea86e6a
5 changed files with 377 additions and 6 deletions

View File

@ -22,6 +22,21 @@ namespace clang {
namespace tidy {
namespace cppcoreguidelines {
// FIXME: Copied from 'NoMallocCheck.cpp'. Has to be refactored into 'util' or
// something like that.
namespace {
Matcher<FunctionDecl> hasAnyListedName(const std::string &FunctionNames) {
const std::vector<std::string> NameList =
utils::options::parseStringList(FunctionNames);
return hasAnyName(std::vector<StringRef>(NameList.begin(), NameList.end()));
}
} // namespace
void OwningMemoryCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "LegacyResourceProducers", LegacyResourceProducers);
Options.store(Opts, "LegacyResourceConsumers", LegacyResourceConsumers);
}
/// Match common cases, where the owner semantic is relevant, like function
/// calls, delete expressions and others.
void OwningMemoryCheck::registerMatchers(MatchFinder *Finder) {
@ -30,10 +45,31 @@ void OwningMemoryCheck::registerMatchers(MatchFinder *Finder) {
const auto OwnerDecl = typeAliasTemplateDecl(hasName("::gsl::owner"));
const auto IsOwnerType = hasType(OwnerDecl);
const auto LegacyCreatorFunctions = hasAnyListedName(LegacyResourceProducers);
const auto LegacyConsumerFunctions =
hasAnyListedName(LegacyResourceConsumers);
// Legacy functions that are use for resource management but cannot be
// updated to use `gsl::owner<>`, like standard C memory management.
const auto CreatesLegacyOwner =
callExpr(callee(functionDecl(LegacyCreatorFunctions)));
// C-style functions like `::malloc()` sometimes create owners as void*
// which is expected to be cast to the correct type in C++. This case
// must be catched explicitly.
const auto LegacyOwnerCast =
castExpr(hasSourceExpression(CreatesLegacyOwner));
// Functions that do manual resource management but cannot be updated to use
// owner. Best example is `::free()`.
const auto LegacyOwnerConsumers = functionDecl(LegacyConsumerFunctions);
const auto CreatesOwner =
anyOf(cxxNewExpr(), callExpr(callee(functionDecl(
returns(qualType(hasDeclaration(OwnerDecl)))))));
const auto ConsideredOwner = anyOf(IsOwnerType, CreatesOwner);
anyOf(cxxNewExpr(),
callExpr(callee(
functionDecl(returns(qualType(hasDeclaration(OwnerDecl)))))),
CreatesLegacyOwner, LegacyOwnerCast);
const auto ConsideredOwner = eachOf(IsOwnerType, CreatesOwner);
// Find delete expressions that delete non-owners.
Finder->addMatcher(
@ -43,6 +79,21 @@ void OwningMemoryCheck::registerMatchers(MatchFinder *Finder) {
.bind("delete_expr"),
this);
// Ignoring the implicit casts is vital because the legacy owners do not work
// with the 'owner<>' annotation and therefore always implicitly cast to the
// legacy type (even 'void *').
//
// Furthermore, legacy owner functions are assumed to use raw pointers for
// resources. This check assumes that all pointer arguments of a legacy
// functions shall be 'gsl::owner<>'.
Finder->addMatcher(
callExpr(
allOf(callee(LegacyOwnerConsumers),
hasAnyArgument(allOf(unless(ignoringImpCasts(ConsideredOwner)),
hasType(pointerType())))))
.bind("legacy_consumer"),
this);
// Matching assignment to owners, with the rhs not being an owner nor creating
// one.
Finder->addMatcher(binaryOperator(allOf(matchers::isAssignmentOperator(),
@ -133,6 +184,7 @@ void OwningMemoryCheck::check(const MatchFinder::MatchResult &Result) {
bool CheckExecuted = false;
CheckExecuted |= handleDeletion(Nodes);
CheckExecuted |= handleLegacyConsumers(Nodes);
CheckExecuted |= handleExpectedOwner(Nodes);
CheckExecuted |= handleAssignmentAndInit(Nodes);
CheckExecuted |= handleAssignmentFromNewOwner(Nodes);
@ -168,6 +220,22 @@ bool OwningMemoryCheck::handleDeletion(const BoundNodes &Nodes) {
return false;
}
bool OwningMemoryCheck::handleLegacyConsumers(const BoundNodes &Nodes) {
// Result of matching for legacy consumer-functions like `::free()`.
const auto *LegacyConsumer = Nodes.getNodeAs<CallExpr>("legacy_consumer");
// FIXME: `freopen` should be handled seperately because it takes the filename
// as a pointer, which should not be an owner. The argument that is an owner
// is known and the false positive coming from the filename can be avoided.
if (LegacyConsumer) {
diag(LegacyConsumer->getLocStart(),
"calling legacy resource function without passing a 'gsl::owner<>'")
<< LegacyConsumer->getSourceRange();
return true;
}
return false;
}
bool OwningMemoryCheck::handleExpectedOwner(const BoundNodes &Nodes) {
// Result of function call matchers.
const auto *ExpectedOwner = Nodes.getNodeAs<Expr>("expected_owner_argument");

View File

@ -24,17 +24,36 @@ namespace cppcoreguidelines {
class OwningMemoryCheck : public ClangTidyCheck {
public:
OwningMemoryCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context) {}
: ClangTidyCheck(Name, Context),
LegacyResourceProducers(Options.get(
"LegacyResourceProducers", "::malloc;::aligned_alloc;::realloc;"
"::calloc;::fopen;::freopen;::tmpfile")),
LegacyResourceConsumers(Options.get(
"LegacyResourceConsumers", "::free;::realloc;::freopen;::fclose")) {
}
/// Make configuration of checker discoverable.
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
private:
bool handleDeletion(const ast_matchers::BoundNodes &Nodes);
bool handleLegacyConsumers(const ast_matchers::BoundNodes &Nodes);
bool handleExpectedOwner(const ast_matchers::BoundNodes &Nodes);
bool handleAssignmentAndInit(const ast_matchers::BoundNodes &Nodes);
bool handleAssignmentFromNewOwner(const ast_matchers::BoundNodes &Nodes);
bool handleReturnValues(const ast_matchers::BoundNodes &Nodes);
bool handleOwnerMembers(const ast_matchers::BoundNodes &Nodes);
/// List of old C-style functions that create resources.
/// Defaults to
/// `::malloc;::aligned_alloc;::realloc;::calloc;::fopen;::freopen;::tmpfile`.
const std::string LegacyResourceProducers;
/// List of old C-style functions that consume or release resources.
/// Defaults to `::free;::realloc;::freopen;::fclose`.
const std::string LegacyResourceConsumers;
};
} // namespace cppcoreguidelines

View File

@ -20,7 +20,8 @@ the `Guideline Support Library <https://github.com/isocpp/CppCoreGuidelines/blob
All checks are purely type based and not (yet) flow sensitive.
The following examples will demonstrate the correct and incorrect initializations
of owners, assignment is handled the same way.
of owners, assignment is handled the same way. Note that both ``new`` and
``malloc()``-like resource functions are considered to produce resources.
.. code-block:: c++
@ -69,6 +70,33 @@ argument get called with either a ``gsl::owner<T*>`` or a newly created resource
expects_owner(Owner); // Good
expects_owner(new int(42)); // Good as well, recognized created resource
// Port legacy code for better resource-safety
gsl::owner<FILE*> File = fopen("my_file.txt", "rw+");
FILE* BadFile = fopen("another_file.txt", "w"); // Bad, warned
// ... use the file
fclose(File); // Ok, File is annotated as 'owner<>'
fclose(BadFile); // BadFile is not an 'owner<>', will be warned
Options
-------
.. option:: LegacyResourceProducers
Semicolon-separated list of fully qualified names of legacy functions that create
resources but cannot introduce ``gsl::owner<>``.
Defaults to ``::malloc;::aligned_alloc;::realloc;::calloc;::fopen;::freopen;::tmpfile``.
.. option:: LegacyResourceConsumers
Semicolon-separated list of fully qualified names of legacy functions expecting
resource owners as pointer arguments but cannot introduce ``gsl::owner<>``.
Defaults to ``::free;::realloc;::freopen;::fclose``.
Limitations
-----------
@ -82,7 +110,8 @@ Using ``gsl::owner<T*>`` in a typedef or alias is not handled correctly.
The ``gsl::owner<T*>`` is declared as a templated type alias.
In template functions and classes, like in the example below, the information
of the type aliases gets lost. Therefore using ``gsl::owner<T*>`` in a heavy templated
code base might lead to false positives.
code base might lead to false positives.
This limitation results in ``std::vector<gsl::owner<T*>>`` not being diagnosed correctly.
.. code-block:: c++

View File

@ -0,0 +1,61 @@
// RUN: %check_clang_tidy %s cppcoreguidelines-owning-memory %t
namespace gsl {
template <typename T>
using owner = T;
}
namespace std {
// Not actually a vector, but more a dynamic, fixed size array. Just to demonstrate
// functionality or the lack of the same.
template <typename T>
class vector {
public:
vector(unsigned long size, T val) : data{new T[size]}, size{size} {
for (unsigned long i = 0ul; i < size; ++i) {
data[i] = val;
}
}
T *begin() { return data; }
T *end() { return &data[size]; }
T &operator[](unsigned long index) { return data[index]; }
private:
T *data;
unsigned long size;
};
} // namespace std
// All of the following codesnippets should be valid with appropriate 'owner<>' anaylsis,
// but currently the type information of 'gsl::owner<>' gets lost in typededuction.
int main() {
std::vector<gsl::owner<int *>> OwnerStdVector(100, nullptr);
// Rangebased looping in resource vector.
for (auto *Element : OwnerStdVector) {
Element = new int(42);
// CHECK-MESSAGES: [[@LINE-1]]:5: warning: assigning newly created 'gsl::owner<>' to non-owner 'int *'
}
for (auto *Element : OwnerStdVector) {
delete Element;
// CHECK-MESSAGES: [[@LINE-1]]:5: warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead
// CHECK-MESSAGES: [[@LINE-3]]:8: note: variable declared here
}
// Indexbased looping in resource vector.
for (int i = 0; i < 100; ++i) {
OwnerStdVector[i] = new int(42);
// CHECK-MESSAGES: [[@LINE-1]]:5: warning: assigning newly created 'gsl::owner<>' to non-owner 'int *'
}
for (int i = 0; i < 100; ++i) {
delete OwnerStdVector[i];
// CHECK-MESSAGES: [[@LINE-1]]:5: warning: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead
// A note gets emitted here pointing to the return value of the operator[] from the
// vector implementation. Maybe this is considered misleading.
}
return 0;
}

View File

@ -0,0 +1,194 @@
// RUN: %check_clang_tidy %s cppcoreguidelines-owning-memory %t \
// RUN: -config='{CheckOptions: \
// RUN: [{key: cppcoreguidelines-owning-memory.LegacyResourceProducers, value: "::malloc;::aligned_alloc;::realloc;::calloc;::fopen;::freopen;::tmpfile"}, \
// RUN: {key: cppcoreguidelines-owning-memory.LegacyResourceConsumers, value: "::free;::realloc;::freopen;::fclose"}]}' \
// RUN: -- -std=c++11 -nostdlib -nostdinc++
namespace gsl {
template <class T>
using owner = T;
} // namespace gsl
extern "C" {
using size_t = unsigned long;
using FILE = int;
void *malloc(size_t ByteCount);
void *aligned_alloc(size_t Alignment, size_t Size);
void *calloc(size_t Count, size_t SizeSingle);
void *realloc(void *Resource, size_t NewByteCount);
void free(void *Resource);
FILE *tmpfile(void);
FILE *fopen(const char *filename, const char *mode);
FILE *freopen(const char *filename, const char *mode, FILE *stream);
void fclose(FILE *Resource);
}
namespace std {
using ::FILE;
using ::size_t;
using ::fclose;
using ::fopen;
using ::freopen;
using ::tmpfile;
using ::aligned_alloc;
using ::calloc;
using ::free;
using ::malloc;
using ::realloc;
} // namespace std
void nonOwningCall(int *Resource, size_t Size) {}
void nonOwningCall(FILE *Resource) {}
void consumesResource(gsl::owner<int *> Resource, size_t Size) {}
void consumesResource(gsl::owner<FILE *> Resource) {}
void testNonCasted(void *Resource) {}
void testNonCastedOwner(gsl::owner<void *> Resource) {}
FILE *fileFactory1() { return ::fopen("new_file.txt", "w"); }
// CHECK-MESSAGES: [[@LINE-1]]:24: warning: returning a newly created resource of type 'FILE *' (aka 'int *') or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>'
gsl::owner<FILE *> fileFactory2() { return std::fopen("new_file.txt", "w"); } // Ok
int *arrayFactory1() { return (int *)std::malloc(100); }
// CHECK-MESSAGES: [[@LINE-1]]:24: warning: returning a newly created resource of type 'int *' or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>'
gsl::owner<int *> arrayFactory2() { return (int *)std::malloc(100); } // Ok
void *dataFactory1() { return std::malloc(100); }
// CHECK-MESSAGES: [[@LINE-1]]:24: warning: returning a newly created resource of type 'void *' or 'gsl::owner<>' from a function whose return type is not 'gsl::owner<>'
gsl::owner<void *> dataFactory2() { return std::malloc(100); } // Ok
void test_resource_creators() {
const unsigned int ByteCount = 25 * sizeof(int);
int Bad = 42;
int *IntArray1 = (int *)std::malloc(ByteCount);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>'
int *IntArray2 = static_cast<int *>(std::malloc(ByteCount)); // Bad
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>'
void *IntArray3 = std::malloc(ByteCount);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'void *' with a newly created 'gsl::owner<>'
int *IntArray4 = (int *)::malloc(ByteCount);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>'
int *IntArray5 = static_cast<int *>(::malloc(ByteCount)); // Bad
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>'
void *IntArray6 = ::malloc(ByteCount);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'void *' with a newly created 'gsl::owner<>'
gsl::owner<int *> IntArray7 = (int *)malloc(ByteCount); // Ok
gsl::owner<void *> IntArray8 = malloc(ByteCount); // Ok
gsl::owner<int *> IntArray9 = &Bad;
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: expected initialization with value of type 'gsl::owner<>'; got 'int *'
nonOwningCall((int *)malloc(ByteCount), 25);
// CHECK-MESSAGES: [[@LINE-1]]:24: warning: initializing non-owner argument of type 'int *' with a newly created 'gsl::owner<>'
nonOwningCall((int *)::malloc(ByteCount), 25);
// CHECK-MESSAGES: [[@LINE-1]]:24: warning: initializing non-owner argument of type 'int *' with a newly created 'gsl::owner<>'
consumesResource((int *)malloc(ByteCount), 25); // Ok
consumesResource((int *)::malloc(ByteCount), 25); // Ok
testNonCasted(malloc(ByteCount));
// CHECK-MESSAGES: [[@LINE-1]]:17: warning: initializing non-owner argument of type 'void *' with a newly created 'gsl::owner<>'
testNonCastedOwner(gsl::owner<void *>(malloc(ByteCount))); // Ok
testNonCastedOwner(malloc(ByteCount)); // Ok
FILE *File1 = std::fopen("test_name.txt", "w+");
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
FILE *File2 = ::fopen("test_name.txt", "w+");
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
gsl::owner<FILE *> File3 = ::fopen("test_name.txt", "w+"); // Ok
FILE *File4;
File4 = ::fopen("test_name.txt", "w+");
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: assigning newly created 'gsl::owner<>' to non-owner 'FILE *' (aka 'int *')
gsl::owner<FILE *> File5;
File5 = ::fopen("test_name.txt", "w+"); // Ok
File5 = File1;
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: expected assignment source to be of type 'gsl::owner<>'; got 'FILE *' (aka 'int *')
gsl::owner<FILE *> File6 = File1;
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: expected initialization with value of type 'gsl::owner<>'; got 'FILE *' (aka 'int *')
FILE *File7 = tmpfile();
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
gsl::owner<FILE *> File8 = tmpfile(); // Ok
nonOwningCall(::fopen("test_name.txt", "r"));
// CHECK-MESSAGES: [[@LINE-1]]:17: warning: initializing non-owner argument of type 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
nonOwningCall(std::fopen("test_name.txt", "r"));
// CHECK-MESSAGES: [[@LINE-1]]:17: warning: initializing non-owner argument of type 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
consumesResource(::fopen("test_name.txt", "r")); // Ok
int *HeapPointer3 = (int *)aligned_alloc(16ul, 4ul * 32ul);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>'
gsl::owner<int *> HeapPointer4 = static_cast<int *>(aligned_alloc(16ul, 4ul * 32ul)); // Ok
void *HeapPointer5 = calloc(10ul, 4ul);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'void *' with a newly created 'gsl::owner<>'
gsl::owner<void *> HeapPointer6 = calloc(10ul, 4ul); // Ok
}
void test_legacy_consumers() {
int StackInteger = 42;
int *StackPointer = &StackInteger;
int *HeapPointer1 = (int *)malloc(100);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'int *' with a newly created 'gsl::owner<>'
gsl::owner<int *> HeapPointer2 = (int *)malloc(100);
std::free(StackPointer);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>'
std::free(HeapPointer1);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>'
std::free(HeapPointer2); // Ok
// CHECK MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>'
// FIXME: the check complains about initialization of 'void *' with new created owner.
// This happens, because the argument of `free` is not marked as 'owner<>' (and cannot be),
// and the check will not figure out could be meant as owner.
// This property will probably never be fixed, because it is probably a rather rare
// use-case and 'owner<>' should be wrapped in RAII classes anyway!
std::free(std::malloc(100)); // Ok but silly :)
// CHECK-MESSAGES: [[@LINE-1]]:13: warning: initializing non-owner argument of type 'void *' with a newly created 'gsl::owner<>'
// Demonstrate, that multi-argument functions are diagnosed as well.
std::realloc(StackPointer, 200);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>'
std::realloc(HeapPointer1, 200);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>'
std::realloc(HeapPointer2, 200); // Ok
std::realloc(std::malloc(100), 200); // Ok but silly
// CHECK-MESSAGES: [[@LINE-1]]:16: warning: initializing non-owner argument of type 'void *' with a newly created 'gsl::owner<>'
fclose(fileFactory1());
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling legacy resource function without passing a 'gsl::owner<>'
fclose(fileFactory2()); // Ok, same as FIXME with `free(malloc(100))` applies here
// CHECK-MESSAGES: [[@LINE-1]]:10: warning: initializing non-owner argument of type 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
gsl::owner<FILE *> File1 = fopen("testfile.txt", "r"); // Ok
FILE *File2 = freopen("testfile.txt", "w", File1);
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
// CHECK-MESSAGES: [[@LINE-2]]:17: warning: calling legacy resource function without passing a 'gsl::owner<>'
// FIXME: The warning for not passing and owner<> is a false positive since both the filename and the
// mode are not supposed to be owners but still pointers. The check is to coarse for
// this function. Maybe `freopen` gets special treatment.
gsl::owner<FILE *> File3 = freopen("testfile.txt", "w", File2); // Bad, File2 no owner
// CHECK-MESSAGES: [[@LINE-1]]:30: warning: calling legacy resource function without passing a 'gsl::owner<>'
FILE *TmpFile = tmpfile();
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
FILE *File6 = freopen("testfile.txt", "w", TmpFile); // Bad, both return and argument
// CHECK-MESSAGES: [[@LINE-1]]:3: warning: initializing non-owner 'FILE *' (aka 'int *') with a newly created 'gsl::owner<>'
// CHECK-MESSAGES: [[@LINE-2]]:17: warning: calling legacy resource function without passing a 'gsl::owner<>'
}