[PATCH] D11328: [clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidy
Aaron Ballman
aaron at aaronballman.com
Sat Jul 25 10:07:18 PDT 2015
On Sat, Jul 25, 2015 at 4:33 AM, Tobias Langner
<randomcppprogrammer at gmail.com> wrote:
> Hi,
>
> I modified the code. During this, additional questions popped up that I’d like to resolve before I put up a new patch. Please check my comments below.
>
>> On 20 Jul 2015, at 17:17, Aaron Ballman <aaron at aaronballman.com> wrote:
>>
>> On Sat, Jul 18, 2015 at 11:44 AM, Tobias Langner
>> <randomcppprogrammer at gmail.com> wrote:
>>> randomcppprogrammer created this revision.
>>> randomcppprogrammer added reviewers: klimek, cfe-commits, jbcoe.
>>>
>>> This patch contains an implementation to check whether exceptions where thrown by value and caught by const reference. This is a guideline mentioned in different places, e.g. "C++ Coding Standards" by H. Sutter.
>>
>> I happened to be working on this exact same problem while developing
>> rules for CERT's C++ secure coding guidelines, using a very similar
>> approach. Thank you for looking into this! If you'd like another
>> coding standard to compare this to:
>> https://www.securecoding.cert.org/confluence/display/cplusplus/ERR61-CPP.+Catch+exceptions+by+lvalue+reference
>> (we have a recommendation for throwing rvalues as well).
>>
>> A few things I noticed:
>>
>> This is not testing whether you are throwing by value, but are instead
>> testing whether you are throwing a pointer. Is this intentional?
>
> Yes. It’s easier to test and since you can only throw by pointer or by value, it’s in my opinion equivalent.
You can throw things other than pointers and values. Consider:
void f(Foo &f) { throw f; } // Okay
That's not throwing a value or a pointer.
I think the advice I would like to see is more generally to throw
rvalues. You want to catch dubious code like:
void f(Foo param) {
Foo SomeLocal;
throw SomeLocal; // Not good
throw param; // Not good
}
but allow code like:
Foo g();
void f(Foo &obj) {
throw Foo(); // ok
throw g(); // ok
throw obj; // ok, even though it's not an rvalue, it's a common code pattern
}
>
>> Also,
>> not all thrown pointers are dangerous. For instance: throw "This is a
>> cheap exception"; is very reasonable code, while throw new
>> BadIdea(12); is not.
>
> this can be handled by allowing StringLiterals to be thrown, but no other pointers.
Doesn't builtinType() already handle that?
> My problem with this is that I don’t know how to recognise
> this at the catch statement without removing the check for pointers. The only idea that I have right now is to check whether the pointed to type is ‘const char’ or ‘const wchar_t’ or one of the new ones for unicode. But it feels a little bit brittle.
I think that seems like the valid approach, since some pointers are
acceptable to catch on.
>> As for catching, catching by const lvalue reference is not always the
>> correct recommendation. For instance, the catch block could be
>> modifying the exception object and rethrowing it, at which point you
>> cannot catch by const reference. Also, not all types really need to be
>> caught by reference, such as builtin types (think int) or trivial
>> types.
>
> I added the check for builtin & trivial types - that was a good idea. Regarding catching as reference (opposed to const reference), I could think of 2 different options:
> - require nolint - which I think is viable. It marks in source code that someone had thought about this potential problem.
> - having an option for the test (something like enforce const reference).
> Both have their benefits & drawbacks. If a project requires at most call sites that the exception is modified, the second one is better - but I don’t know how to test different option given the current test infrastructure. If modifying an exception is rare, I think the first option is better as it forces to comment this rare situation.
I think that modifying the object is less common, so there's something
to be said for the nolint idea. However, I don't personally see a
reason to restrict to catching by const reference as opposed to any
lvalue reference (modulo trivial types, etc).
> I would not like to remove the test for const-ness on catch because it can be a real performance hog that some might not be aware about.
I cannot imagine a situation where catching by reference is less
performant than catching by const reference, in practice. Do you have
some example code that demonstrates a performance difference?
> Any advice which way to go?
I still think that there's no need to guide people towards catching
const references as opposed to catching by lvalue reference (const or
otherwise). That addresses the correctness and security concerns.
Constness seems like an orthogonal checker that focuses on making your
code const correct (which would also be very useful IMO). By
separating those concerns, the user has the most flexibility with the
least amount of annotations or gymnastics.
~Aaron
>>
>> Some comments on the code below.
>>
>>>
>>> Patch by Tobias Langner
>>>
>>> http://reviews.llvm.org/D11328
>>>
>>> Files:
>>> clang-tidy/misc/CMakeLists.txt
>>> clang-tidy/misc/MiscTidyModule.cpp
>>> clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp
>>> clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h
>>> test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp
>>
>>> Index: test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp
>>> ===================================================================
>>> --- /dev/null
>>> +++ test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp
>>> @@ -0,0 +1,71 @@
>>> +// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-throw-by-value-catch-by-reference %t
>>> +// REQUIRES: shell
>>> +
>>> +class logic_error {
>>> +public:
>>> + logic_error(const char *message) {}
>>> +};
>>> +
>>> +int selector;
>>
>> Don't need this.
> Done
>>
>>> +typedef logic_error *logic_ptr;
>>> +typedef logic_ptr logic_double_typedef;
>>> +
>>> +void testThrowFunc() {
>>> + switch (selector) {
>>
>> Or the switch.
> Done
>>
>>> + case 0:
>>> + throw new logic_error("by pointer");
>>> + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: avoid throwing pointer
>>> + // [misc-throw-by-value-catch-by-reference]
>>> + case 1: {
>>> + logic_ptr tmp = new logic_error("by pointer");
>>> + throw tmp;
>>> + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: avoid throwing pointer
>>> + // [misc-throw-by-value-catch-by-reference]
>>> + }
>>> + case 2:
>>> + throw logic_error("by value");
>>> + }
>>> +}
>>> +
>>> +void catchByPointer() {
>>> + try {
>>> + testThrowFunc();
>>> + } catch (logic_error *e) {
>>> + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by const reference
>>> + // [misc-throw-by-value-catch-by-reference]
>>> + }
>>> +}
>>> +
>>> +void catchByValue() {
>>> + try {
>>> + testThrowFunc();
>>> + } catch (logic_error e) {
>>> + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by const reference
>>> + // [misc-throw-by-value-catch-by-reference]
>>> + }
>>> +}
>>> +
>>> +void catchByReference() {
>>> + try {
>>> + testThrowFunc();
>>> + } catch (logic_error &e) {
>>> + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by const reference to
>>> + // avoid unnecessary copies [misc-throw-by-value-catch-by-reference]
>>> + }
>>> +}
>>> +
>>> +void catchByConstReference() {
>>> + try {
>>> + testThrowFunc();
>>> + } catch (const logic_error &e) {
>>> + }
>>> +}
>>> +
>>> +void catchTypedef() {
>>> + try {
>>> + testThrowFunc();
>>> + } catch (logic_ptr e) {
>>> + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by const reference
>>> + // [misc-throw-by-value-catch-by-reference]
>>> + }
>>> +}
>>> Index: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h
>>> ===================================================================
>>> --- /dev/null
>>> +++ clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h
>>> @@ -0,0 +1,32 @@
>>> +//===--- ThrowByValueCatchByReferenceCheck.h - clang-tidy--------*- C++ -*-===//
>>> +//
>>> +// The LLVM Compiler Infrastructure
>>> +//
>>> +// This file is distributed under the University of Illinois Open Source
>>> +// License. See LICENSE.TXT for details.
>>> +//
>>> +//===----------------------------------------------------------------------===//
>>> +
>>> +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H
>>> +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H
>>> +
>>> +#include "../ClangTidy.h"
>>> +
>>> +namespace clang {
>>> +namespace tidy {
>>> +
>>> +///\brief checks for locations that do not throw by value
>>> +// or catch by reference.
>>> +class ThrowByValueCatchByReferenceCheck : public ClangTidyCheck {
>>> +public:
>>
>> Since everything's public, perhaps just use a struct instead?
>
> for me structs are close to pods (and some static analysis tools like QAC++ actually have rules that enforce that). This is clearly not a pod so I think class is appropriate (unless that’s against the coding guidelines of the clang project).
We do not differentiate between struct and class because that's an
arbitrary delineation (they are identical aside from default access
rights). Most often, we use struct when everything is public, and
class otherwise, because it declutters the code.
~Aaron
>
>>
>>> + ThrowByValueCatchByReferenceCheck(StringRef Name, ClangTidyContext *Context)
>>> + : ClangTidyCheck(Name, Context) {}
>>> + void registerMatchers(ast_matchers::MatchFinder *Finder) override;
>>> + void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
>>> +};
>>> +
>>> +} // namespace tidy
>>> +} // namespace clang
>>> +
>>> +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H
>>> +
>>> Index: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp
>>> ===================================================================
>>> --- /dev/null
>>> +++ clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp
>>> @@ -0,0 +1,57 @@
>>> +//===--- ThrowByValueCatchByReferenceCheck.cpp - clang-tidy----------------===//
>>> +//
>>> +// The LLVM Compiler Infrastructure
>>> +//
>>> +// This file is distributed under the University of Illinois Open Source
>>> +// License. See LICENSE.TXT for details.
>>> +//
>>> +//===----------------------------------------------------------------------===//
>>> +
>>> +#include "ThrowByValueCatchByReferenceCheck.h"
>>> +#include "clang/AST/ASTContext.h"
>>> +#include "clang/ASTMatchers/ASTMatchFinder.h"
>>> +
>>> +using namespace clang::ast_matchers;
>>> +
>>> +namespace clang {
>>> +namespace tidy {
>>> +
>>> +void ThrowByValueCatchByReferenceCheck::registerMatchers(MatchFinder *Finder) {
>>> + Finder->addMatcher(throwExpr().bind("throw"), this);
>>> + Finder->addMatcher(catchStmt().bind("catch"), this);
>>> +}
>>> +
>>> +void ThrowByValueCatchByReferenceCheck::check(
>>> + const MatchFinder::MatchResult &Result) {
>>> + const auto *throwExpr = Result.Nodes.getNodeAs<CXXThrowExpr>("throw");
>>> + if (throwExpr != nullptr) {
>>> + auto *subExpr = throwExpr->getSubExpr();
>>> + auto qualType = subExpr->getType();
>>> + auto *type = qualType.getTypePtr();
>>> + if (type->isPointerType() == true) { // throwing a pointer
>>
>> Should drop the ==; also, elide the braces to match the coding conventions.
> done
>>
>>> + diag(subExpr->getLocStart(), "avoid throwing pointer");
>>> + }
>>> + }
>>> + const auto *catchStmt = Result.Nodes.getNodeAs<CXXCatchStmt>("catch");
>>> + if (catchStmt != nullptr) {
>>> + auto caughtType = catchStmt->getCaughtType();
>>> + auto *type = caughtType.getTypePtr();
>>> + auto *varDecl = catchStmt->getExceptionDecl();
>>
>> Not all catch clauses have a type, such as catch (...). Should guard
>> against type being null, and include a test case for this. That also
>> goes for varDecl, so you may want to make the relationship more clear
>> that if one is null, they both are null.
> done
>>
>>> + if (type->isPointerType()) {
>>> + diag(varDecl->getLocStart(),
>>> + "catch by const reference");
>>> + } else if (!type->isReferenceType()) {
>>> + diag(varDecl->getLocStart(), "catch by const reference");
>>> + } else {
>>> + auto* reference = type->castAs<const ReferenceType>();
>>> + assert(reference!=nullptr);
>>> + if (reference->getPointeeType().isConstQualified() == false) {
>>> + diag(varDecl->getLocStart(),
>>> + "catch by const reference to avoid unnecessary copies");
>>> + }
>>> + }
>>> + }
>>> +}
>>> +
>>> +} // namespace tidy
>>> +} // namespace clang
>>> Index: clang-tidy/misc/MiscTidyModule.cpp
>>> ===================================================================
>>> --- clang-tidy/misc/MiscTidyModule.cpp
>>> +++ clang-tidy/misc/MiscTidyModule.cpp
>>> @@ -21,6 +21,7 @@
>>> #include "NoexceptMoveConstructorCheck.h"
>>> #include "StaticAssertCheck.h"
>>> #include "SwappedArgumentsCheck.h"
>>> +#include "ThrowByValueCatchByReferenceCheck.h"
>>> #include "UndelegatedConstructor.h"
>>> #include "UniqueptrResetReleaseCheck.h"
>>> #include "UnusedRAIICheck.h"
>>> @@ -54,6 +55,8 @@
>>> "misc-static-assert");
>>> CheckFactories.registerCheck<SwappedArgumentsCheck>(
>>> "misc-swapped-arguments");
>>> + CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(
>>> + "misc-throw-by-value-catch-by-reference");
>>> CheckFactories.registerCheck<UndelegatedConstructorCheck>(
>>> "misc-undelegated-constructor");
>>> CheckFactories.registerCheck<UniqueptrResetReleaseCheck>(
>>> Index: clang-tidy/misc/CMakeLists.txt
>>> ===================================================================
>>> --- clang-tidy/misc/CMakeLists.txt
>>> +++ clang-tidy/misc/CMakeLists.txt
>>> @@ -13,6 +13,7 @@
>>> NoexceptMoveConstructorCheck.cpp
>>> StaticAssertCheck.cpp
>>> SwappedArgumentsCheck.cpp
>>> + ThrowByValueCatchByReferenceCheck.cpp
>>> UndelegatedConstructor.cpp
>>> UnusedRAIICheck.cpp
>>> UniqueptrResetReleaseCheck.cpp
>>>
>>
>> ~Aaron
>
More information about the cfe-commits
mailing list