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

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

  reply	other threads:[~2017-05-08  0:05 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 [this message]
2017-05-08 13:14       ` Jason Merrill via gcc-patches
2017-05-14 15:30       ` Volker Reichelt
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=c5a2c640-1ac3-ec0b-0215-e2c2d270b25d@gmail.com \
    --to=msebor@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=v.reichelt@netcologne.de \
    /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).