Issue 5448109: Fix a bug in ThreadSanitizer pass

Issue 5448109: Fix a bug in ThreadSanitizer pass (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
12 years, 4 months ago by dvyukov

Modified:
12 years, 4 months ago

Reviewers:
Diego Novillo, davidxl

CC:
gcc-patches_gcc.gnu.org

Base URL:
svn+ssh://gcc.gnu.org/svn/gcc/branches/google/main/

Visibility:
Public.

Patch Set 1 #

Patch Set 2 : ail #

Patch Set 3 : Fix a bug in ThreadSanitizer pass #

Created: 12 years, 4 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+13 lines, -1 line)			Patch
	M	gcc/ChangeLog.google-main	View		1 chunk	+5 lines, -0 lines	0 comments	Download
	M	gcc/tree-tsan.c	View		1 chunk	+8 lines, -1 line	0 comments	Download

Messages

Total messages: 3

Expand All Messages | Collapse All Messages

dvyukov

This is for google-main branch. Fix taking address of SSA_NAME in ThreadSanitizer pass. Index: gcc/tree-tsan.c ...

12 years, 4 months ago (2011-12-05 17:05:17 UTC) #1

dvyukov

On 2011/12/05 17:05:17, dvyukov wrote: > This is for google-main branch. > Fix taking address ...

12 years, 4 months ago (2011-12-07 11:04:43 UTC) #2

On 2011/12/05 17:05:17, dvyukov wrote:
> This is for google-main branch.
> Fix taking address of SSA_NAME in ThreadSanitizer pass.
> 
> Index: gcc/tree-tsan.c
> ===================================================================
> --- gcc/tree-tsan.c	(revision 182014)
> +++ gcc/tree-tsan.c	(working copy)
> @@ -726,13 +726,20 @@
>    struct mop_desc mop;
>    unsigned fld_off;
>    unsigned fld_size;
> +  tree base;
>  
> +
> +  base = get_base_address (expr);
> +  if (base == NULL_TREE
> +      || TREE_CODE (base) == SSA_NAME
> +      || TREE_CODE (base) == STRING_CST)
> +    return;
> +
>    tcode = TREE_CODE (expr);
>  
>    /* Below are things we do not instrument
>       (no possibility of races or not implemented yet).  */
>    if ((func_ignore & (tsan_ignore_mop | tsan_ignore_rec))
> -      || get_base_address (expr) == NULL
>        /* Compiler-emitted artificial variables.  */
>        || (DECL_P (expr) && DECL_ARTIFICIAL (expr))
>        /* The var does not live in memory -> no possibility of races.  */
> Index: gcc/ChangeLog.google-main
> ===================================================================
> --- gcc/ChangeLog.google-main	(revision 182014)
> +++ gcc/ChangeLog.google-main	(working copy)
> @@ -1,3 +1,8 @@
> +2011-12-05   Dmitriy Vyukov  <mailto:dvyukov@google.com>
> +
> +	Fix taking address of SSA_NAME in ThreadSanitizer pass.
> +        * gcc/tree-tsan.c (handle_expr): Add the additional check.
> +
>  2011-11-30   Dmitriy Vyukov  <mailto:dvyukov@google.com>
>  
>  	Add directives to run ThreadSanitizer tests
> 
> --
> This patch is available for review at http://codereview.appspot.com/5448109

Sorry, I forgot to provide details about the crash.

The compiler crashes on the following code (dump just before tsan pass):

store_param_double (struct NET * net, struct MYSQL_BIND * param)
{
  double value;
  unsigned char * D.26882;
  long int D.26881;
  long int D.26879;
  unsigned char * D.26877;
  double value.122;
  void * D.26875;

<bb 2>:
  D.26875_2 = param_1(D)->buffer;
  value.122_3 = MEM[(double *)D.26875_2];
  D.26877_5 = net_4(D)->write_pos;
  D.26879_7 = VIEW_CONVERT_EXPR<union doubleget_union>(value.122_3).m[0];
  MEM[(long int *)D.26877_5] = D.26879_7;
  D.26877_8 = net_4(D)->write_pos;
  D.26881_11 = VIEW_CONVERT_EXPR<union doubleget_union>(value.122_3).m[1];
  MEM[(long int *)D.26877_8 + 4B] = D.26881_11;
  D.26877_12 = net_4(D)->write_pos;
  D.26882_13 = D.26877_12 + 8;
  net_4(D)->write_pos = D.26882_13;
  return;
}

with the following error message:

libmysql.c: In function 'store_param_double':
libmysql.c:2314:13: error: conversion of an SSA_NAME on the left hand side
VIEW_CONVERT_EXPR<union doubleget_union>(D.29825_23);
D.29824_24 = &VIEW_CONVERT_EXPR<union doubleget_union>(D.29825_23).m[0];
libmysql.c:2314:13: internal compiler error: verify_gimple failed

Is it a correct fix for the issue? If base_address refers to SSA_NAME we just do
not instrument it, right?

davidxl

12 years, 4 months ago (2011-12-07 17:42:21 UTC) #3

ok for google/main.

David

On Wed, Dec 7, 2011 at 3:04 AM,  <dvyukov@google.com> wrote:
> On 2011/12/05 17:05:17, dvyukov wrote:
>>
>> This is for google-main branch.
>> Fix taking address of SSA_NAME in ThreadSanitizer pass.
>
>
>> Index: gcc/tree-tsan.c
>> ===================================================================
>> --- gcc/tree-tsan.c     (revision 182014)
>> +++ gcc/tree-tsan.c     (working copy)
>> @@ -726,13 +726,20 @@
>>    struct mop_desc mop;
>>    unsigned fld_off;
>>    unsigned fld_size;
>> +  tree base;
>
>
>> +
>> +  base = get_base_address (expr);
>> +  if (base == NULL_TREE
>> +      || TREE_CODE (base) == SSA_NAME
>> +      || TREE_CODE (base) == STRING_CST)
>> +    return;
>> +
>>    tcode = TREE_CODE (expr);
>
>
>>    /* Below are things we do not instrument
>>       (no possibility of races or not implemented yet).  */
>>    if ((func_ignore & (tsan_ignore_mop | tsan_ignore_rec))
>> -      || get_base_address (expr) == NULL
>>        /* Compiler-emitted artificial variables.  */
>>        || (DECL_P (expr) && DECL_ARTIFICIAL (expr))
>>        /* The var does not live in memory -> no possibility of races.
>
> */
>>
>> Index: gcc/ChangeLog.google-main
>> ===================================================================
>> --- gcc/ChangeLog.google-main   (revision 182014)
>> +++ gcc/ChangeLog.google-main   (working copy)
>> @@ -1,3 +1,8 @@
>> +2011-12-05   Dmitriy Vyukov  <mailto:dvyukov@google.com>
>>
>> +
>> +       Fix taking address of SSA_NAME in ThreadSanitizer pass.
>> +        * gcc/tree-tsan.c (handle_expr): Add the additional check.
>> +
>>  2011-11-30   Dmitriy Vyukov  <mailto:dvyukov@google.com>
>
>
>>        Add directives to run ThreadSanitizer tests
>
>
>> --
>> This patch is available for review at
>
> http://codereview.appspot.com/5448109
>
> Sorry, I forgot to provide details about the crash.
>
> The compiler crashes on the following code (dump just before tsan pass):
>
> store_param_double (struct NET * net, struct MYSQL_BIND * param)
> {
>  double value;
>  unsigned char * D.26882;
>  long int D.26881;
>  long int D.26879;
>  unsigned char * D.26877;
>  double value.122;
>  void * D.26875;
>
> <bb 2>:
>  D.26875_2 = param_1(D)->buffer;
>  value.122_3 = MEM[(double *)D.26875_2];
>  D.26877_5 = net_4(D)->write_pos;
>  D.26879_7 = VIEW_CONVERT_EXPR<union
> doubleget_union>(value.122_3).m[0];
>  MEM[(long int *)D.26877_5] = D.26879_7;
>  D.26877_8 = net_4(D)->write_pos;
>  D.26881_11 = VIEW_CONVERT_EXPR<union
> doubleget_union>(value.122_3).m[1];
>  MEM[(long int *)D.26877_8 + 4B] = D.26881_11;
>  D.26877_12 = net_4(D)->write_pos;
>  D.26882_13 = D.26877_12 + 8;
>  net_4(D)->write_pos = D.26882_13;
>  return;
> }
>
> with the following error message:
>
> libmysql.c: In function 'store_param_double':
> libmysql.c:2314:13: error: conversion of an SSA_NAME on the left hand
> side
> VIEW_CONVERT_EXPR<union doubleget_union>(D.29825_23);
> D.29824_24 = &VIEW_CONVERT_EXPR<union doubleget_union>(D.29825_23).m[0];
> libmysql.c:2314:13: internal compiler error: verify_gimple failed
>
> Is it a correct fix for the issue? If base_address refers to SSA_NAME we
> just do not instrument it, right?
>
>
> http://codereview.appspot.com/5448109/

Expand All Messages | Collapse All Messages