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

[Rong Xu <xur at google dot com>]

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

Attachment: patch_set2.diff
Description: Binary data