{PATCH] New C++ warning -Wcatch-value

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 <T*>" 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  <v.reichelt@netcologne.de>

	* doc/invoke.texi (-Wcatch-value=): Document new warning option.

===================================================================

{PATCH] New C++ warning -Wcatch-value

Commit Message

Comments

Patch