Patchwork gnu-tm: Dont allow assigning transaction_unsafe functions to transaction_safe function pointers (issue6198054)

login
register
mail settings
Submitter Dave Boutcher
Date May 8, 2012, 11:02 p.m.
Message ID <20120508230202.F3FD2DC03AD@sandy>
Download mbox | patch
Permalink /patch/157831/
State New
Headers show

Comments

Dave Boutcher - May 8, 2012, 11:02 p.m.
Without this patch it is perfectly fine to assign non-transaction_safe
functions to function pointers marked as transaction_safe.  Unpleasantness
happens at run time.

e.g. 

 __attribute__((transaction_safe)) long (*compare)(int, int); 

compare = my_funky_random_function;


 gcc/c-typeck.c |    7 +++++++
 1 file changed, 7 insertions(+)
Torvald Riegel - May 15, 2012, 4:23 p.m.
On Tue, 2012-05-08 at 18:02 -0500, Dave Boutcher wrote:
> Without this patch it is perfectly fine to assign non-transaction_safe
> functions to function pointers marked as transaction_safe. Unpleasantness
> happens at run time.
> 
> e.g. 
> 
>  __attribute__((transaction_safe)) long (*compare)(int, int); 
> 
> compare = my_funky_random_function;
> 
> 
>  gcc/c-typeck.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/gcc/c-typeck.c b/gcc/c-typeck.c
> index fc01a79..69687d6 100644
> --- a/gcc/c-typeck.c
> +++ b/gcc/c-typeck.c
> @@ -5608,6 +5608,13 @@ convert_for_assignment (location_t location, tree type, tree rhs,
>  	  }
>  	}
>  
> +      /* Check for assignment to transaction safe */
> +      if (is_tm_safe(type) && !is_tm_safe_or_pure (rhs)) {

I don't think that assigning a tm_pure function to tm_safe works.  There
will be no instrumented version of it.  I don't think we generate an
alias or sth like that yet.

When contributing patches, please submit testcases along with it.  For
example, regarding this particular problem, I would believe that there
are more cases that we don't check properly yet.

Also, did you do the FSF copyright assignment paperwork yet?


Torvald
Dave Boutcher - May 16, 2012, 1:50 a.m.
On Tue, May 15, 2012 at 11:23 AM, Torvald Riegel <triegel@redhat.com> wrote:
>
> On Tue, 2012-05-08 at 18:02 -0500, Dave Boutcher wrote:
> > Without this patch it is perfectly fine to assign non-transaction_safe
> > functions to function pointers marked as transaction_safe. Unpleasantness
> > happens at run time.
> >
> > e.g.
> >
> >  __attribute__((transaction_safe)) long (*compare)(int, int);
> >
> > compare = my_funky_random_function;
> >
> >

> I don't think that assigning a tm_pure function to tm_safe works.  There
> will be no instrumented version of it.  I don't think we generate an
> alias or sth like that yet.

Torvald, we do create an alias...  In trans-mem.c around 4789

/* ... marked tm_pure, record that fact for the runtime by
  indicating that the pure function is its own tm_callable.
  No need to do this if the function's address can't be taken.  */
if (is_tm_pure (node->decl))
 {
   if (!node->local.local) {
     record_tm_clone_pair (node->decl, node->decl);

creates an entry in the clone table that points to itself so its safe
to later call _ITM_getTMCloneSafe()

> When contributing patches, please submit testcases along with it.  For
> example, regarding this particular problem, I would believe that there
> are more cases that we don't check properly yet.

Yeah, very likely.  This one was biting me so I added the check.

> Also, did you do the FSF copyright assignment paperwork yet?
I sent off the form...

--
Dave B

Patch

diff --git a/gcc/c-typeck.c b/gcc/c-typeck.c
index fc01a79..69687d6 100644
--- a/gcc/c-typeck.c
+++ b/gcc/c-typeck.c
@@ -5608,6 +5608,13 @@  convert_for_assignment (location_t location, tree type, tree rhs,
 	  }
 	}
 
+      /* Check for assignment to transaction safe */
+      if (is_tm_safe(type) && !is_tm_safe_or_pure (rhs)) {
+	  warning_at (location, 0,
+		      "Assigning unsafe function to transaction_safe "
+		      "function pointer"); 
+      }
+
       /* Any non-function converts to a [const][volatile] void *
 	 and vice versa; otherwise, targets must be the same.
 	 Meanwhile, the lhs target must have all the qualifiers of the rhs.  */