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

login
register
mail settings
Submitter Patrick Marlier
Date May 15, 2012, 9:16 p.m.
Message ID <4FB2C792.9040500@gmail.com>
Download mbox | patch
Permalink /patch/159454/
State New
Headers show

Comments

Patrick Marlier - May 15, 2012, 9:16 p.m.
Follow-up of Dave's patch. I would prefer to see such checks in 
trans-mem.c as follows.
In a transaction, a function pointer can be declared and assigned but 
there is no check that the function pointer is transaction_safe. So at 
runtime, if the function was unsafe, libitm stops on assert because the 
clone is not found.

Tested on i686.
Is the patch ok? Thanks.

BTW, Should we generate a warning or an error?
--
2012-05-15  Patrick Marlier  <patrick.marlier@gmail.com>

         * trans-mem.c (diagnose_tm_1_op): Warn about assignment of 
transaction
         unsafe function to safe function pointer.

testsuite/
2012-05-15  Patrick Marlier  <patrick.marlier@gmail.com>

         * c-c++-common/tm/indirect-1.c: New test.


On 05/15/2012 12:23 PM, Torvald Riegel 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;
>>
>>
>>   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:53 a.m.
On Tue, May 15, 2012 at 4:16 PM, Patrick Marlier
<patrick.marlier@gmail.com> wrote:
> Follow-up of Dave's patch. I would prefer to see such checks in trans-mem.c
> as follows.
> In a transaction, a function pointer can be declared and assigned but there
> is no check that the function pointer is transaction_safe. So at runtime, if
> the function was unsafe, libitm stops on assert because the clone is not
> found.

Of the three tm patches I sent, I'm least fond of this one.  I agree
that this check should be
in trans-mem.c, but it wasn't obvious to me how to add it there, since
the assignment of a
function to a pointer can happen anywhere in the code...far away from
an actual transaction.
Richard Henderson - May 16, 2012, 3:49 p.m.
On 05/15/2012 02:16 PM, Patrick Marlier wrote:
> Tested on i686.
> Is the patch ok? Thanks.
>
> BTW, Should we generate a warning or an error?
> --
> 2012-05-15  Patrick Marlier <patrick.marlier@gmail.com>
>
>          * trans-mem.c (diagnose_tm_1_op): Warn about assignment of transaction
>          unsafe function to safe function pointer.

Hum.  I wonder if there's some other way to do this in the front end.
The transition safe->unsafe is of course ok, but unsafe->safe is not.
It sure seems like we ought to be able to leverage the type check
elsewhere in the language.

Unfortunately, the only existing switch we have for attributes is a
hard compatible/incompatible flag.  So it would take some effort to
extend that to somethine more complicated.

I don't think this check here really does any good at all, since it
only applies to tm regions, and the assignment can happen anywhere.

I suppose for the moment we could flip the switch and force
incompatibility, then relax that as we improve the front end?


r~

Patch

Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 187371)
+++ trans-mem.c	(working copy)
@@ -572,6 +572,16 @@  diagnose_tm_1_op (tree *tp, int *walk_subtrees ATT
 		*tp);
     }
 
+  if (code == VAR_DECL
+      && TREE_CODE (TREE_TYPE (*tp)) == POINTER_TYPE
+      && ((d->block_flags & (DIAG_TM_SAFE | DIAG_TM_RELAXED))
+	  || (d->func_flags & DIAG_TM_SAFE))
+      && is_gimple_assign (d->stmt)
+      && is_tm_safe (*tp)
+      && !is_tm_safe (gimple_assign_rhs1 (d->stmt)))
+    warning_at (gimple_location (d->stmt), 0, "Assigning unsafe function to "
+		"transaction_safe function pointer");
+
   return NULL_TREE;
 }
 
Index: testsuite/c-c++-common/tm/indirect-1.c
===================================================================
--- testsuite/c-c++-common/tm/indirect-1.c	(revision 0)
+++ testsuite/c-c++-common/tm/indirect-1.c	(revision 0)
@@ -0,0 +1,25 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm" } */
+
+__attribute__((transaction_safe)) void (*func_ptr)(void);
+
+void bar(void);
+
+__attribute__((transaction_safe))
+void foo1(void)
+{
+  func_ptr = bar;     /* { dg-warning "Assigning unsafe function to transaction_safe" } */
+}
+
+void foo2(void)
+{
+  func_ptr = bar;
+}
+
+void foo3(void)
+{
+  __transaction_atomic {
+    func_ptr = bar;  /* { dg-warning "Assigning unsafe function to transaction_safe" } */
+  }
+}
+