public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Volker Reichelt <v.reichelt@netcologne.de>
To: Martin Sebor <msebor@gmail.com>, gcc-patches@gcc.gnu.org
Subject: Re: {PATCH] New C++ warning -Wcatch-value
Date: Sun, 14 May 2017 15:30:00 -0000	[thread overview]
Message-ID: <tkrat.8c7b4260a533be2f@netcologne.de> (raw)
In-Reply-To: <c5a2c640-1ac3-ec0b-0215-e2c2d270b25d@gmail.com>

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" }
+}
===================================================================

  parent reply	other threads:[~2017-05-14 11:12 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-05-01  8:38 Volker Reichelt
2017-05-03  3:34 ` Martin Sebor
2017-05-07 20:28   ` Volker Reichelt
2017-05-08  3:18     ` Martin Sebor
2017-05-08 13:14       ` Jason Merrill via gcc-patches
2017-05-14 15:30       ` Volker Reichelt [this message]
2017-05-15 20:03         ` Martin Sebor
2017-05-24 20:13           ` Jason Merrill
2017-05-30  6:22             ` Volker Reichelt
2017-05-31 20:55               ` Jason Merrill
2017-06-05 10:55           ` Volker Reichelt
2017-06-05 19:08             ` Jason Merrill

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=tkrat.8c7b4260a533be2f@netcologne.de \
    --to=v.reichelt@netcologne.de \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=msebor@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).