This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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 those changes?

> Let's avoid repeating the strings "__cxa_throw" and "_ITM_cxa_throw".

Will do.

	Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]