Patchwork [google,4.7] fix line number checksum mismatch in lipo-use (issue6566044)

login
register
mail settings
Submitter Rong Xu
Date Sept. 24, 2012, 9:42 p.m.
Message ID <20120924214232.B0F1B55CAC@rong2.dls.corp.google.com>
Download mbox | patch
Permalink /patch/186580/
State New
Headers show

Comments

Rong Xu - Sept. 24, 2012, 9:42 p.m.
Hi, 

This is for google branches only. 
It fix the lino number checksum mismatch during LIPO-use build.

Tested with SPEC and google internal banchmarks.

Thanks,

-Rong

2012-09-24  Rong Xu  <xur@google.com>

	* gcc/coverage.c (coverage_checksum_string): strip out LIPO 
        specific string. 
        (crc32_string_1): New function.
	* gcc/cp/decl2.c (start_static_storage_duration_function):
        generate LIPO specific string.


--
This patch is available for review at http://codereview.appspot.com/6566044
Xinliang David Li - Sept. 25, 2012, 9:25 p.m.
On Mon, Sep 24, 2012 at 2:42 PM, Rong Xu <xur@google.com> wrote:
> Hi,
>
> This is for google branches only.
> It fix the lino number checksum mismatch during LIPO-use build.
>
> Tested with SPEC and google internal banchmarks.
>
> Thanks,
>
> -Rong
>
> 2012-09-24  Rong Xu  <xur@google.com>
>
>         * gcc/coverage.c (coverage_checksum_string): strip out LIPO
>         specific string.
>         (crc32_string_1): New function.
>         * gcc/cp/decl2.c (start_static_storage_duration_function):
>         generate LIPO specific string.
>
> Index: gcc/coverage.c
> ===================================================================
> --- gcc/coverage.c      (revision 191679)
> +++ gcc/coverage.c      (working copy)
> @@ -903,6 +903,27 @@
>  }
>
>
> +/* Generate a crc32 of a string with specified STR_ELN when it's not 0.

STR_ELN --> STR_LEN

> +   Non-zero STR_LEN should only be seen in LIPO mode.  */

Empty line needed.

> +static unsigned
> +crc32_string_1 (unsigned chksum, const char *string, unsigned str_len)
> +{
> +  char *dup;
> +
> +  if (!L_IPO_COMP_MODE || str_len == 0)
> +    return crc32_string (chksum, string);
> +
> +  gcc_assert (str_len > 0 && str_len < strlen(string));
> +  dup = xstrdup (string);
> +  dup[str_len] = 0;
> +  chksum = crc32_string (chksum, dup);
> +  free (dup);
> +
> +  return chksum;
> +

Remove extra lines after return.

> +
> +}
> +
>  /* Generate a checksum for a string.  CHKSUM is the current
>     checksum.  */
>
> @@ -911,7 +932,26 @@
>  {
>    int i;
>    char *dup = NULL;
> +  unsigned lipo_orig_str_len = 0;
>
> +  /* Strip out the ending "_cmo_[0-9]*" string from function
> +     name. Otherwise we will have lineno checksum mismatch.  */
> +  if (L_IPO_COMP_MODE)
> +    {
> +      int len;
> +
> +      i = len = strlen (string);
> +      while (i--)
> +        if ((string[i] < '0' || string[i] > '9'))
> +          break;
> +      if ((i > 5) && (i != len - 1))

 i >= 5?

> +        {
> +          if (!strncmp (string + i - 4, "_cmo_", 5))

_cmo_ or .cmo. ?

> +            lipo_orig_str_len = i - 4;
> +        }
> +
> +    }
> +
>    /* Look for everything that looks if it were produced by
>       get_file_function_name and zero out the second part
>       that may result from flag_random_seed.  This is not critical
> @@ -957,7 +997,7 @@
>         }
>      }
>
> -  chksum = crc32_string (chksum, string);
> +  chksum = crc32_string_1 (chksum, string, lipo_orig_str_len);
>    if (dup)
>      free (dup);
>
> Index: gcc/cp/decl2.c
> ===================================================================
> --- gcc/cp/decl2.c      (revision 191679)
> +++ gcc/cp/decl2.c      (working copy)
> @@ -2911,7 +2911,7 @@
>       SSDF_IDENTIFIER_<number>.  */
>    sprintf (id, "%s_%u", SSDF_IDENTIFIER, count);
>    if (L_IPO_IS_AUXILIARY_MODULE)
> -    sprintf (id, "%s_%u", id, current_module_id);
> +    sprintf (id, "%s_cmo_%u", id, current_module_id);

_cmo_ or .cmo. for consistency?

David

>
>    type = build_function_type_list (void_type_node,
>                                    integer_type_node, integer_type_node,
>
> --
> This patch is available for review at http://codereview.appspot.com/6566044
Rong Xu - Sept. 28, 2012, 5:22 p.m.
Comments are inlined.
Attached is the new patch.

Thanks,

-Rong

On Tue, Sep 25, 2012 at 2:25 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Mon, Sep 24, 2012 at 2:42 PM, Rong Xu <xur@google.com> wrote:
>> Hi,
>>
>> This is for google branches only.
>> It fix the lino number checksum mismatch during LIPO-use build.
>>
>> Tested with SPEC and google internal banchmarks.
>>
>> Thanks,
>>
>> -Rong
>>
>> 2012-09-24  Rong Xu  <xur@google.com>
>>
>>         * gcc/coverage.c (coverage_checksum_string): strip out LIPO
>>         specific string.
>>         (crc32_string_1): New function.
>>         * gcc/cp/decl2.c (start_static_storage_duration_function):
>>         generate LIPO specific string.
>>
>> Index: gcc/coverage.c
>> ===================================================================
>> --- gcc/coverage.c      (revision 191679)
>> +++ gcc/coverage.c      (working copy)
>> @@ -903,6 +903,27 @@
>>  }
>>
>>
>> +/* Generate a crc32 of a string with specified STR_ELN when it's not 0.
>
> STR_ELN --> STR_LEN

Fixed.

>
>> +   Non-zero STR_LEN should only be seen in LIPO mode.  */
>
> Empty line needed.

Fixed.

>
>> +static unsigned
>> +crc32_string_1 (unsigned chksum, const char *string, unsigned str_len)
>> +{
>> +  char *dup;
>> +
>> +  if (!L_IPO_COMP_MODE || str_len == 0)
>> +    return crc32_string (chksum, string);
>> +
>> +  gcc_assert (str_len > 0 && str_len < strlen(string));
>> +  dup = xstrdup (string);
>> +  dup[str_len] = 0;
>> +  chksum = crc32_string (chksum, dup);
>> +  free (dup);
>> +
>> +  return chksum;
>> +
>
> Remove extra lines after return.

Fixed.

>
>> +
>> +}
>> +
>>  /* Generate a checksum for a string.  CHKSUM is the current
>>     checksum.  */
>>
>> @@ -911,7 +932,26 @@
>>  {
>>    int i;
>>    char *dup = NULL;
>> +  unsigned lipo_orig_str_len = 0;
>>
>> +  /* Strip out the ending "_cmo_[0-9]*" string from function
>> +     name. Otherwise we will have lineno checksum mismatch.  */
>> +  if (L_IPO_COMP_MODE)
>> +    {
>> +      int len;
>> +
>> +      i = len = strlen (string);
>> +      while (i--)
>> +        if ((string[i] < '0' || string[i] > '9'))
>> +          break;
>> +      if ((i > 5) && (i != len - 1))
>
>  i >= 5?

This should not matter because we are expecting a non-empty sub-string
before "_cmo_". If there not sub-string before "_cmo_", the original
code will do nothing (which I think it's correct in the case of user
defined name.)

>
>> +        {
>> +          if (!strncmp (string + i - 4, "_cmo_", 5))
>
> _cmo_ or .cmo. ?
>
>> +            lipo_orig_str_len = i - 4;
>> +        }
>> +
>> +    }
>> +
>>    /* Look for everything that looks if it were produced by
>>       get_file_function_name and zero out the second part
>>       that may result from flag_random_seed.  This is not critical
>> @@ -957,7 +997,7 @@
>>         }
>>      }
>>
>> -  chksum = crc32_string (chksum, string);
>> +  chksum = crc32_string_1 (chksum, string, lipo_orig_str_len);
>>    if (dup)
>>      free (dup);
>>
>> Index: gcc/cp/decl2.c
>> ===================================================================
>> --- gcc/cp/decl2.c      (revision 191679)
>> +++ gcc/cp/decl2.c      (working copy)
>> @@ -2911,7 +2911,7 @@
>>       SSDF_IDENTIFIER_<number>.  */
>>    sprintf (id, "%s_%u", SSDF_IDENTIFIER, count);
>>    if (L_IPO_IS_AUXILIARY_MODULE)
>> -    sprintf (id, "%s_%u", id, current_module_id);
>> +    sprintf (id, "%s_cmo_%u", id, current_module_id);
>
> _cmo_ or .cmo. for consistency?

Changed all "_cmo_" to ".cmo.".

>
> David
>
>>
>>    type = build_function_type_list (void_type_node,
>>                                    integer_type_node, integer_type_node,
>>
>> --
>> This patch is available for review at http://codereview.appspot.com/6566044
Xinliang David Li - Sept. 28, 2012, 5:33 p.m.
ok (for google-47 and google/main)

thanks,

David

On Fri, Sep 28, 2012 at 10:22 AM, Rong Xu <xur@google.com> wrote:
> Comments are inlined.
> Attached is the new patch.
>
> Thanks,
>
> -Rong
>
> On Tue, Sep 25, 2012 at 2:25 PM, Xinliang David Li <davidxl@google.com> wrote:
>> On Mon, Sep 24, 2012 at 2:42 PM, Rong Xu <xur@google.com> wrote:
>>> Hi,
>>>
>>> This is for google branches only.
>>> It fix the lino number checksum mismatch during LIPO-use build.
>>>
>>> Tested with SPEC and google internal banchmarks.
>>>
>>> Thanks,
>>>
>>> -Rong
>>>
>>> 2012-09-24  Rong Xu  <xur@google.com>
>>>
>>>         * gcc/coverage.c (coverage_checksum_string): strip out LIPO
>>>         specific string.
>>>         (crc32_string_1): New function.
>>>         * gcc/cp/decl2.c (start_static_storage_duration_function):
>>>         generate LIPO specific string.
>>>
>>> Index: gcc/coverage.c
>>> ===================================================================
>>> --- gcc/coverage.c      (revision 191679)
>>> +++ gcc/coverage.c      (working copy)
>>> @@ -903,6 +903,27 @@
>>>  }
>>>
>>>
>>> +/* Generate a crc32 of a string with specified STR_ELN when it's not 0.
>>
>> STR_ELN --> STR_LEN
>
> Fixed.
>
>>
>>> +   Non-zero STR_LEN should only be seen in LIPO mode.  */
>>
>> Empty line needed.
>
> Fixed.
>
>>
>>> +static unsigned
>>> +crc32_string_1 (unsigned chksum, const char *string, unsigned str_len)
>>> +{
>>> +  char *dup;
>>> +
>>> +  if (!L_IPO_COMP_MODE || str_len == 0)
>>> +    return crc32_string (chksum, string);
>>> +
>>> +  gcc_assert (str_len > 0 && str_len < strlen(string));
>>> +  dup = xstrdup (string);
>>> +  dup[str_len] = 0;
>>> +  chksum = crc32_string (chksum, dup);
>>> +  free (dup);
>>> +
>>> +  return chksum;
>>> +
>>
>> Remove extra lines after return.
>
> Fixed.
>
>>
>>> +
>>> +}
>>> +
>>>  /* Generate a checksum for a string.  CHKSUM is the current
>>>     checksum.  */
>>>
>>> @@ -911,7 +932,26 @@
>>>  {
>>>    int i;
>>>    char *dup = NULL;
>>> +  unsigned lipo_orig_str_len = 0;
>>>
>>> +  /* Strip out the ending "_cmo_[0-9]*" string from function
>>> +     name. Otherwise we will have lineno checksum mismatch.  */
>>> +  if (L_IPO_COMP_MODE)
>>> +    {
>>> +      int len;
>>> +
>>> +      i = len = strlen (string);
>>> +      while (i--)
>>> +        if ((string[i] < '0' || string[i] > '9'))
>>> +          break;
>>> +      if ((i > 5) && (i != len - 1))
>>
>>  i >= 5?
>
> This should not matter because we are expecting a non-empty sub-string
> before "_cmo_". If there not sub-string before "_cmo_", the original
> code will do nothing (which I think it's correct in the case of user
> defined name.)
>
>>
>>> +        {
>>> +          if (!strncmp (string + i - 4, "_cmo_", 5))
>>
>> _cmo_ or .cmo. ?
>>
>>> +            lipo_orig_str_len = i - 4;
>>> +        }
>>> +
>>> +    }
>>> +
>>>    /* Look for everything that looks if it were produced by
>>>       get_file_function_name and zero out the second part
>>>       that may result from flag_random_seed.  This is not critical
>>> @@ -957,7 +997,7 @@
>>>         }
>>>      }
>>>
>>> -  chksum = crc32_string (chksum, string);
>>> +  chksum = crc32_string_1 (chksum, string, lipo_orig_str_len);
>>>    if (dup)
>>>      free (dup);
>>>
>>> Index: gcc/cp/decl2.c
>>> ===================================================================
>>> --- gcc/cp/decl2.c      (revision 191679)
>>> +++ gcc/cp/decl2.c      (working copy)
>>> @@ -2911,7 +2911,7 @@
>>>       SSDF_IDENTIFIER_<number>.  */
>>>    sprintf (id, "%s_%u", SSDF_IDENTIFIER, count);
>>>    if (L_IPO_IS_AUXILIARY_MODULE)
>>> -    sprintf (id, "%s_%u", id, current_module_id);
>>> +    sprintf (id, "%s_cmo_%u", id, current_module_id);
>>
>> _cmo_ or .cmo. for consistency?
>
> Changed all "_cmo_" to ".cmo.".
>
>>
>> David
>>
>>>
>>>    type = build_function_type_list (void_type_node,
>>>                                    integer_type_node, integer_type_node,
>>>
>>> --
>>> This patch is available for review at http://codereview.appspot.com/6566044

Patch

Index: gcc/coverage.c
===================================================================
--- gcc/coverage.c	(revision 191679)
+++ gcc/coverage.c	(working copy)
@@ -903,6 +903,27 @@ 
 }
 
 
+/* Generate a crc32 of a string with specified STR_ELN when it's not 0.
+   Non-zero STR_LEN should only be seen in LIPO mode.  */
+static unsigned
+crc32_string_1 (unsigned chksum, const char *string, unsigned str_len)
+{
+  char *dup;
+
+  if (!L_IPO_COMP_MODE || str_len == 0)
+    return crc32_string (chksum, string);
+
+  gcc_assert (str_len > 0 && str_len < strlen(string));
+  dup = xstrdup (string);
+  dup[str_len] = 0;
+  chksum = crc32_string (chksum, dup);
+  free (dup);
+
+  return chksum;
+
+
+}
+
 /* Generate a checksum for a string.  CHKSUM is the current
    checksum.  */
 
@@ -911,7 +932,26 @@ 
 {
   int i;
   char *dup = NULL;
+  unsigned lipo_orig_str_len = 0;
 
+  /* Strip out the ending "_cmo_[0-9]*" string from function
+     name. Otherwise we will have lineno checksum mismatch.  */
+  if (L_IPO_COMP_MODE)
+    {
+      int len;
+
+      i = len = strlen (string);
+      while (i--)
+        if ((string[i] < '0' || string[i] > '9'))
+          break;
+      if ((i > 5) && (i != len - 1))
+        {
+          if (!strncmp (string + i - 4, "_cmo_", 5))
+            lipo_orig_str_len = i - 4;
+        }
+
+    }
+
   /* Look for everything that looks if it were produced by
      get_file_function_name and zero out the second part
      that may result from flag_random_seed.  This is not critical
@@ -957,7 +997,7 @@ 
 	}
     }
 
-  chksum = crc32_string (chksum, string);
+  chksum = crc32_string_1 (chksum, string, lipo_orig_str_len);
   if (dup)
     free (dup);
 
Index: gcc/cp/decl2.c
===================================================================
--- gcc/cp/decl2.c	(revision 191679)
+++ gcc/cp/decl2.c	(working copy)
@@ -2911,7 +2911,7 @@ 
      SSDF_IDENTIFIER_<number>.  */
   sprintf (id, "%s_%u", SSDF_IDENTIFIER, count);
   if (L_IPO_IS_AUXILIARY_MODULE)
-    sprintf (id, "%s_%u", id, current_module_id);
+    sprintf (id, "%s_cmo_%u", id, current_module_id);
 
   type = build_function_type_list (void_type_node,
 				   integer_type_node, integer_type_node,