Patchwork [trans-mem] random segfault

login
register
mail settings
Submitter Aldy Hernandez
Date June 15, 2010, 2:25 p.m.
Message ID <20100615142525.GC27062@redhat.com>
Download mbox | patch
Permalink /patch/55675/
State New
Headers show

Comments

Aldy Hernandez - June 15, 2010, 2:25 p.m.
> Hi Aldy,
>
> I was trying to compile almost the same code and gcc is going into a  
> stack overflow (infinite call).
>
> Here the testcase (gcc -fgnu-tm -Wall -O1 -o common.o -c common.c):
> __attribute__((transaction_safe))
> void Info_RemoveKey (char *s)
> {
>         char    *o = 0;
>         while (1)
>         {
>                 s++;
>                 while (*s)
>                 {
>                         if (!*s)
>                                 return;
>                         *o++ = *s++;
>                 }
>                 *o = 0;
>         }
> }

thread_private_new_memory() is recursing on itself when the web of PHI
nodes has a mutual dependency (albeit with SSA variables originating
from different basic blocks).

Fixed by making sure we're not recursing on the same variable.

OK for branch?

	* trans-mem.c (thread_private_new_memory): Avoid infinite recursion.
Richard Henderson - June 15, 2010, 4:32 p.m.
On 06/15/2010 07:25 AM, Aldy Hernandez wrote:
> Fixed by making sure we're not recursing on the same variable.
> 
> OK for branch?
> 
> 	* trans-mem.c (thread_private_new_memory): Avoid infinite recursion.

Ah, a side-effect of the last patch that changed the insert handling.
I'm not sure this is right, Aldy, since this only detects a chain of
length 1, and not longer chains.

I think you have to go back and change 

  slot = htab_find_slot (tm_new_mem_hash, &elt, INSERT)
  elt_p = (tm_new_mem_map_t *) *slot;
  if (elt_p)
    return elt_p->local_new_memory;

  /* Optimistically assume the memory is transaction local during
     processing.  This catches recursion into this variable.  */
  *slot = elt_p = XNEW (tm_new_mem_map_t);
  elt_p->val = val;
  elt_p->local_new_memory = mem_transaction_local;

  /* Search DEF chain... */
  ...

 new_memory_ret:
  elt_p->local_new_memory = retval;
  return retval;


This should fix both bugs since (1) we detect recursion on longer
chains and (2) we don't touch SLOT after recursion; we only touch
ELT_P, which does not change with hash resizing.


r~
Patrick Marlier - June 15, 2010, 5:44 p.m.
On 06/15/2010 06:32 PM, Richard Henderson wrote:
> On 06/15/2010 07:25 AM, Aldy Hernandez wrote:
>> Fixed by making sure we're not recursing on the same variable.
>>
>> OK for branch?
>>
>> 	* trans-mem.c (thread_private_new_memory): Avoid infinite recursion.
>
> Ah, a side-effect of the last patch that changed the insert handling.
> I'm not sure this is right, Aldy, since this only detects a chain of
> length 1, and not longer chains.
>
> This should fix both bugs since (1) we detect recursion on longer
> chains and (2) we don't touch SLOT after recursion; we only touch
> ELT_P, which does not change with hash resizing.

Hello Aldy,

I think I fall into this problem with this testcase:

#include <stdlib.h>

__attribute__((transaction_safe))
static void cfltcvt(char *buffer)
{
   int decpt, pos;
   char *digits = NULL;

   pos = decpt = 0;
   while (*digits)
   {
     if (pos++ == decpt) *buffer++ = '.';
     *buffer++ = *digits++;
   }

}

__attribute__((transaction_pure))
double varg();

__attribute__((transaction_safe))
int Q_vsprintf()
{
   char *str;

   varg();

   cfltcvt(str);
   return 0;
}

Here the backtrace:

(gdb) bt
#0  0x0000000001096c74 in htab_mod (hash=4285609909, htab=Cannot access 
memory at address 0x7fffff5feff8
) at ../../transactional-memory/libiberty/hashtab.c:271
#1  0x00000000010977b0 in htab_find_slot_with_hash (htab=0x1750110, 
element=0x7fffff5ff0e0, hash=4285609909, insert=NO_INSERT)
     at ../../transactional-memory/libiberty/hashtab.c:627
#2  0x000000000109799e in htab_find_slot (htab=0x1750110, 
element=0x7fffff5ff0e0, insert=NO_INSERT)
     at ../../transactional-memory/libiberty/hashtab.c:681
#3  0x00000000009ae186 in thread_private_new_memory 
(entry_block=0x7ffff7f5e750, x=0x7ffff7137b58)
     at ../../transactional-memory/gcc/trans-mem.c:1271
#4  0x00000000009ae34e in thread_private_new_memory 
(entry_block=0x7ffff7f5e750, x=0x7ffff7137630)
     at ../../transactional-memory/gcc/trans-mem.c:1338
...
#72785 0x00000000009ae34e in thread_private_new_memory 
(entry_block=0x7ffff7f5e750, x=0x7ffff7137b58)
     at ../../transactional-memory/gcc/trans-mem.c:1338
#72786 0x00000000009ae34e in thread_private_new_memory 
(entry_block=0x7ffff7f5e750, x=0x7ffff7137630)
     at ../../transactional-memory/gcc/trans-mem.c:1338
#72787 0x00000000009ae34e in thread_private_new_memory 
(entry_block=0x7ffff7f5e750, x=0x7ffff7137b58)
     at ../../transactional-memory/gcc/trans-mem.c:1338
#72788 0x00000000009ae60b in requires_barrier 
(entry_block=0x7ffff7f5e750, x=0x7ffff70d7e10, stmt=0x0)
     at ../../transactional-memory/gcc/trans-mem.c:1395
#72789 0x00000000009b00f0 in expand_assign_tm (region=0x1797480, 
gsi=0x7fffffffe010)
     at ../../transactional-memory/gcc/trans-mem.c:2047
#72790 0x00000000009b05ea in expand_block_tm (region=0x1797480, 
bb=0x7ffff71384e0)
     at ../../transactional-memory/gcc/trans-mem.c:2191
#72791 0x00000000009b0998 in execute_tm_mark () at 
../../transactional-memory/gcc/trans-mem.c:2301
#72792 0x00000000008af445 in execute_one_pass (pass=0x165c260) at 
../../transactional-memory/gcc/passes.c:1579
#72793 0x00000000008af63f in execute_pass_list (pass=0x165c260) at 
../../transactional-memory/gcc/passes.c:1634
#72794 0x00000000008af660 in execute_pass_list (pass=0x165c200) at 
../../transactional-memory/gcc/passes.c:1635
#72795 0x0000000000a3602c in tree_rest_of_compilation 
(fndecl=0x7ffff7127d00) at 
../../transactional-memory/gcc/tree-optimize.c:413

#72796 0x0000000000c915ca in cgraph_expand_function 
(node=0x7ffff7e834e0) at ../../transactional-memory/gcc/cgraphunit.c:1548
#72797 0x0000000000c9185f in cgraph_expand_all_functions () at 
../../transactional-memory/gcc/cgraphunit.c:1627
#72798 0x0000000000c91ec0 in cgraph_optimize () at 
../../transactional-memory/gcc/cgraphunit.c:1875
#72799 0x0000000000c8faae in cgraph_finalize_compilation_unit () at 
../../transactional-memory/gcc/cgraphunit.c:1096

#72800 0x00000000004afdd2 in c_write_global_declarations () at 
../../transactional-memory/gcc/c-decl.c:9552
#72801 0x00000000009a5b52 in compile_file () at 
../../transactional-memory/gcc/toplev.c:1065
#72802 0x00000000009a7e19 in do_compile () at 
../../transactional-memory/gcc/toplev.c:2424
#72803 0x00000000009a7ee9 in toplev_main (argc=17, argv=0x7fffffffe438) 
at ../../transactional-memory/gcc/toplev.c:2466
#72804 0x000000000056c0f0 in main (argc=17, argv=0x7fffffffe438) at 
../../transactional-memory/gcc/main.c:35

Have a nice day,

Patrick.

Patch

Index: testsuite/gcc.dg/tm/20100615-2.c
===================================================================
--- testsuite/gcc.dg/tm/20100615-2.c	(revision 0)
+++ testsuite/gcc.dg/tm/20100615-2.c	(revision 0)
@@ -0,0 +1,19 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm -O" } */
+
+__attribute__((transaction_safe))
+void Info_RemoveKey (char *s)
+{
+        char    *o = 0;
+        while (1)
+        {
+                s++;
+                while (*s)
+                {
+                        if (!*s)
+                                return;
+                        *o++ = *s++;
+                }
+                *o = 0;
+        }
+}
Index: trans-mem.c
===================================================================
--- trans-mem.c	(revision 160765)
+++ trans-mem.c	(working copy)
@@ -1331,6 +1331,10 @@  thread_private_new_memory (basic_block e
 		  if (phi_result == op)
 		    continue;
 
+		  /* Ignore self-recursion in calculation.  */
+		  if (val == op)
+		    continue;
+
 		  mem = thread_private_new_memory (entry_block, op);
 		  if (mem == mem_non_local)
 		    {