Re: [C++ PATCH] Sanity check __cxa_* user declarations (PR c++/88482)
On 12/15/18 8:08 AM, Jakub Jelinek wrote: On Sat, Dec 15, 2018 at 12:22:44PM +0100, Jakub Jelinek wrote: Fix libitm. Ok, will do. Here it is in patch form: OK, thanks. Jason
Re: [C++ PATCH] Sanity check __cxa_* user declarations (PR c++/88482)
On Sat, Dec 15, 2018 at 12:22:44PM +0100, Jakub Jelinek wrote:
> > Fix libitm.
>
> Ok, will do.
Hre it is in patch form:
2018-12-15 Jakub Jelinek
PR c++/88482
* except.c (verify_library_fn): New function.
(declare_library_fn): Use it. Initialize TM even if the non-TM
library function has been user declared.
(do_end_catch): Don't set TREE_NOTHROW on error_mark_node.
(expand_start_catch_block): Don't call initialize_handler_parm
for error_mark_node.
(build_throw): Use verify_library_fn. Initialize TM even if the
non-TM library function has been user declared. Don't crash if
any library fn is error_mark_node.
* g++.dg/eh/builtin5.C: New test.
* g++.dg/eh/builtin6.C: New test.
* g++.dg/eh/builtin7.C: New test.
* g++.dg/eh/builtin8.C: New test.
* g++.dg/eh/builtin9.C: New test.
* g++.dg/eh/builtin10.C: New test.
* g++.dg/eh/builtin11.C: New test.
* g++.dg/parse/crash55.C: Adjust expected diagnostics.
* eh_cpp.cc (__cxa_throw): Change DEST argument type from
void * to void (*) (void *).
(_ITM_cxa_throw): Likewise.
* libitm.h (_ITM_cxa_throw): Likewise.
* libitm.texi (_ITM_cxa_throw): Likewise.
--- gcc/cp/except.c.jj 2018-12-14 00:32:56.042200543 +0100
+++ gcc/cp/except.c 2018-12-15 13:39:29.301459729 +0100
@@ -132,6 +132,49 @@ build_exc_ptr (void)
1, integer_zero_node);
}
+/* Check that user declared function FN is a function and has return
+ type RTYPE and argument types ARG{1,2,3}TYPE. */
+
+static bool
+verify_library_fn (tree fn, const char *name, tree rtype,
+ tree arg1type, tree arg2type, tree arg3type)
+{
+ if (TREE_CODE (fn) != FUNCTION_DECL
+ || TREE_CODE (TREE_TYPE (fn)) != FUNCTION_TYPE)
+{
+ bad:
+ error_at (DECL_SOURCE_LOCATION (fn), "%qs declared incorrectly", name);
+ return false;
+}
+ tree fntype = TREE_TYPE (fn);
+ if (!same_type_p (TREE_TYPE (fntype), rtype))
+goto bad;
+ tree targs = TYPE_ARG_TYPES (fntype);
+ tree args[3] = { arg1type, arg2type, arg3type };
+ for (int i = 0; i < 3 && args[i]; i++)
+{
+ if (targs == NULL_TREE)
+ goto bad;
+ if (!same_type_p (TREE_VALUE (targs), args[i]))
+ {
+ if (i == 0)
+ goto bad;
+ /* Be less strict for second and following arguments, __cxa_throw
+needs to be more permissive. */
+ if (TYPE_PTROBV_P (TREE_VALUE (targs)) && TYPE_PTROBV_P (args[i]))
+ /* Both object pointers. */;
+ else if (TYPE_PTRFN_P (TREE_VALUE (targs)) && TYPE_PTRFN_P (args[i]))
+ /* Both function pointers. */;
+ else
+ goto bad;
+ }
+ targs = TREE_CHAIN (targs);
+}
+ if (targs != void_list_node)
+goto bad;
+ return true;
+}
+
/* Find or declare a function NAME, returning RTYPE, taking a single
parameter PTYPE, with an empty exception specification. ECF are the
library fn flags. If TM_ECF is non-zero, also find or create a
@@ -148,21 +191,39 @@ declare_library_fn (const char *name, tr
{
tree ident = get_identifier (name);
tree res = get_global_binding (ident);
+ tree fntype = NULL_TREE;
+ tree except = NULL_TREE;
if (!res)
{
- tree type = build_function_type_list (rtype, ptype, NULL_TREE);
- tree except = ecf & ECF_NOTHROW ? empty_except_spec : NULL_TREE;
- res = push_library_fn (ident, type, except, ecf);
- if (tm_ecf && flag_tm)
+ fntype = build_function_type_list (rtype, ptype, NULL_TREE);
+ if (ecf & ECF_NOTHROW)
+ except = empty_except_spec;
+ res = push_library_fn (ident, fntype, except, ecf);
+}
+ else if (!verify_library_fn (res, name, rtype, ptype, NULL_TREE, NULL_TREE))
+return error_mark_node;
+
+ if (tm_ecf && flag_tm)
+{
+ char *tm_name = concat ("_ITM_", name + 2, NULL_TREE);
+ tree tm_ident = get_identifier (tm_name);
+ tree tm_fn = get_global_binding (tm_ident);
+ if (!tm_fn)
{
- char *tm_name = concat ("_ITM_", name + 2, NULL_TREE);
- tree tm_ident = get_identifier (tm_name);
- free (tm_name);
- tree tm_fn = get_global_binding (tm_ident);
- if (!tm_fn)
- tm_fn = push_library_fn (tm_ident, type, except, ecf | tm_ecf);
- record_tm_replacement (res, tm_fn);
+ if (!fntype)
+ {
+ fntype = build_function_type_list (rtype, ptype, NULL_TREE);
+ if (ecf & ECF_NOTHROW)
+ except = empty_except_spec;
+ }
+ tm_fn = push_library_fn (tm_ident, fntype, except, ecf | tm_ecf);
}
+ else if (!verify_library_fn (tm_fn, tm_name, rtype, ptype,
+ NULL_TREE, NULL_TREE))
+ tm_fn = error_mark_node;
+ free (tm_name);
+ if (tm_fn != error_mark_node)
+ record_tm_replacement
Re: [C++ PATCH] Sanity check __cxa_* user declarations (PR c++/88482)
On Fri, Dec 14, 2018 at 04:59:44PM -0500, Jason Merrill wrote:
> > extern void __cxa_throw (void *, void *, void *) WEAK;
> > void
> > _ITM_cxa_throw (void *obj, void *tinfo, void *dest)
> > {
> >// This used to be instrumented, but does not need to be anymore.
> >__cxa_throw (obj, tinfo, dest);
> > }
> > Shall we fix libitm to use void (*dest) (void *) instead of void *dest,
> > or shall I make the verify_library_fn handle both i == 1 and i == 2
> > the same?
>
> Fix libitm.
Ok, will do.
> Do function pointers and object pointers have the same
> representation on all the targets we support?
Not sure about this, but we have all kinds of weird targets.
> > +/* Check that user declared function FN is a function and has return
> > + type RTYPE and argument types ARG{1,2,3}TYPE. */
> > +
> > +static bool
> > +verify_library_fn (tree fn, const char *name, tree rtype,
> > + tree arg1type, tree arg2type, tree arg3type)
> > +{
>
> Do we want to skip all of this if DECL_ARTIFICIAL?
Not sure how a DECL_ARTIFICIAL fn could appear there. This function is
only called 1) the first time we need a particular __cxa_* routine
2) and the declaration exists already (when we create it ourselves,
we don't need to check it).
> > + /* Be less strict for __cxa_throw last 2 arguments. */
> > + if (i == 1 && TREE_CODE (TREE_VALUE (targs)) != POINTER_TYPE)
> > + goto bad;
> > + if (i == 2
> > + && (TREE_CODE (TREE_VALUE (targs)) != POINTER_TYPE
> > + || (TREE_CODE (TREE_TYPE (TREE_VALUE (targs)))
> > + != FUNCTION_TYPE)))
>
> These seem to assume that any library function with more than one parameter
> will have pointers for the second and third parameters.
>
> TYPE_PTROBV_P might be useful.
I can do e.g.
if (i == 0)
goto bad;
/* Be less strict for second and following arguments, __cxa_throw
needs to be more permissive. */
if (TYPE_PTROBV_P (TREE_VALUE (args)) && TYPE_PTROBV_P (args[i]))
/* Both object pointers. */;
else if (TYPE_PTRFN_P (TREE_VALUE (args)) && TYPE_PTRFN_P (args[i]))
/* Both function pointers. */;
else
goto bad;
> > @@ -625,10 +677,22 @@ build_throw (tree exp)
> > tree itm_fn = get_global_binding (itm_name);
> > if (!itm_fn)
> > itm_fn = push_throw_library_fn (itm_name, tmp);
> > - apply_tm_attr (itm_fn, get_identifier ("transaction_pure"));
> > - record_tm_replacement (throw_fn, itm_fn);
> > + else if (!verify_library_fn (itm_fn, "_ITM_cxa_throw",
> > + void_type_node, ptr_type_node,
> > + ptr_type_node, cleanup_type))
> > + itm_fn = error_mark_node;
> > + if (itm_fn != error_mark_node)
> > + {
> > + apply_tm_attr (itm_fn,
> > +get_identifier ("transaction_pure"));
> > + record_tm_replacement (throw_fn, itm_fn);
> > + }
> > }
> > }
> > + else if (!verify_library_fn (throw_fn, "__cxa_throw", void_type_node,
> > + ptr_type_node, ptr_type_node,
> > + cleanup_type))
> > + throw_fn = error_mark_node;
>
> This should happen whether or not flag_tm.
It does, it is else of:
if (!throw_fn)
{
tree name = get_identifier ("__cxa_throw");
throw_fn = get_global_binding (name);
if (!throw_fn)
{
That said, I think what is wrong is that we have the TM handling solely
in the spots where we create the __cxa_ functions ourselves. In all the
places, it looks like:
tree ident = get_identifier (name);
tree res = get_global_binding (ident);
if (!res)
{
res = build_it_ourselves;
if (tm_ecf && flag_tm)
{
char *tm_name = ...;
tree tm_ident = get_identifier (tm_name);
free (tm_name);
tree tm_fn = get_global_binding (tm_ident);
if (!tm_fn)
tm_fn = build_it_ourselves;
else
verify;
if (valid)
record_tm_replacement (res, tm_fn);
}
}
else
verify;
IMHO it should look like:
tree ident = get_identifier (name);
tree res = get_global_binding (ident);
if (!res)
res = build_it_ourselves;
else
verify;
if (valid && tm_ecf && flag_tm)
{
char *tm_name = ...;
tree tm_ident = get_identifier (tm_name);
tree tm_fn = get_global_binding (tm_ident);
if (!tm_fn)
tm_fn = build_it_ourselves;
else
verify;
free (tm_name);
if (valid)
record_tm_replacement (res, tm_fn);
}
So that if we e.g. have the __cxa_* prototypes, but not the TM ones,
we use user's prototypes for the former and create the latter.
Are you ok with
Re: [C++ PATCH] Sanity check __cxa_* user declarations (PR c++/88482)
On 12/13/18 6:29 PM, Jakub Jelinek wrote:
Hi!
If the user provides his own __cxa_* prototypes and does so incorrectly
(or even worse declares them as variables etc.), we can get various ICEs.
The following patch adds some sanity checking, mainly that they are actually
functions and with a compatible return type and argument type(s).
For __cxa_throw it gives some freedom for the second and third arguments,
but apparently not enough for libitm where it causes
+FAIL: libitm.c++/throwdown.C (test for excess errors)
This is because cxxabi.h has:
void
__cxa_throw(void*, std::type_info*, void (_GLIBCXX_CDTOR_CALLABI *) (void *))
__attribute__((__noreturn__));
and the patch checks that the second argument is a pointer (any kind) and
third parameter is a function pointer, but libitm.h has:
extern void _ITM_cxa_throw (void *obj, void *tinfo, void *dest);
and eh_cpp.cc has:
extern void __cxa_throw (void *, void *, void *) WEAK;
void
_ITM_cxa_throw (void *obj, void *tinfo, void *dest)
{
// This used to be instrumented, but does not need to be anymore.
__cxa_throw (obj, tinfo, dest);
}
Shall we fix libitm to use void (*dest) (void *) instead of void *dest,
or shall I make the verify_library_fn handle both i == 1 and i == 2
the same?
Fix libitm. Do function pointers and object pointers have the same
representation on all the targets we support?
Bootstrapped/regtested on x86_64-linux and i686-linux (with the above
mentioned FAIL), ok for trunk (and with what solution for libitm)?
2018-12-13 Jakub Jelinek
PR c++/88482
* except.c (verify_library_fn): New function.
(declare_library_fn): Use it.
(do_end_catch): Don't set TREE_NOTHROW on error_mark_node.
(expand_start_catch_block): Don't call initialize_handler_parm
for error_mark_node.
(build_throw): Use verify_library_fn. Don't crash if
any library fn is error_mark_node.
* g++.dg/eh/builtin5.C: New test.
* g++.dg/eh/builtin6.C: New test.
* g++.dg/eh/builtin7.C: New test.
* g++.dg/eh/builtin8.C: New test.
* g++.dg/eh/builtin9.C: New test.
* g++.dg/eh/builtin10.C: New test.
* g++.dg/eh/builtin11.C: New test.
* g++.dg/parse/crash55.C: Adjust expected diagnostics.
--- gcc/cp/except.c.jj 2018-12-07 00:23:15.008998854 +0100
+++ gcc/cp/except.c 2018-12-13 20:05:23.053122023 +0100
@@ -132,6 +132,49 @@ build_exc_ptr (void)
1, integer_zero_node);
}
+/* Check that user declared function FN is a function and has return
+ type RTYPE and argument types ARG{1,2,3}TYPE. */
+
+static bool
+verify_library_fn (tree fn, const char *name, tree rtype,
+ tree arg1type, tree arg2type, tree arg3type)
+{
Do we want to skip all of this if DECL_ARTIFICIAL?
+ if (TREE_CODE (fn) != FUNCTION_DECL
+ || TREE_CODE (TREE_TYPE (fn)) != FUNCTION_TYPE)
+{
+ bad:
+ error_at (DECL_SOURCE_LOCATION (fn), "%qs declared incorrectly", name);
+ return false;
+}
+ tree fntype = TREE_TYPE (fn);
+ if (!same_type_p (TREE_TYPE (fntype), rtype))
+goto bad;
+ tree targs = TYPE_ARG_TYPES (fntype);
+ tree args[3] = { arg1type, arg2type, arg3type };
+ for (int i = 0; i < 3 && args[i]; i++)
+{
+ if (targs == NULL_TREE)
+ goto bad;
+ if (!same_type_p (TREE_VALUE (targs), args[i]))
+ {
+ if (i == 0)
+ goto bad;
+ /* Be less strict for __cxa_throw last 2 arguments. */
+ if (i == 1 && TREE_CODE (TREE_VALUE (targs)) != POINTER_TYPE)
+ goto bad;
+ if (i == 2
+ && (TREE_CODE (TREE_VALUE (targs)) != POINTER_TYPE
+ || (TREE_CODE (TREE_TYPE (TREE_VALUE (targs)))
+ != FUNCTION_TYPE)))
These seem to assume that any library function with more than one
parameter will have pointers for the second and third parameters.
TYPE_PTROBV_P might be useful.
+ goto bad;
+ }
+ targs = TREE_CHAIN (targs);
+}
+ if (targs != void_list_node)
+goto bad;
+ return true;
+}
+
/* Find or declare a function NAME, returning RTYPE, taking a single
parameter PTYPE, with an empty exception specification. ECF are the
library fn flags. If TM_ECF is non-zero, also find or create a
@@ -161,9 +204,16 @@ declare_library_fn (const char *name, tr
tree tm_fn = get_global_binding (tm_ident);
if (!tm_fn)
tm_fn = push_library_fn (tm_ident, type, except, ecf | tm_ecf);
- record_tm_replacement (res, tm_fn);
+ else if (!verify_library_fn (tm_fn, tm_name, rtype, ptype,
+ NULL_TREE, NULL_TREE))
+ tm_fn = error_mark_node;
+ if (tm_fn != error_mark_node)
+ record_tm_replacement (res, tm_fn);
}
}
+ else if (!verify_library_fn (res, name, rtype, ptype, NULL_TREE, NULL_TREE))
+return error_mark_node;
+
return res;
}
@@ -236,7
[C++ PATCH] Sanity check __cxa_* user declarations (PR c++/88482)
Hi!
If the user provides his own __cxa_* prototypes and does so incorrectly
(or even worse declares them as variables etc.), we can get various ICEs.
The following patch adds some sanity checking, mainly that they are actually
functions and with a compatible return type and argument type(s).
For __cxa_throw it gives some freedom for the second and third arguments,
but apparently not enough for libitm where it causes
+FAIL: libitm.c++/throwdown.C (test for excess errors)
This is because cxxabi.h has:
void
__cxa_throw(void*, std::type_info*, void (_GLIBCXX_CDTOR_CALLABI *) (void *))
__attribute__((__noreturn__));
and the patch checks that the second argument is a pointer (any kind) and
third parameter is a function pointer, but libitm.h has:
extern void _ITM_cxa_throw (void *obj, void *tinfo, void *dest);
and eh_cpp.cc has:
extern void __cxa_throw (void *, void *, void *) WEAK;
void
_ITM_cxa_throw (void *obj, void *tinfo, void *dest)
{
// This used to be instrumented, but does not need to be anymore.
__cxa_throw (obj, tinfo, dest);
}
Shall we fix libitm to use void (*dest) (void *) instead of void *dest,
or shall I make the verify_library_fn handle both i == 1 and i == 2
the same?
Bootstrapped/regtested on x86_64-linux and i686-linux (with the above
mentioned FAIL), ok for trunk (and with what solution for libitm)?
2018-12-13 Jakub Jelinek
PR c++/88482
* except.c (verify_library_fn): New function.
(declare_library_fn): Use it.
(do_end_catch): Don't set TREE_NOTHROW on error_mark_node.
(expand_start_catch_block): Don't call initialize_handler_parm
for error_mark_node.
(build_throw): Use verify_library_fn. Don't crash if
any library fn is error_mark_node.
* g++.dg/eh/builtin5.C: New test.
* g++.dg/eh/builtin6.C: New test.
* g++.dg/eh/builtin7.C: New test.
* g++.dg/eh/builtin8.C: New test.
* g++.dg/eh/builtin9.C: New test.
* g++.dg/eh/builtin10.C: New test.
* g++.dg/eh/builtin11.C: New test.
* g++.dg/parse/crash55.C: Adjust expected diagnostics.
--- gcc/cp/except.c.jj 2018-12-07 00:23:15.008998854 +0100
+++ gcc/cp/except.c 2018-12-13 20:05:23.053122023 +0100
@@ -132,6 +132,49 @@ build_exc_ptr (void)
1, integer_zero_node);
}
+/* Check that user declared function FN is a function and has return
+ type RTYPE and argument types ARG{1,2,3}TYPE. */
+
+static bool
+verify_library_fn (tree fn, const char *name, tree rtype,
+ tree arg1type, tree arg2type, tree arg3type)
+{
+ if (TREE_CODE (fn) != FUNCTION_DECL
+ || TREE_CODE (TREE_TYPE (fn)) != FUNCTION_TYPE)
+{
+ bad:
+ error_at (DECL_SOURCE_LOCATION (fn), "%qs declared incorrectly", name);
+ return false;
+}
+ tree fntype = TREE_TYPE (fn);
+ if (!same_type_p (TREE_TYPE (fntype), rtype))
+goto bad;
+ tree targs = TYPE_ARG_TYPES (fntype);
+ tree args[3] = { arg1type, arg2type, arg3type };
+ for (int i = 0; i < 3 && args[i]; i++)
+{
+ if (targs == NULL_TREE)
+ goto bad;
+ if (!same_type_p (TREE_VALUE (targs), args[i]))
+ {
+ if (i == 0)
+ goto bad;
+ /* Be less strict for __cxa_throw last 2 arguments. */
+ if (i == 1 && TREE_CODE (TREE_VALUE (targs)) != POINTER_TYPE)
+ goto bad;
+ if (i == 2
+ && (TREE_CODE (TREE_VALUE (targs)) != POINTER_TYPE
+ || (TREE_CODE (TREE_TYPE (TREE_VALUE (targs)))
+ != FUNCTION_TYPE)))
+ goto bad;
+ }
+ targs = TREE_CHAIN (targs);
+}
+ if (targs != void_list_node)
+goto bad;
+ return true;
+}
+
/* Find or declare a function NAME, returning RTYPE, taking a single
parameter PTYPE, with an empty exception specification. ECF are the
library fn flags. If TM_ECF is non-zero, also find or create a
@@ -161,9 +204,16 @@ declare_library_fn (const char *name, tr
tree tm_fn = get_global_binding (tm_ident);
if (!tm_fn)
tm_fn = push_library_fn (tm_ident, type, except, ecf | tm_ecf);
- record_tm_replacement (res, tm_fn);
+ else if (!verify_library_fn (tm_fn, tm_name, rtype, ptype,
+ NULL_TREE, NULL_TREE))
+ tm_fn = error_mark_node;
+ if (tm_fn != error_mark_node)
+ record_tm_replacement (res, tm_fn);
}
}
+ else if (!verify_library_fn (res, name, rtype, ptype, NULL_TREE, NULL_TREE))
+return error_mark_node;
+
return res;
}
@@ -236,7 +286,8 @@ do_end_catch (tree type)
tree cleanup = cp_build_function_call_vec (end_catch_fn,
NULL, tf_warning_or_error);
- TREE_NOTHROW (cleanup) = dtor_nothrow (type);
+ if (cleanup != error_mark_node)
+TREE_NOTHROW (cleanup) = dtor_nothrow (type);
return cleanup;
}
@@ -400,7 +451,8 @@ expand_start_catch_block (tree decl)
&&
