Issue 6198054: gnu-tm: Dont allow assigning transaction_unsafe functions to transaction_safe function pointers
|
Created:
12 years, 11 months ago by daveb Modified:
1 year, 5 months ago Reviewers:
patrick.marlier, triegel, rth, patrick.marlier CC:
gcc-patches_gcc.gnu.org Visibility:
Public. |
Patch Set 1 #
MessagesTotal messages: 6
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)) {
+ 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. */
--
1.7.9.5
--
This patch is available for review at http://codereview.appspot.com/6198054
Sign in to reply to this message.
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
Sign in to reply to this message.
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 > >
Sign in to reply to this message.
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
Sign in to reply to this message.
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. -- Dave B
Sign in to reply to this message.
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~
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||
