| Message ID | 4FB2C792.9040500@gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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~
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" } */ + } +} +
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 > >