Re: gnu-tm: Dont allow assigning transaction_unsafe functions to transaction_safe function pointers (issue6198054)
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.marl...@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~
Re: gnu-tm: Dont allow assigning transaction_unsafe functions to transaction_safe function pointers (issue6198054)
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
Re: gnu-tm: Dont allow assigning transaction_unsafe functions to transaction_safe function pointers (issue6198054)
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.marl...@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.marl...@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
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 } */
+ }
+}
+
Re: gnu-tm: Dont allow assigning transaction_unsafe functions to transaction_safe function pointers (issue6198054)
On Tue, May 15, 2012 at 11:23 AM, Torvald Riegel trie...@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
Re: gnu-tm: Dont allow assigning transaction_unsafe functions to transaction_safe function pointers (issue6198054)
On Tue, May 15, 2012 at 4:16 PM, Patrick Marlier patrick.marl...@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. -- Dave B
