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

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

[Volker Reichelt]

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".

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Regards,
Volker


2017-05-01  Volker Reichelt  <v.reichelt@netcologne.de>

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

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 247416)
+++ 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  -Wclobbered  -Wcomment  @gol
 -Wconditionally-supported  @gol
 -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
 -Wdelete-incomplete @gol
@@ -5827,6 +5827,11 @@
 literals to @code{char *}.  This warning is enabled by default for C++
 programs.
 
+@item -Wcatch-value @r{(C++ and Objective-C++ only)}
+@opindex Wcatch-value
+@opindex Wno-catch-value
+Warn about catch handler of non-reference type.
+
 @item -Wclobbered
 @opindex Wclobbered
 @opindex Wno-clobbered
===================================================================

2017-05-01  Volker Reichelt  <v.reichelt@netcologne.de>

	* c.opt (Wcatch-value): New C++ warning flag.

Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 247416)
+++ 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
+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-01  Volker Reichelt  <v.reichelt@netcologne.de>

	* semantics.c (finish_handler_parms): Warn about non-reference type
	catch handlers.

Index: gcc/cp/semantics.c
===================================================================
--- gcc/cp/semantics.c	(revision 247416)
+++ gcc/cp/semantics.c	(working copy)
@@ -1321,7 +1321,15 @@
 	}
     }
   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)
+	warning (OPT_Wcatch_value,
+		 "catching non-reference type %qT", TREE_TYPE (decl));
+    }
   HANDLER_TYPE (handler) = type;
 }
 
===================================================================
 
2017-05-01  Volker Reichelt  <v.reichelt@netcologne.de>

	* g++.dg/warn/Wcatch-value-1.C: New test.

Index: gcc/testsuite/g++.dg/warn/Wcatch-value-1.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Wcatch-value-1.C	2017-05-01
+++ gcc/testsuite/g++.dg/warn/Wcatch-value-1.C	2017-05-01
@@ -0,0 +1,45 @@
+// { dg-options "-Wcatch-value" }
+
+struct A {};
+struct B {};
+struct C {};
+
+void foo()
+{
+  try {}
+  catch (int)      {}  // { dg-warning "catching non-reference type" }
+  catch (double*)  {}  // { dg-warning "catching non-reference type" }
+  catch (float&)   {}
+  catch (A)        {}  // { dg-warning "catching non-reference type" }
+  catch (A[2])     {}  // { dg-warning "catching non-reference type" }
+  catch (B*)       {}  // { dg-warning "catching non-reference type" }
+  catch (B&)       {}
+  catch (const C&) {}
+}
+
+template<typename T> void foo1()
+{
+  try {}
+  catch (T) {}
+}
+
+void bar1()
+{
+  foo1<int&>();
+  foo1<const A&>();
+}
+
+template<typename T> void foo2()
+{
+  try {}
+  catch (T) {}  // { dg-warning "catching non-reference type" }
+
+  try {}
+  catch (T&) {}
+}
+
+void bar2()
+{
+  foo2<int*>();      // { dg-message "required" }
+  foo2<A>();         // { dg-message "required" }
+}
===================================================================

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

[Martin Sebor]

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

>
> Bootstrapped and regtested on x86_64-pc-linux-gnu.
> OK for trunk?
>
> Regards,
> Volker
>
>
> 2017-05-01  Volker Reichelt  <v.reichelt@netcologne.de>
>
> 	* doc/invoke.texi (-Wcatch-value): Document new warning option.
>
> Index: gcc/doc/invoke.texi
> ===================================================================
> --- gcc/doc/invoke.texi	(revision 247416)
> +++ 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  -Wclobbered  -Wcomment  @gol
>  -Wconditionally-supported  @gol
>  -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
>  -Wdelete-incomplete @gol
> @@ -5827,6 +5827,11 @@
>  literals to @code{char *}.  This warning is enabled by default for C++
>  programs.
>
> +@item -Wcatch-value @r{(C++ and Objective-C++ only)}
> +@opindex Wcatch-value
> +@opindex Wno-catch-value
> +Warn about catch handler of non-reference type.
> +
>  @item -Wclobbered
>  @opindex Wclobbered
>  @opindex Wno-clobbered
> ===================================================================
>
> 2017-05-01  Volker Reichelt  <v.reichelt@netcologne.de>
>
> 	* c.opt (Wcatch-value): New C++ warning flag.
>
> Index: gcc/c-family/c.opt
> ===================================================================
> --- gcc/c-family/c.opt	(revision 247416)
> +++ 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
> +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-01  Volker Reichelt  <v.reichelt@netcologne.de>
>
> 	* semantics.c (finish_handler_parms): Warn about non-reference type
> 	catch handlers.
>
> Index: gcc/cp/semantics.c
> ===================================================================
> --- gcc/cp/semantics.c	(revision 247416)
> +++ gcc/cp/semantics.c	(working copy)
> @@ -1321,7 +1321,15 @@
>  	}
>      }
>    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)
> +	warning (OPT_Wcatch_value,
> +		 "catching non-reference type %qT", TREE_TYPE (decl));
> +    }
>    HANDLER_TYPE (handler) = type;
>  }
>
> ===================================================================
>
> 2017-05-01  Volker Reichelt  <v.reichelt@netcologne.de>
>
> 	* g++.dg/warn/Wcatch-value-1.C: New test.
>
> Index: gcc/testsuite/g++.dg/warn/Wcatch-value-1.C
> ===================================================================
> --- gcc/testsuite/g++.dg/warn/Wcatch-value-1.C	2017-05-01
> +++ gcc/testsuite/g++.dg/warn/Wcatch-value-1.C	2017-05-01
> @@ -0,0 +1,45 @@
> +// { dg-options "-Wcatch-value" }
> +
> +struct A {};
> +struct B {};
> +struct C {};
> +
> +void foo()
> +{
> +  try {}
> +  catch (int)      {}  // { dg-warning "catching non-reference type" }
> +  catch (double*)  {}  // { dg-warning "catching non-reference type" }
> +  catch (float&)   {}
> +  catch (A)        {}  // { dg-warning "catching non-reference type" }
> +  catch (A[2])     {}  // { dg-warning "catching non-reference type" }
> +  catch (B*)       {}  // { dg-warning "catching non-reference type" }
> +  catch (B&)       {}
> +  catch (const C&) {}
> +}
> +
> +template<typename T> void foo1()
> +{
> +  try {}
> +  catch (T) {}
> +}
> +
> +void bar1()
> +{
> +  foo1<int&>();
> +  foo1<const A&>();
> +}
> +
> +template<typename T> void foo2()
> +{
> +  try {}
> +  catch (T) {}  // { dg-warning "catching non-reference type" }
> +
> +  try {}
> +  catch (T&) {}
> +}
> +
> +void bar2()
> +{
> +  foo2<int*>();      // { dg-message "required" }
> +  foo2<A>();         // { dg-message "required" }
> +}
> ===================================================================
>

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

[Volker Reichelt]

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?

Regards,
Volker

>> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>> OK for trunk?
>>
>> Regards,
>> Volker
>>
>>
>> 2017-05-01  Volker Reichelt  <v.reichelt@netcologne.de>
>>
>> 	* doc/invoke.texi (-Wcatch-value): Document new warning option.
>>
>> Index: gcc/doc/invoke.texi
>> ===================================================================
>> --- gcc/doc/invoke.texi	(revision 247416)
>> +++ 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  -Wclobbered  -Wcomment  @gol
>>  -Wconditionally-supported  @gol
>>  -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
>>  -Wdelete-incomplete @gol
>> @@ -5827,6 +5827,11 @@
>>  literals to @code{char *}.  This warning is enabled by default for C++
>>  programs.
>>
>> +@item -Wcatch-value @r{(C++ and Objective-C++ only)}
>> +@opindex Wcatch-value
>> +@opindex Wno-catch-value
>> +Warn about catch handler of non-reference type.
>> +
>>  @item -Wclobbered
>>  @opindex Wclobbered
>>  @opindex Wno-clobbered
>> ===================================================================
>>
>> 2017-05-01  Volker Reichelt  <v.reichelt@netcologne.de>
>>
>> 	* c.opt (Wcatch-value): New C++ warning flag.
>>
>> Index: gcc/c-family/c.opt
>> ===================================================================
>> --- gcc/c-family/c.opt	(revision 247416)
>> +++ 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
>> +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-01  Volker Reichelt  <v.reichelt@netcologne.de>
>>
>> 	* semantics.c (finish_handler_parms): Warn about non-reference type
>> 	catch handlers.
>>
>> Index: gcc/cp/semantics.c
>> ===================================================================
>> --- gcc/cp/semantics.c	(revision 247416)
>> +++ gcc/cp/semantics.c	(working copy)
>> @@ -1321,7 +1321,15 @@
>>  	}
>>      }
>>    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)
>> +	warning (OPT_Wcatch_value,
>> +		 "catching non-reference type %qT", TREE_TYPE (decl));
>> +    }
>>    HANDLER_TYPE (handler) = type;
>>  }
>>
>> ===================================================================
>>
>> 2017-05-01  Volker Reichelt  <v.reichelt@netcologne.de>
>>
>> 	* g++.dg/warn/Wcatch-value-1.C: New test.
>>
>> Index: gcc/testsuite/g++.dg/warn/Wcatch-value-1.C
>> ===================================================================
>> --- gcc/testsuite/g++.dg/warn/Wcatch-value-1.C	2017-05-01
>> +++ gcc/testsuite/g++.dg/warn/Wcatch-value-1.C	2017-05-01
>> @@ -0,0 +1,45 @@
>> +// { dg-options "-Wcatch-value" }
>> +
>> +struct A {};
>> +struct B {};
>> +struct C {};
>> +
>> +void foo()
>> +{
>> +  try {}
>> +  catch (int)      {}  // { dg-warning "catching non-reference type" }
>> +  catch (double*)  {}  // { dg-warning "catching non-reference type" }
>> +  catch (float&)   {}
>> +  catch (A)        {}  // { dg-warning "catching non-reference type" }
>> +  catch (A[2])     {}  // { dg-warning "catching non-reference type" }
>> +  catch (B*)       {}  // { dg-warning "catching non-reference type" }
>> +  catch (B&)       {}
>> +  catch (const C&) {}
>> +}
>> +
>> +template<typename T> void foo1()
>> +{
>> +  try {}
>> +  catch (T) {}
>> +}
>> +
>> +void bar1()
>> +{
>> +  foo1<int&>();
>> +  foo1<const A&>();
>> +}
>> +
>> +template<typename T> void foo2()
>> +{
>> +  try {}
>> +  catch (T) {}  // { dg-warning "catching non-reference type" }
>> +
>> +  try {}
>> +  catch (T&) {}
>> +}
>> +
>> +void bar2()
>> +{
>> +  foo2<int*>();      // { dg-message "required" }
>> +  foo2<A>();         // { dg-message "required" }
>> +}
>> ===================================================================
>>
> 

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

[Martin Sebor]

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.

>
> Regards,
> Volker
>
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>>> OK for trunk?
>>>
>>> Regards,
>>> Volker
>>>
>>>
>>> 2017-05-01  Volker Reichelt  <v.reichelt@netcologne.de>
>>>
>>> 	* doc/invoke.texi (-Wcatch-value): Document new warning option.
>>>
>>> Index: gcc/doc/invoke.texi
>>> ===================================================================
>>> --- gcc/doc/invoke.texi	(revision 247416)
>>> +++ 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  -Wclobbered  -Wcomment  @gol
>>>  -Wconditionally-supported  @gol
>>>  -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
>>>  -Wdelete-incomplete @gol
>>> @@ -5827,6 +5827,11 @@
>>>  literals to @code{char *}.  This warning is enabled by default for C++
>>>  programs.
>>>
>>> +@item -Wcatch-value @r{(C++ and Objective-C++ only)}
>>> +@opindex Wcatch-value
>>> +@opindex Wno-catch-value
>>> +Warn about catch handler of non-reference type.
>>> +
>>>  @item -Wclobbered
>>>  @opindex Wclobbered
>>>  @opindex Wno-clobbered
>>> ===================================================================
>>>
>>> 2017-05-01  Volker Reichelt  <v.reichelt@netcologne.de>
>>>
>>> 	* c.opt (Wcatch-value): New C++ warning flag.
>>>
>>> Index: gcc/c-family/c.opt
>>> ===================================================================
>>> --- gcc/c-family/c.opt	(revision 247416)
>>> +++ 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
>>> +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-01  Volker Reichelt  <v.reichelt@netcologne.de>
>>>
>>> 	* semantics.c (finish_handler_parms): Warn about non-reference type
>>> 	catch handlers.
>>>
>>> Index: gcc/cp/semantics.c
>>> ===================================================================
>>> --- gcc/cp/semantics.c	(revision 247416)
>>> +++ gcc/cp/semantics.c	(working copy)
>>> @@ -1321,7 +1321,15 @@
>>>  	}
>>>      }
>>>    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)
>>> +	warning (OPT_Wcatch_value,
>>> +		 "catching non-reference type %qT", TREE_TYPE (decl));
>>> +    }
>>>    HANDLER_TYPE (handler) = type;
>>>  }
>>>
>>> ===================================================================
>>>
>>> 2017-05-01  Volker Reichelt  <v.reichelt@netcologne.de>
>>>
>>> 	* g++.dg/warn/Wcatch-value-1.C: New test.
>>>
>>> Index: gcc/testsuite/g++.dg/warn/Wcatch-value-1.C
>>> ===================================================================
>>> --- gcc/testsuite/g++.dg/warn/Wcatch-value-1.C	2017-05-01
>>> +++ gcc/testsuite/g++.dg/warn/Wcatch-value-1.C	2017-05-01
>>> @@ -0,0 +1,45 @@
>>> +// { dg-options "-Wcatch-value" }
>>> +
>>> +struct A {};
>>> +struct B {};
>>> +struct C {};
>>> +
>>> +void foo()
>>> +{
>>> +  try {}
>>> +  catch (int)      {}  // { dg-warning "catching non-reference type" }
>>> +  catch (double*)  {}  // { dg-warning "catching non-reference type" }
>>> +  catch (float&)   {}
>>> +  catch (A)        {}  // { dg-warning "catching non-reference type" }
>>> +  catch (A[2])     {}  // { dg-warning "catching non-reference type" }
>>> +  catch (B*)       {}  // { dg-warning "catching non-reference type" }
>>> +  catch (B&)       {}
>>> +  catch (const C&) {}
>>> +}
>>> +
>>> +template<typename T> void foo1()
>>> +{
>>> +  try {}
>>> +  catch (T) {}
>>> +}
>>> +
>>> +void bar1()
>>> +{
>>> +  foo1<int&>();
>>> +  foo1<const A&>();
>>> +}
>>> +
>>> +template<typename T> void foo2()
>>> +{
>>> +  try {}
>>> +  catch (T) {}  // { dg-warning "catching non-reference type" }
>>> +
>>> +  try {}
>>> +  catch (T&) {}
>>> +}
>>> +
>>> +void bar2()
>>> +{
>>> +  foo2<int*>();      // { dg-message "required" }
>>> +  foo2<A>();         // { dg-message "required" }
>>> +}
>>> ===================================================================
>>>
>>
>

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

[Jason Merrill via gcc-patches]

On Sun, May 7, 2017 at 8:05 PM, Martin Sebor <msebor@gmail.com> 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.

I agree with Martin's comments.

Jason

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

[Volker Reichelt]

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.

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

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

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

	* 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<typename T> void foo1()
+{
+  try {}
+  catch (T) {}  // { dg-warning "catching polymorphic type" }
+}
+
+template<typename T> void foo2()
+{
+  try {}
+  catch (T*) {}
+
+  try {}
+  catch (T&) {}
+
+  try {}
+  catch (const T&) {}
+}
+
+void bar()
+{
+  foo1<int&>();
+  foo1<const A&>();
+  foo1<B&>();
+  foo1<const C&>();
+  foo1<D&>();
+
+  foo1<int>();
+  foo1<A>();  // { dg-message "required" }
+  foo1<B>();  // { dg-message "required" }
+  foo1<C>();
+  foo1<D>();
+
+  foo2<int>();
+  foo2<A>();
+  foo2<B>();
+  foo2<C>();
+  foo2<D>();
+}
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<typename T> void foo1()
+{
+  try {}
+  catch (T) {}  // { dg-warning "catching" }
+}
+
+template<typename T> void foo2()
+{
+  try {}
+  catch (T*) {}
+
+  try {}
+  catch (T&) {}
+
+  try {}
+  catch (const T&) {}
+}
+
+void bar()
+{
+  foo1<int&>();
+  foo1<const A&>();
+  foo1<B&>();
+  foo1<const C&>();
+  foo1<D&>();
+
+  foo1<int>();
+  foo1<A>();  // { dg-message "required" }
+  foo1<B>();  // { dg-message "required" }
+  foo1<C>();  // { dg-message "required" }
+  foo1<D>();  // { dg-message "required" }
+
+  foo2<int>();
+  foo2<A>();
+  foo2<B>();
+  foo2<C>();
+  foo2<D>();
+}
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<typename T> void foo1()
+{
+  try {}
+  catch (T) {}  // { dg-warning "catching" }
+}
+
+template<typename T> void foo2()
+{
+  try {}
+  catch (T*) {}  // { dg-warning "catching non-reference type" }
+
+  try {}
+  catch (T&) {}
+
+  try {}
+  catch (const T&) {}
+}
+
+void bar()
+{
+  foo1<int&>();
+  foo1<const A&>();
+  foo1<B&>();
+  foo1<const C&>();
+  foo1<D&>();
+
+  foo1<int>();  // { dg-message "required" }
+  foo1<A>();    // { dg-message "required" }
+  foo1<B>();    // { dg-message "required" }
+  foo1<C>();    // { dg-message "required" }
+  foo1<D>();    // { dg-message "required" }
+
+  foo2<int>();  // { dg-message "required" }
+  foo2<A>();    // { dg-message "required" }
+  foo2<B>();    // { dg-message "required" }
+  foo2<C>();    // { dg-message "required" }
+  foo2<D>();    // { dg-message "required" }
+}
===================================================================

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

[Martin Sebor]

> 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.

It seems reasonable to me.  I'm not too fond of multi-level
warnings since few users take advantage of anything but the
default, but this case is simple and innocuous enough that
I don't think it can do harm.

>
> 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.

I can't think of a use case for polymorphic slicing that's not
harmful so unless there is a common one that escapes me, I'd say
-Wall.

What are your thoughts on enhancing the warning to also handle
the rethrow case?

Also, it seems that a similar warning would be useful even beyond
catch handlers, to help detect slicing when passing arguments to
functions by value.  Especially in code that mixes OOP with the
STL (or other template libraries).  Have you thought about tackling
that at some point as well?

Martin

>
> Regards,
> Volker
>
>
> 2017-05-13  Volker Reichelt  <v.reichelt@netcologne.de>
>
> 	* 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  <v.reichelt@netcologne.de>
>
> 	* 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  <v.reichelt@netcologne.de>
>
> 	* 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  <v.reichelt@netcologne.de>
>
> 	* 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<typename T> void foo1()
> +{
> +  try {}
> +  catch (T) {}  // { dg-warning "catching polymorphic type" }
> +}
> +
> +template<typename T> void foo2()
> +{
> +  try {}
> +  catch (T*) {}
> +
> +  try {}
> +  catch (T&) {}
> +
> +  try {}
> +  catch (const T&) {}
> +}
> +
> +void bar()
> +{
> +  foo1<int&>();
> +  foo1<const A&>();
> +  foo1<B&>();
> +  foo1<const C&>();
> +  foo1<D&>();
> +
> +  foo1<int>();
> +  foo1<A>();  // { dg-message "required" }
> +  foo1<B>();  // { dg-message "required" }
> +  foo1<C>();
> +  foo1<D>();
> +
> +  foo2<int>();
> +  foo2<A>();
> +  foo2<B>();
> +  foo2<C>();
> +  foo2<D>();
> +}
> 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<typename T> void foo1()
> +{
> +  try {}
> +  catch (T) {}  // { dg-warning "catching" }
> +}
> +
> +template<typename T> void foo2()
> +{
> +  try {}
> +  catch (T*) {}
> +
> +  try {}
> +  catch (T&) {}
> +
> +  try {}
> +  catch (const T&) {}
> +}
> +
> +void bar()
> +{
> +  foo1<int&>();
> +  foo1<const A&>();
> +  foo1<B&>();
> +  foo1<const C&>();
> +  foo1<D&>();
> +
> +  foo1<int>();
> +  foo1<A>();  // { dg-message "required" }
> +  foo1<B>();  // { dg-message "required" }
> +  foo1<C>();  // { dg-message "required" }
> +  foo1<D>();  // { dg-message "required" }
> +
> +  foo2<int>();
> +  foo2<A>();
> +  foo2<B>();
> +  foo2<C>();
> +  foo2<D>();
> +}
> 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<typename T> void foo1()
> +{
> +  try {}
> +  catch (T) {}  // { dg-warning "catching" }
> +}
> +
> +template<typename T> void foo2()
> +{
> +  try {}
> +  catch (T*) {}  // { dg-warning "catching non-reference type" }
> +
> +  try {}
> +  catch (T&) {}
> +
> +  try {}
> +  catch (const T&) {}
> +}
> +
> +void bar()
> +{
> +  foo1<int&>();
> +  foo1<const A&>();
> +  foo1<B&>();
> +  foo1<const C&>();
> +  foo1<D&>();
> +
> +  foo1<int>();  // { dg-message "required" }
> +  foo1<A>();    // { dg-message "required" }
> +  foo1<B>();    // { dg-message "required" }
> +  foo1<C>();    // { dg-message "required" }
> +  foo1<D>();    // { dg-message "required" }
> +
> +  foo2<int>();  // { dg-message "required" }
> +  foo2<A>();    // { dg-message "required" }
> +  foo2<B>();    // { dg-message "required" }
> +  foo2<C>();    // { dg-message "required" }
> +  foo2<D>();    // { dg-message "required" }
> +}
> ===================================================================
>

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

[Jason Merrill]

On Mon, May 15, 2017 at 3:58 PM, Martin Sebor <msebor@gmail.com> wrote:
>> 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.
>
> It seems reasonable to me.  I'm not too fond of multi-level
> warnings since few users take advantage of anything but the
> default, but this case is simple and innocuous enough that
> I don't think it can do harm.

>> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>> OK for trunk?

OK.

>> 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.
>
> I can't think of a use case for polymorphic slicing that's not
> harmful so unless there is a common one that escapes me, I'd say
> -Wall.

Agreed.  But then you'll probably want to allow -Wno-catch-value to turn it off.

Jason

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

[Volker Reichelt]

On 24 May, Jason Merrill wrote:
> On Mon, May 15, 2017 at 3:58 PM, Martin Sebor <msebor@gmail.com> wrote:
>>> 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.
>>
>> It seems reasonable to me.  I'm not too fond of multi-level
>> warnings since few users take advantage of anything but the
>> default, but this case is simple and innocuous enough that
>> I don't think it can do harm.
> 
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>>> OK for trunk?
> 
> OK.

Committed.

>>> 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.
>>
>> I can't think of a use case for polymorphic slicing that's not
>> harmful so unless there is a common one that escapes me, I'd say
>> -Wall.
> 
> Agreed.  But then you'll probably want to allow -Wno-catch-value to turn it off.
> 
> Jason

So how about the following then?
Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Regards,
Volker

2017-05-30  Volker Reichelt  <v.reichelt@netcologne.de>

	* doc/invoke.texi (-Wcatch-value): Document new shortcut.
	Add to -Wall section.

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 248585)
+++ gcc/doc/invoke.texi	(working copy)
@@ -265,8 +265,8 @@
 -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  -Wcatch-value=@var{n}  -Wclobbered  -Wcomment  @gol
--Wconditionally-supported  @gol
+-Wchar-subscripts  -Wchkp  -Wcatch-value  -Wcatch-value=@var{n} @gol
+-Wclobbered  -Wcomment  -Wconditionally-supported @gol
 -Wconversion  -Wcoverage-mismatch  -Wno-cpp  -Wdangling-else  -Wdate-time @gol
 -Wdelete-incomplete @gol
 -Wno-deprecated  -Wno-deprecated-declarations  -Wno-designated-init @gol
@@ -3757,6 +3757,7 @@
 -Wbool-compare  @gol
 -Wbool-operation  @gol
 -Wc++11-compat  -Wc++14-compat  @gol
+-Wcatch-value @r{(C++ and Objective-C++ only)}  @gol
 -Wchar-subscripts  @gol
 -Wcomment  @gol
 -Wduplicate-decl-specifier @r{(C and Objective-C only)} @gol
@@ -5834,13 +5835,16 @@
 literals to @code{char *}.  This warning is enabled by default for C++
 programs.
 
-@item -Wcatch-value=@var{n} @r{(C++ and Objective-C++ only)}
+@item -Wcatch-value
+@itemx -Wcatch-value=@var{n} @r{(C++ and Objective-C++ only)}
 @opindex Wcatch-value
+@opindex Wno-catch-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.
+With @option{-Wcatch-value=1} (or @option{-Wcatch-value} for short)
+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. @option{-Wcatch-value} is enabled by @option{-Wall}.
 
 @item -Wclobbered
 @opindex Wclobbered
===================================================================

2017-05-30  Volker Reichelt  <v.reichelt@netcologne.de>

	* c.opt (Wcatch-value): New shortcut for Wcatch-value=1.
	(Wcatch-value=1): Enable by -Wall.

Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 248585)
+++ gcc/c-family/c.opt	(working copy)
@@ -388,8 +388,12 @@
 C ObjC C++ ObjC++ Var(warn_cast_qual) Warning
 Warn about casts which discard qualifiers.
 
+Wcatch-value
+C++ ObjC++ Warning Alias(Wcatch-value=, 1, 0)
+Warn about catch handlers of non-reference type.
+
 Wcatch-value=
-C++ ObjC++ Var(warn_catch_value) Warning Joined RejectNegative UInteger
+C++ ObjC++ Var(warn_catch_value) Warning Joined RejectNegative UInteger LangEnabledBy(C++ ObjC++,Wall, 1, 0)
 Warn about catch handlers of non-reference type.
 
 Wchar-subscripts
===================================================================

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

[Jason Merrill]

On Tue, May 30, 2017 at 2:14 AM, Volker Reichelt
<v.reichelt@netcologne.de> wrote:
> On 24 May, Jason Merrill wrote:
>> On Mon, May 15, 2017 at 3:58 PM, Martin Sebor <msebor@gmail.com> wrote:
>>>> 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.
>>>
>>> It seems reasonable to me.  I'm not too fond of multi-level
>>> warnings since few users take advantage of anything but the
>>> default, but this case is simple and innocuous enough that
>>> I don't think it can do harm.
>>
>>>> Bootstrapped and regtested on x86_64-pc-linux-gnu.
>>>> OK for trunk?
>>
>> OK.
>
> Committed.
>
>>>> 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.
>>>
>>> I can't think of a use case for polymorphic slicing that's not
>>> harmful so unless there is a common one that escapes me, I'd say
>>> -Wall.
>>
>> Agreed.  But then you'll probably want to allow -Wno-catch-value to turn it off.
>
> So how about the following then?
> Bootstrapped and regtested on x86_64-pc-linux-gnu.
> OK for trunk?

OK, thanks.

Jason

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

[Volker Reichelt]

On 15 May, Martin Sebor wrote:
>> 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.
> 
> It seems reasonable to me.  I'm not too fond of multi-level
> warnings since few users take advantage of anything but the
> default, but this case is simple and innocuous enough that
> I don't think it can do harm.
> 
>>
>> 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.
> 
> I can't think of a use case for polymorphic slicing that's not
> harmful so unless there is a common one that escapes me, I'd say
> -Wall.

So that's what I committed after Jason's OK.

> What are your thoughts on enhancing the warning to also handle
> the rethrow case?
> 
> Also, it seems that a similar warning would be useful even beyond
> catch handlers, to help detect slicing when passing arguments to
> functions by value.  Especially in code that mixes OOP with the
> STL (or other template libraries).  Have you thought about tackling
> that at some point as well?

I don't have any plans to handle handle the rethrow case.

A general slicing warning would be very nice to have. Actually
clang-tidy has one (which is a little buggy, though).
Implementing this is over my head, though.
I'd rather stick with something less ambitious.

> Martin
> 
>>
>> Regards,
>> Volker

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

[Jason Merrill]

On Mon, Jun 5, 2017 at 6:55 AM, Volker Reichelt
<v.reichelt@netcologne.de> wrote:
> On 15 May, Martin Sebor wrote:
>>> 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.
>>
>> It seems reasonable to me.  I'm not too fond of multi-level
>> warnings since few users take advantage of anything but the
>> default, but this case is simple and innocuous enough that
>> I don't think it can do harm.
>>
>>>
>>> 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.
>>
>> I can't think of a use case for polymorphic slicing that's not
>> harmful so unless there is a common one that escapes me, I'd say
>> -Wall.
>
> So that's what I committed after Jason's OK.
>
>> What are your thoughts on enhancing the warning to also handle
>> the rethrow case?
>>
>> Also, it seems that a similar warning would be useful even beyond
>> catch handlers, to help detect slicing when passing arguments to
>> functions by value.  Especially in code that mixes OOP with the
>> STL (or other template libraries).  Have you thought about tackling
>> that at some point as well?
>
> I don't have any plans to handle handle the rethrow case.
>
> A general slicing warning would be very nice to have. Actually
> clang-tidy has one (which is a little buggy, though).
> Implementing this is over my head, though.
> I'd rather stick with something less ambitious.

FWIW I think it would be pretty straightforward to do this in
convert_like_real, under the ck_base case.

Jason