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 those changes?
> Let's avoid repeating the strings "__cxa_throw" and "_ITM_cxa_throw".
Will do.
Jakub
