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