From patchwork Sun May 14 11:12:26 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Volker Reichelt X-Patchwork-Id: 762122 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wQgzq0j1yz9s8H for ; Sun, 14 May 2017 21:12:53 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="hoLNqn2V"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:subject:to:in-reply-to:message-id:references:mime-version :content-type; q=dns; s=default; b=tlNV9T5dfGc0/E76gaVNY3Y+t0kfr tklMQhMS6RiDGSpVjYsoOvvltKNgUQUJkzAxh0I1ShTHs6oFg/8fJlFKAHuN7qW3 tAjb1xI5Jn22satvC4s9ovr4VvlGbK1fAszZzeG8B0BcV8LRq33I3dG7RrrgqM96 A1dX/n1ItTwluA= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:subject:to:in-reply-to:message-id:references:mime-version :content-type; s=default; bh=XalsqvZbx+zFRMf7vzyp3T9ZyOU=; b=hoL Nqn2V7RMXszjxepjU+VIluvtWDTHO8PTMY4SVxpVte5HqtZABQ3P5rlTjc1DTnMP Lvo/3/rOfSjijkHPXF6lY9no1BdD5i62uvYKxeZ98vgxTNyEMz4K/ShLTfegygWx 9i9TrdqL75SFoQcSwT1skS3RgeIsAof6h1TDv75w= Received: (qmail 11843 invoked by alias); 14 May 2017 11:12:38 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 11803 invoked by uid 89); 14 May 2017 11:12:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.2 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_LOW, RP_MATCHES_RCVD, SPAM_BODY, SPF_PASS autolearn=ham version=3.3.2 spammy=sk:www.sec, sk:wwwsec, reichelt, confluence X-HELO: cc-smtpout2.netcologne.de Received: from cc-smtpout2.netcologne.de (HELO cc-smtpout2.netcologne.de) (89.1.8.212) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 14 May 2017 11:12:31 +0000 Received: from cc-smtpin3.netcologne.de (cc-smtpin3.netcologne.de [89.1.8.203]) by cc-smtpout2.netcologne.de (Postfix) with ESMTP id 9B5E8125C3; Sun, 14 May 2017 13:12:29 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by cc-smtpin3.netcologne.de (Postfix) with ESMTP id 8E54611DE5; Sun, 14 May 2017 13:12:29 +0200 (CEST) Received: from [89.0.2.141] (helo=cc-smtpin3.netcologne.de) by localhost with ESMTP (eXpurgate 4.1.9) (envelope-from ) id 59183b9d-0242-7f0000012729-7f0000019fe8-1 for ; Sun, 14 May 2017 13:12:29 +0200 Received: from linux-w03z.fritz.box (xdsl-89-0-2-141.netcologne.de [89.0.2.141]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by cc-smtpin3.netcologne.de (Postfix) with ESMTPSA; Sun, 14 May 2017 13:12:27 +0200 (CEST) Date: Sun, 14 May 2017 13:12:26 +0200 (CEST) From: Volker Reichelt Subject: Re: {PATCH] New C++ warning -Wcatch-value To: Martin Sebor , gcc-patches@gcc.gnu.org In-Reply-To: Message-ID: References: <297400e4-5e33-08aa-8048-cffd3626740a@gmail.com> MIME-Version: 1.0 Content-Disposition: INLINE On 7 May, Martin Sebor wrote: > On 05/07/2017 02:03 PM, Volker Reichelt wrote: >> On 2 May, Martin Sebor wrote: >>> On 05/01/2017 02:38 AM, Volker Reichelt wrote: >>>> Hi, >>>> >>>> catching exceptions by value is a bad thing, as it may cause slicing, i.e. >>>> a) a superfluous copy >>>> b) which is only partial. >>>> See also https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#e15-catch-exceptions-from-a-hierarchy-by-reference >>>> >>>> To warn the user about catch handlers of non-reference type, >>>> the following patch adds a new C++/ObjC++ warning option "-Wcatch-value". >>> >>> I think the problems related to catching exceptions by value >>> apply to (a subset of) class types but not so much to fundamental >>> types. I would expect indiscriminately warning on every type to >>> be overly restrictive. >>> >>> The Enforcement section of the C++ guideline suggests to >>> >>> Flag by-value exceptions if their types are part of a hierarchy >>> (could require whole-program analysis to be perfect). >>> >>> The corresponding CERT C++ Coding Standard guideline offers >>> a similar suggestion here: >>> >>> https://www.securecoding.cert.org/confluence/x/TAD5CQ >>> >>> so I would suggest to model the warning on that approach (within >>> limits of a single translation unit, of course). I.e., warn only >>> for catching by value objects of non-trivial types, or perhaps even >>> only polymorphic types? >>> >>> Martin >> >> I've never seen anybody throw integers in real-world code, so I didn't >> want to complicate things for this case. But maybe I should only warn >> about class-types. >> >> IMHO it makes sense to warn about non-polymorphic class types >> (although slicing is not a problem there), because you still have to >> deal with redundant copies. >> >> Another thing would be pointers. I've never seen pointers in catch >> handlers (except some 'catch (const char*)' which I would consider >> bad practice). Therefore I'd like to warn about 'catch (const A*)' >> which might be a typo that should read 'catch (const A&)' instead. >> >> Would that be OK? > > To my knowledge, catch by value of non-polymorphic types (and > certainly fundamental types) is not a cause of common bugs. > It's part of the recommended practice to throw by value, catch > by reference, which is grounded in avoiding the slicing problem. > It's also sometimes recommended for non-trivial class types to > avoid creating a copy of the object (which, for non-trivial types, > may need to allocate resource and could throw). Otherwise, it's > not dissimilar to pass-by value vs pass-by-reference (or even > pass-by-pointer). Both may be good practices for some types or > in some situations but neither is necessary to avoid bugs or > universally applicable to achieve superior performance. > > The pointer case is interesting. In C++ Coding Standards, > Sutter and Alexandrescu recommend to throw (and catch) smart > pointers over plain pointers because it obviates having to deal > with memory management issues. That's sound advice but it seems > more like a design guideline than a coding rule aimed at directly > preventing bugs. I also think that the memory management bugs > that it might find might be more easily detected at the throw > site instead. E.g., warning on the throw expression below: > > { > Exception e; > throw &e; > } > > or perhaps even on > > { > throw *new Exception (); > } > > A more sophisticated (and less restrictive) checker could detect > and warn on "throw " if it found a catch (T) or catch (T&) > in the same file and no catch (T*) (but not warn otherwise). > > Martin > > PS After re-reading some of the coding guidelines on this topic > it occurs to me that (if your patch doesn't handle this case yet) > it might be worth considering to enhance it to also warn on > rethrowing caught polymorphic objects (i.e., warn on > > catch (E &e) { throw e; } > > and suggest to use "throw;" instead, for the same reason: to > help avoid slicing. > > PPS It may be a useful feature to implement some of other ideas > you mentioned (e.g., throw by value rather than pointer) but it > feels like a separate and more ambitious project than detecting > the relatively common and narrow slicing problem. So how about the following then? I stayed with the catch part and added a parameter to the warning to let the user decide on the warnings she/he wants to get: -Wcatch-value=n. -Wcatch-value=1 only warns for polymorphic classes that are caught by value (to avoid slicing), -Wcatch-value=2 warns for all classes that are caught by value (to avoid copies). And finally -Wcatch-value=3 warns for everything not caught by reference to find typos (like pointer instead of reference) and bad coding practices. Bootstrapped and regtested on x86_64-pc-linux-gnu. OK for trunk? If so, would it make sense to add -Wcatch-value=1 to -Wextra or even -Wall? I would do this in a seperate patch, becuase I haven't checked what that would mean for the testsuite. Regards, Volker 2017-05-13 Volker Reichelt * doc/invoke.texi (-Wcatch-value=): Document new warning option. =================================================================== Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 248004) +++ gcc/doc/invoke.texi (working copy) @@ -265,7 +265,7 @@ -Wno-builtin-declaration-mismatch @gol -Wno-builtin-macro-redefined -Wc90-c99-compat -Wc99-c11-compat @gol -Wc++-compat -Wc++11-compat -Wc++14-compat -Wcast-align -Wcast-qual @gol --Wchar-subscripts -Wchkp -Wclobbered -Wcomment @gol +-Wchar-subscripts -Wchkp -Wcatch-value=@var{n} -Wclobbered -Wcomment @gol -Wconditionally-supported @gol -Wconversion -Wcoverage-mismatch -Wno-cpp -Wdangling-else -Wdate-time @gol -Wdelete-incomplete @gol @@ -5832,6 +5832,14 @@ literals to @code{char *}. This warning is enabled by default for C++ programs. +@item -Wcatch-value=@var{n} @r{(C++ and Objective-C++ only)} +@opindex Wcatch-value +Warn about catch handlers that do not catch via reference. +With @option{-Wcatch-value=1} warn about polymorphic class types that +are caught by value. With @option{-Wcatch-value=2} warn about all class +types that are caught by value. With @option{-Wcatch-value=3} warn about +all types that are not caught by reference. + @item -Wclobbered @opindex Wclobbered @opindex Wno-clobbered =================================================================== 2017-05-13 Volker Reichelt * c.opt (Wcatch-value=): New C++ warning flag. Index: gcc/c-family/c.opt =================================================================== --- gcc/c-family/c.opt (revision 248004) +++ gcc/c-family/c.opt (working copy) @@ -388,6 +388,10 @@ C ObjC C++ ObjC++ Var(warn_cast_qual) Warning Warn about casts which discard qualifiers. +Wcatch-value= +C++ ObjC++ Var(warn_catch_value) Warning Joined RejectNegative UInteger +Warn about catch handlers of non-reference type. + Wchar-subscripts C ObjC C++ ObjC++ Var(warn_char_subscripts) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Warn about subscripts whose type is \"char\". =================================================================== 2017-05-13 Volker Reichelt * semantics.c (finish_handler_parms): Warn about non-reference type catch handlers. Index: gcc/cp/semantics.c =================================================================== --- gcc/cp/semantics.c (revision 248004) +++ gcc/cp/semantics.c (working copy) @@ -1321,7 +1321,28 @@ } } else - type = expand_start_catch_block (decl); + { + type = expand_start_catch_block (decl); + if (warn_catch_value + && type != NULL_TREE + && type != error_mark_node + && TREE_CODE (TREE_TYPE (decl)) != REFERENCE_TYPE) + { + tree orig_type = TREE_TYPE (decl); + if (CLASS_TYPE_P (orig_type)) + { + if (TYPE_POLYMORPHIC_P (orig_type)) + warning (OPT_Wcatch_value_, + "catching polymorphic type %q#T by value", orig_type); + else if (warn_catch_value > 1) + warning (OPT_Wcatch_value_, + "catching type %q#T by value", orig_type); + } + else if (warn_catch_value > 2) + warning (OPT_Wcatch_value_, + "catching non-reference type %q#T", orig_type); + } + } HANDLER_TYPE (handler) = type; } =================================================================== 2017-05-13 Volker Reichelt * g++.dg/warn/Wcatch-value-1.C: New test. * g++.dg/warn/Wcatch-value-2.C: New test. * g++.dg/warn/Wcatch-value-3.C: New test. Index: gcc/testsuite/g++.dg/warn/Wcatch-value-1.C =================================================================== --- gcc/testsuite/g++.dg/warn/Wcatch-value-1.C 2017-05-13 +++ gcc/testsuite/g++.dg/warn/Wcatch-value-1.C 2017-05-13 @@ -0,0 +1,64 @@ +// { dg-options "-Wcatch-value=1" } + +struct A { virtual ~A() {} }; +struct B : A {}; +struct C {}; +struct D : C {}; + +void foo() +{ + try {} + catch (D) {} + catch (C) {} + catch (B) {} // { dg-warning "catching polymorphic type" } + catch (A) {} // { dg-warning "catching polymorphic type" } + catch (A*) {} + catch (int) {} + + try {} + catch (D&) {} + catch (C&) {} + catch (B&) {} + catch (A&) {} + catch (A*) {} + catch (int&) {} +} + +template void foo1() +{ + try {} + catch (T) {} // { dg-warning "catching polymorphic type" } +} + +template void foo2() +{ + try {} + catch (T*) {} + + try {} + catch (T&) {} + + try {} + catch (const T&) {} +} + +void bar() +{ + foo1(); + foo1(); + foo1(); + foo1(); + foo1(); + + foo1(); + foo1(); // { dg-message "required" } + foo1(); // { dg-message "required" } + foo1(); + foo1(); + + foo2(); + foo2(); + foo2(); + foo2(); + foo2(); +} Index: gcc/testsuite/g++.dg/warn/Wcatch-value-2.C =================================================================== --- gcc/testsuite/g++.dg/warn/Wcatch-value-2.C 2017-05-13 +++ gcc/testsuite/g++.dg/warn/Wcatch-value-2.C 2017-05-13 @@ -0,0 +1,64 @@ +// { dg-options "-Wcatch-value=2" } + +struct A { virtual ~A() {} }; +struct B : A {}; +struct C {}; +struct D : C {}; + +void foo() +{ + try {} + catch (D) {} // { dg-warning "catching type" } + catch (C) {} // { dg-warning "catching type" } + catch (B) {} // { dg-warning "catching polymorphic type" } + catch (A) {} // { dg-warning "catching polymorphic type" } + catch (A*) {} + catch (int) {} + + try {} + catch (D&) {} + catch (C&) {} + catch (B&) {} + catch (A&) {} + catch (A*) {} + catch (int&) {} +} + +template void foo1() +{ + try {} + catch (T) {} // { dg-warning "catching" } +} + +template void foo2() +{ + try {} + catch (T*) {} + + try {} + catch (T&) {} + + try {} + catch (const T&) {} +} + +void bar() +{ + foo1(); + foo1(); + foo1(); + foo1(); + foo1(); + + foo1(); + foo1(); // { dg-message "required" } + foo1(); // { dg-message "required" } + foo1(); // { dg-message "required" } + foo1(); // { dg-message "required" } + + foo2(); + foo2(); + foo2(); + foo2(); + foo2(); +} Index: gcc/testsuite/g++.dg/warn/Wcatch-value-3.C =================================================================== --- gcc/testsuite/g++.dg/warn/Wcatch-value-3.C 2017-05-13 +++ gcc/testsuite/g++.dg/warn/Wcatch-value-3.C 2017-05-13 @@ -0,0 +1,64 @@ +// { dg-options "-Wcatch-value=3" } + +struct A { virtual ~A() {} }; +struct B : A {}; +struct C {}; +struct D : C {}; + +void foo() +{ + try {} + catch (D) {} // { dg-warning "catching type" } + catch (C) {} // { dg-warning "catching type" } + catch (B) {} // { dg-warning "catching polymorphic type" } + catch (A) {} // { dg-warning "catching polymorphic type" } + catch (A*) {} // { dg-warning "catching non-reference type" } + catch (int) {} // { dg-warning "catching non-reference type" } + + try {} + catch (D&) {} + catch (C&) {} + catch (B&) {} + catch (A&) {} + catch (A*) {} // { dg-warning "catching non-reference type" } + catch (int&) {} +} + +template void foo1() +{ + try {} + catch (T) {} // { dg-warning "catching" } +} + +template void foo2() +{ + try {} + catch (T*) {} // { dg-warning "catching non-reference type" } + + try {} + catch (T&) {} + + try {} + catch (const T&) {} +} + +void bar() +{ + foo1(); + foo1(); + foo1(); + foo1(); + foo1(); + + foo1(); // { dg-message "required" } + foo1(); // { dg-message "required" } + foo1(); // { dg-message "required" } + foo1(); // { dg-message "required" } + foo1(); // { dg-message "required" } + + foo2(); // { dg-message "required" } + foo2(); // { dg-message "required" } + foo2(); // { dg-message "required" } + foo2(); // { dg-message "required" } + foo2(); // { dg-message "required" } +}