[Bug libitm/53008] New: abort in _ITM_getTMCloneSafe

[Bug libitm/53008] New: abort in _ITM_getTMCloneSafe

[daveboutcher at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53008

             Bug #: 53008
           Summary: abort in _ITM_getTMCloneSafe
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libitm
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: daveboutcher@gmail.com


Created attachment 27169
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27169
testcase for abort in _ITM_getTMCloneSafe

Abort in _ITM_getTMCloneSafe() within atomic transaction calling a function via
a function pointer.  The abort only appears to happen if the function pointer
is defined as __attribute__((transaction_safe)).  Only happens at -O2 and
above.

Fairly minimal testcase attached.

[Bug libitm/53008] abort in _ITM_getTMCloneSafe

[daveboutcher at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53008

--- Comment #1 from Dave Boutcher <daveboutcher at gmail dot com> 2012-04-18 17:46:17 UTC ---
The problem seems to be that functions referenced only by function pointers are
not put in the tmclone table.  I can work around the problem by making a bogus
call to the function anywhere inside a transaction.

[Bug libitm/53008] abort in _ITM_getTMCloneSafe

[patrick.marlier at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53008

Patrick Marlier <patrick.marlier at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |patrick.marlier at gmail
                   |                            |dot com

--- Comment #2 from Patrick Marlier <patrick.marlier at gmail dot com> 2012-04-27 15:00:16 UTC ---
Hello Dave,

This is because your mycompare function is not transaction_safe, so no clone
can be found at runtime.

So if you add transaction_safe to your comparison function, it should work. In
your example it should be:

__attribute__((transaction_safe)) static long mycompare(int a, int b)

If it works, please closed the bug report. Thanks.
--
Patrick Marlier

[Bug libitm/53008] abort in _ITM_getTMCloneSafe

[daveboutcher at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53008

--- Comment #3 from Dave Boutcher <daveboutcher at gmail dot com> 2012-05-08 22:56:23 UTC ---
I just submitted a patch for this one (Issue 6201064)
(http://codereview.appspot.com/6201064/)

Patch included here just for fun:

Index: gcc/trans-mem.c
diff --git a/gcc/trans-mem.c b/gcc/trans-mem.c
index 24073fa..20ed5a0 100644
--- a/gcc/trans-mem.c
+++ b/gcc/trans-mem.c
@@ -4319,6 +4319,9 @@ ipa_tm_create_version_alias (struct cgraph_node *node,
void *data)

   record_tm_clone_pair (old_decl, new_decl);

+  /* If someone refers to this function indirectly, mark it needed */
+  if (ipa_ref_list_first_refering (&info->old_node->ref_list)) 
+      ipa_tm_mark_needed_node (info->old_node);
   if (info->old_node->needed)
     ipa_tm_mark_needed_node (new_node);
   return false;
@@ -4372,6 +4375,10 @@ ipa_tm_create_version (struct cgraph_node *old_node)
   record_tm_clone_pair (old_decl, new_decl);

   cgraph_call_function_insertion_hooks (new_node);
+  /* If someone refers to this function indirectly, mark it needed */
+  if (ipa_ref_list_first_refering (&old_node->ref_list)) 
+      ipa_tm_mark_needed_node (old_node);
+
   if (old_node->needed)
     ipa_tm_mark_needed_node (new_node);

@@ -4778,8 +4785,13 @@ ipa_tm_execute (void)
        No need to do this if the function's address can't be taken.  */
     if (is_tm_pure (node->decl))
       {
-        if (!node->local.local)
+        if (!node->local.local) {
           record_tm_clone_pair (node->decl, node->decl);
+          /* if someone refers to this function other than as a call,
+         mark it needed */
+          if (ipa_ref_list_first_refering (&node->ref_list)) 
+          ipa_tm_mark_needed_node (node);
+        }
         continue;
       }

[Bug libitm/53008] abort in _ITM_getTMCloneSafe

[torvald at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53008

torvald at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |torvald at gcc dot gnu.org

--- Comment #4 from torvald at gcc dot gnu.org 2012-05-15 16:18:00 UTC ---
So, is there still a libitm issue even with your GCC patch?

[Bug libitm/53008] abort in _ITM_getTMCloneSafe

[patrick.marlier at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53008

--- Comment #5 from Patrick Marlier <patrick.marlier at gmail dot com> 2012-05-15 22:23:54 UTC ---
Created attachment 27412
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27412
testcase for gcc testsuite

The problem is not into libitm.
Attached a testcase for testsuite (gcc/testsuite/gcc.dg/tm/indirect-3.c).

Note that with your patch, the tm testsuite fails. pr51516.C fails because the
original function is output and not only the clone (only the clone is
required). I am not sure the way to properly fix it.

Thanks for having a look at the problem!
--
Patrick Marlier

[Bug libitm/53008] abort in _ITM_getTMCloneSafe

[daveboutcher at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53008

--- Comment #6 from Dave Boutcher <daveboutcher at gmail dot com> 2012-05-16 01:43:51 UTC ---
Thanks for the testcase Patrick.  Let me dig into the pr51516.C failure before
I resubmit the patch with the testcase.

Torvald, as Patrick said, this seems to be purely a gcc issue...no libtm
problems I can see

[Bug ada/53008] abort in _ITM_getTMCloneSafe

[aldyh at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53008

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2012-05-22
          Component|libitm                      |ada
     Ever Confirmed|0                           |1

--- Comment #7 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2012-05-22 15:44:16 UTC ---
Is this still a problem?

First, I have compiled and run attachment #27169 for -O2, and there is no
abort.

Second, Patrick has suggested adding a transaction_safe attribute to
mycompare().  This should fix the problem.

Can we close this bug?

[Bug ada/53008] abort in _ITM_getTMCloneSafe

[patrick.marlier at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53008

--- Comment #8 from Patrick Marlier <patrick.marlier at gmail dot com> 2012-05-22 15:58:53 UTC ---
Aldy,

Actually the problem is different that my first thought and it is a real bug.
The problem is well described into the 'testcase for gcc testsuite'.
In the testcase, you have a static function foo which is transaction_safe so a
clone is created. This function foo is assigned to a transaction_safe function
pointer and this function pointer is used in a transaction. However this
tm-clone foo is never referenced so it is removed by
ipa.c:cgraph_remove_unreachable_nodes().
One way to fix the problem is force the output of the clone as following:

Index: trans-mem.c
===================================================================
--- trans-mem.c (revision 187371)
+++ trans-mem.c (working copy)
@@ -4330,7 +4330,8 @@ ipa_tm_create_version_alias (struct cgraph_node *n

   record_tm_clone_pair (old_decl, new_decl);

-  if (info->old_node->symbol.force_output)
+  if (info->old_node->symbol.force_output
+      || ipa_ref_list_first_referring (&info->old_node->symbol.ref_list))
     ipa_tm_mark_force_output_node (new_node);
   return false;
 }
@@ -4383,7 +4384,8 @@ ipa_tm_create_version (struct cgraph_node *old_nod
   record_tm_clone_pair (old_decl, new_decl);

   cgraph_call_function_insertion_hooks (new_node);
-  if (old_node->symbol.force_output)
+  if (old_node->symbol.force_output
+      || ipa_ref_list_first_referring (&old_node->symbol.ref_list))
     ipa_tm_mark_force_output_node (new_node);

   /* Do the same thing, but for any aliases of the original node.  */

However, I am not sure it is really the way to go. Probably we should add the
references from the orginal function to the tm-clone?

Thanks,
Patrick

[Bug middle-end/53008] abort in _ITM_getTMCloneSafe

[aldyh at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53008

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |NEW
          Component|ada                         |middle-end
         AssignedTo|unassigned at gcc dot       |aldyh at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #9 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2012-05-22 16:40:08 UTC ---
(In reply to comment #8)
> Aldy,
> 
> Actually the problem is different that my first thought and it is a real bug.
> The problem is well described into the 'testcase for gcc testsuite'.

Ah, understood.

Mine.

[Bug middle-end/53008] abort in _ITM_getTMCloneSafe

[aldyh at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53008

--- Comment #10 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2012-05-25 17:14:31 UTC ---
Author: aldyh
Date: Fri May 25 17:14:25 2012
New Revision: 187887

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=187887
Log:
        PR middle-end/53008
        * trans-mem.c (ipa_tm_create_version_alias): Output new_node if
        accessed indirectly.
        (ipa_tm_create_version): Same.


Added:
    trunk/gcc/testsuite/gcc.dg/tm/pr53008.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/trans-mem.c

[Bug middle-end/53008] abort in _ITM_getTMCloneSafe

[aldyh at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53008

--- Comment #11 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2012-05-30 22:13:48 UTC ---
Author: aldyh
Date: Wed May 30 22:13:43 2012
New Revision: 188030

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=188030
Log:
        Backport from mainline
        2012-05-25  Aldy Hernandez  <aldyh@redhat.com>
        PR middle-end/53008
        * trans-mem.c (ipa_tm_create_version_alias): Output new_node if
        accessed indirectly.
        (ipa_tm_create_version): Same.

Added:
    branches/gcc-4_7-branch/gcc/testsuite/gcc.dg/tm/pr53008.c
Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/trans-mem.c

[Bug middle-end/53008] abort in _ITM_getTMCloneSafe

[aldyh at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53008

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #12 from Aldy Hernandez <aldyh at gcc dot gnu.org> 2012-05-30 22:15:23 UTC ---
fixed on both mainline and 4.7 branch.