Patchwork [Google] Fix test failure after porting __gcov_get_profile_prefix from google/4_7

login
register
mail settings
Submitter Teresa Johnson
Date Sept. 13, 2013, 5:18 a.m.
Message ID <CAAe5K+WKck0n-pfgztGHX_JnfuUOKwJFAOMzn6UfA0cFUjjgPw@mail.gmail.com>
Download mbox | patch
Permalink /patch/274657/
State New
Headers show

Comments

Teresa Johnson - Sept. 13, 2013, 5:18 a.m.
On Thu, Sep 12, 2013 at 2:32 PM, Xinliang David Li <davidxl@google.com> wrote:
> When absolute path is specified for the object file, no prefix will be
> prepended to the gcda path. If you record the cwd as in the
> _gcov_profile_prefix variable, at profile dump time, the prefix will
> be wrong -- as it is never used.

Yes I think I agree with you now.

Basically, for non-lto compilations, you get the following:
-fprofile-generate={path}
   -> no auxbase-strip and profile_data_prefix={path}
-fprofile-generate -o relative/path/to/file.o
   -> no auxbase-strip and profile_data_prefix=getpwd()
-fprofile-generate -o /absolute/path/to/file.o
   -> auxbase-strip /absolute/path/to/file.o and profile_data_prefix=NULL

But with -flto and -fprofile-generate -o relative/path/to/file.o
   -> auxbase-strip /tmp/file.ltrans.out and profile_data_prefix=NULL

In the LTO case the gcda files will go into cwd, but not in the case
just above where the absolute path is given to the object file.
However, for our purposes we rely on the path being specified to
-fprofile-generate={path} in places where we query
__gcov_profile_prefix in order to find the dump directory. Therefore,
I think it is best to simply record a NULL string as the
profile_data_prefix value in all cases where profile_data_prefix=NULL.

Here is the patch I am regression testing:


>
> David
>
> On Thu, Sep 12, 2013 at 2:07 PM, Teresa Johnson <tejohnson@google.com> wrote:
>> On Thu, Sep 12, 2013 at 1:20 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> On Thu, Sep 12, 2013 at 1:06 PM, Teresa Johnson <tejohnson@google.com> wrote:
>>>> After porting r198033 from google/4_7 to google/4_8 a test case failed
>>>> with an assert when trying to take the strlen of profile_data_prefix.
>>>>
>>>> In most cases this is either set from the directory specified to
>>>> -fprofile-generate=, or to getpwd when a directory is not specified.
>>>> However, the exception is when no directory is specified for
>>>> -fprofile-generate and -auxbase-strip option is used with the absolute
>>>> pathname. In that case the code does not set profile_data_prefix since
>>>> the filenames already have the full path.
>>>>
>>>> In the code that sets __gcov_get_profile_prefix, the fix is to simply
>>>> check if profile_data_prefix is still NULL, and if so just set via
>>>> getpwd.
>>>
>>> Why setting it to getpwd() val? Should it be set to null instead?
>>
>> The specified behavior when no path is given to -fprofile-generate (or
>> -fprofile-dir) is to use the current directory.
>>
>> The case where this was happening was an lto test case, where lto1 was
>> first run in WPA (-fwpa) mode and was emitting the ltrans output to a
>> /tmp/ path (-fltrans-output-list=/tmp/cciR1m1o.ltrans.out). Then lto1
>> was run again in LTRANS mode (-fltrans) with -auxbase-strip
>> /tmp/cciR1m1o.ltrans0.ltrans.o, triggering the problem.
>>
>> Teresa
>>
>>>
>>> David
>>>
>>>>
>>>> Passes regression tests and failure I reproduced. Ok for google branches?
>>>>
>>>> Thanks,
>>>> Teresa
>>>>
>>>> 2013-09-12  Teresa Johnson  <tejohnson@google.com>
>>>>
>>>> * tree-profile.c (tree_init_instrumentation): Handle the case
>>>>         where profile_data_prefix is NULL.
>>>>
>>>> Index: tree-profile.c
>>>> ===================================================================
>>>> --- tree-profile.c (revision 202500)
>>>> +++ tree-profile.c (working copy)
>>>> @@ -470,8 +470,11 @@ tree_init_instrumentation (void)
>>>>                            DECL_ASSEMBLER_NAME (gcov_profile_prefix_decl));
>>>>        TREE_STATIC (gcov_profile_prefix_decl) = 1;
>>>>
>>>> -      prefix_len = strlen (profile_data_prefix);
>>>> -      prefix_string = build_string (prefix_len + 1, profile_data_prefix);
>>>> +      const char *prefix = profile_data_prefix;
>>>> +      if (!prefix)
>>>> +        prefix = getpwd ();
>>>> +      prefix_len = strlen (prefix);
>>>> +      prefix_string = build_string (prefix_len + 1, prefix);
>>>>        TREE_TYPE (prefix_string) = build_array_type
>>>>            (char_type_node, build_index_type
>>>>             (build_int_cst (NULL_TREE, prefix_len)));
>>>>
>>>>
>>>> --
>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
>>
>>
>>
>> --
>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
Teresa Johnson - Sept. 13, 2013, 2:21 p.m.
Testing passes, is the below patch ok for google/4_8?
Thanks, Teresa

On Thu, Sep 12, 2013 at 10:18 PM, Teresa Johnson <tejohnson@google.com> wrote:
> On Thu, Sep 12, 2013 at 2:32 PM, Xinliang David Li <davidxl@google.com> wrote:
>> When absolute path is specified for the object file, no prefix will be
>> prepended to the gcda path. If you record the cwd as in the
>> _gcov_profile_prefix variable, at profile dump time, the prefix will
>> be wrong -- as it is never used.
>
> Yes I think I agree with you now.
>
> Basically, for non-lto compilations, you get the following:
> -fprofile-generate={path}
>    -> no auxbase-strip and profile_data_prefix={path}
> -fprofile-generate -o relative/path/to/file.o
>    -> no auxbase-strip and profile_data_prefix=getpwd()
> -fprofile-generate -o /absolute/path/to/file.o
>    -> auxbase-strip /absolute/path/to/file.o and profile_data_prefix=NULL
>
> But with -flto and -fprofile-generate -o relative/path/to/file.o
>    -> auxbase-strip /tmp/file.ltrans.out and profile_data_prefix=NULL
>
> In the LTO case the gcda files will go into cwd, but not in the case
> just above where the absolute path is given to the object file.
> However, for our purposes we rely on the path being specified to
> -fprofile-generate={path} in places where we query
> __gcov_profile_prefix in order to find the dump directory. Therefore,
> I think it is best to simply record a NULL string as the
> profile_data_prefix value in all cases where profile_data_prefix=NULL.
>
> Here is the patch I am regression testing:
>
> Index: tree-profile.c
> ===================================================================
> --- tree-profile.c (revision 202500)
> +++ tree-profile.c (working copy)
> @@ -470,8 +470,15 @@ tree_init_instrumentation (void)
>                            DECL_ASSEMBLER_NAME (gcov_profile_prefix_decl));
>        TREE_STATIC (gcov_profile_prefix_decl) = 1;
>
> -      prefix_len = strlen (profile_data_prefix);
> -      prefix_string = build_string (prefix_len + 1, profile_data_prefix);
> +      const char null_prefix[] = "\0";
> +      const char *prefix = null_prefix;
> +      prefix_len = 0;
> +      if (profile_data_prefix)
> +        {
> +          prefix_len = strlen (profile_data_prefix);
> +          prefix = profile_data_prefix;
> +        }
> +      prefix_string = build_string (prefix_len + 1, prefix);
>        TREE_TYPE (prefix_string) = build_array_type
>            (char_type_node, build_index_type
>             (build_int_cst (NULL_TREE, prefix_len)));
>
>>
>> David
>>
>> On Thu, Sep 12, 2013 at 2:07 PM, Teresa Johnson <tejohnson@google.com> wrote:
>>> On Thu, Sep 12, 2013 at 1:20 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Thu, Sep 12, 2013 at 1:06 PM, Teresa Johnson <tejohnson@google.com> wrote:
>>>>> After porting r198033 from google/4_7 to google/4_8 a test case failed
>>>>> with an assert when trying to take the strlen of profile_data_prefix.
>>>>>
>>>>> In most cases this is either set from the directory specified to
>>>>> -fprofile-generate=, or to getpwd when a directory is not specified.
>>>>> However, the exception is when no directory is specified for
>>>>> -fprofile-generate and -auxbase-strip option is used with the absolute
>>>>> pathname. In that case the code does not set profile_data_prefix since
>>>>> the filenames already have the full path.
>>>>>
>>>>> In the code that sets __gcov_get_profile_prefix, the fix is to simply
>>>>> check if profile_data_prefix is still NULL, and if so just set via
>>>>> getpwd.
>>>>
>>>> Why setting it to getpwd() val? Should it be set to null instead?
>>>
>>> The specified behavior when no path is given to -fprofile-generate (or
>>> -fprofile-dir) is to use the current directory.
>>>
>>> The case where this was happening was an lto test case, where lto1 was
>>> first run in WPA (-fwpa) mode and was emitting the ltrans output to a
>>> /tmp/ path (-fltrans-output-list=/tmp/cciR1m1o.ltrans.out). Then lto1
>>> was run again in LTRANS mode (-fltrans) with -auxbase-strip
>>> /tmp/cciR1m1o.ltrans0.ltrans.o, triggering the problem.
>>>
>>> Teresa
>>>
>>>>
>>>> David
>>>>
>>>>>
>>>>> Passes regression tests and failure I reproduced. Ok for google branches?
>>>>>
>>>>> Thanks,
>>>>> Teresa
>>>>>
>>>>> 2013-09-12  Teresa Johnson  <tejohnson@google.com>
>>>>>
>>>>> * tree-profile.c (tree_init_instrumentation): Handle the case
>>>>>         where profile_data_prefix is NULL.
>>>>>
>>>>> Index: tree-profile.c
>>>>> ===================================================================
>>>>> --- tree-profile.c (revision 202500)
>>>>> +++ tree-profile.c (working copy)
>>>>> @@ -470,8 +470,11 @@ tree_init_instrumentation (void)
>>>>>                            DECL_ASSEMBLER_NAME (gcov_profile_prefix_decl));
>>>>>        TREE_STATIC (gcov_profile_prefix_decl) = 1;
>>>>>
>>>>> -      prefix_len = strlen (profile_data_prefix);
>>>>> -      prefix_string = build_string (prefix_len + 1, profile_data_prefix);
>>>>> +      const char *prefix = profile_data_prefix;
>>>>> +      if (!prefix)
>>>>> +        prefix = getpwd ();
>>>>> +      prefix_len = strlen (prefix);
>>>>> +      prefix_string = build_string (prefix_len + 1, prefix);
>>>>>        TREE_TYPE (prefix_string) = build_array_type
>>>>>            (char_type_node, build_index_type
>>>>>             (build_int_cst (NULL_TREE, prefix_len)));
>>>>>
>>>>>
>>>>> --
>>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
>>>
>>>
>>>
>>> --
>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
Xinliang David Li - Sept. 13, 2013, 3:07 p.m.
Ok.

David

On Fri, Sep 13, 2013 at 7:21 AM, Teresa Johnson <tejohnson@google.com> wrote:
> Testing passes, is the below patch ok for google/4_8?
> Thanks, Teresa
>
> On Thu, Sep 12, 2013 at 10:18 PM, Teresa Johnson <tejohnson@google.com> wrote:
>> On Thu, Sep 12, 2013 at 2:32 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> When absolute path is specified for the object file, no prefix will be
>>> prepended to the gcda path. If you record the cwd as in the
>>> _gcov_profile_prefix variable, at profile dump time, the prefix will
>>> be wrong -- as it is never used.
>>
>> Yes I think I agree with you now.
>>
>> Basically, for non-lto compilations, you get the following:
>> -fprofile-generate={path}
>>    -> no auxbase-strip and profile_data_prefix={path}
>> -fprofile-generate -o relative/path/to/file.o
>>    -> no auxbase-strip and profile_data_prefix=getpwd()
>> -fprofile-generate -o /absolute/path/to/file.o
>>    -> auxbase-strip /absolute/path/to/file.o and profile_data_prefix=NULL
>>
>> But with -flto and -fprofile-generate -o relative/path/to/file.o
>>    -> auxbase-strip /tmp/file.ltrans.out and profile_data_prefix=NULL
>>
>> In the LTO case the gcda files will go into cwd, but not in the case
>> just above where the absolute path is given to the object file.
>> However, for our purposes we rely on the path being specified to
>> -fprofile-generate={path} in places where we query
>> __gcov_profile_prefix in order to find the dump directory. Therefore,
>> I think it is best to simply record a NULL string as the
>> profile_data_prefix value in all cases where profile_data_prefix=NULL.
>>
>> Here is the patch I am regression testing:
>>
>> Index: tree-profile.c
>> ===================================================================
>> --- tree-profile.c (revision 202500)
>> +++ tree-profile.c (working copy)
>> @@ -470,8 +470,15 @@ tree_init_instrumentation (void)
>>                            DECL_ASSEMBLER_NAME (gcov_profile_prefix_decl));
>>        TREE_STATIC (gcov_profile_prefix_decl) = 1;
>>
>> -      prefix_len = strlen (profile_data_prefix);
>> -      prefix_string = build_string (prefix_len + 1, profile_data_prefix);
>> +      const char null_prefix[] = "\0";
>> +      const char *prefix = null_prefix;
>> +      prefix_len = 0;
>> +      if (profile_data_prefix)
>> +        {
>> +          prefix_len = strlen (profile_data_prefix);
>> +          prefix = profile_data_prefix;
>> +        }
>> +      prefix_string = build_string (prefix_len + 1, prefix);
>>        TREE_TYPE (prefix_string) = build_array_type
>>            (char_type_node, build_index_type
>>             (build_int_cst (NULL_TREE, prefix_len)));
>>
>>>
>>> David
>>>
>>> On Thu, Sep 12, 2013 at 2:07 PM, Teresa Johnson <tejohnson@google.com> wrote:
>>>> On Thu, Sep 12, 2013 at 1:20 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> On Thu, Sep 12, 2013 at 1:06 PM, Teresa Johnson <tejohnson@google.com> wrote:
>>>>>> After porting r198033 from google/4_7 to google/4_8 a test case failed
>>>>>> with an assert when trying to take the strlen of profile_data_prefix.
>>>>>>
>>>>>> In most cases this is either set from the directory specified to
>>>>>> -fprofile-generate=, or to getpwd when a directory is not specified.
>>>>>> However, the exception is when no directory is specified for
>>>>>> -fprofile-generate and -auxbase-strip option is used with the absolute
>>>>>> pathname. In that case the code does not set profile_data_prefix since
>>>>>> the filenames already have the full path.
>>>>>>
>>>>>> In the code that sets __gcov_get_profile_prefix, the fix is to simply
>>>>>> check if profile_data_prefix is still NULL, and if so just set via
>>>>>> getpwd.
>>>>>
>>>>> Why setting it to getpwd() val? Should it be set to null instead?
>>>>
>>>> The specified behavior when no path is given to -fprofile-generate (or
>>>> -fprofile-dir) is to use the current directory.
>>>>
>>>> The case where this was happening was an lto test case, where lto1 was
>>>> first run in WPA (-fwpa) mode and was emitting the ltrans output to a
>>>> /tmp/ path (-fltrans-output-list=/tmp/cciR1m1o.ltrans.out). Then lto1
>>>> was run again in LTRANS mode (-fltrans) with -auxbase-strip
>>>> /tmp/cciR1m1o.ltrans0.ltrans.o, triggering the problem.
>>>>
>>>> Teresa
>>>>
>>>>>
>>>>> David
>>>>>
>>>>>>
>>>>>> Passes regression tests and failure I reproduced. Ok for google branches?
>>>>>>
>>>>>> Thanks,
>>>>>> Teresa
>>>>>>
>>>>>> 2013-09-12  Teresa Johnson  <tejohnson@google.com>
>>>>>>
>>>>>> * tree-profile.c (tree_init_instrumentation): Handle the case
>>>>>>         where profile_data_prefix is NULL.
>>>>>>
>>>>>> Index: tree-profile.c
>>>>>> ===================================================================
>>>>>> --- tree-profile.c (revision 202500)
>>>>>> +++ tree-profile.c (working copy)
>>>>>> @@ -470,8 +470,11 @@ tree_init_instrumentation (void)
>>>>>>                            DECL_ASSEMBLER_NAME (gcov_profile_prefix_decl));
>>>>>>        TREE_STATIC (gcov_profile_prefix_decl) = 1;
>>>>>>
>>>>>> -      prefix_len = strlen (profile_data_prefix);
>>>>>> -      prefix_string = build_string (prefix_len + 1, profile_data_prefix);
>>>>>> +      const char *prefix = profile_data_prefix;
>>>>>> +      if (!prefix)
>>>>>> +        prefix = getpwd ();
>>>>>> +      prefix_len = strlen (prefix);
>>>>>> +      prefix_string = build_string (prefix_len + 1, prefix);
>>>>>>        TREE_TYPE (prefix_string) = build_array_type
>>>>>>            (char_type_node, build_index_type
>>>>>>             (build_int_cst (NULL_TREE, prefix_len)));
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
>>>>
>>>>
>>>>
>>>> --
>>>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
>>
>>
>>
>> --
>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413
>
>
>
> --
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

Patch

Index: tree-profile.c
===================================================================
--- tree-profile.c (revision 202500)
+++ tree-profile.c (working copy)
@@ -470,8 +470,15 @@  tree_init_instrumentation (void)
                           DECL_ASSEMBLER_NAME (gcov_profile_prefix_decl));
       TREE_STATIC (gcov_profile_prefix_decl) = 1;

-      prefix_len = strlen (profile_data_prefix);
-      prefix_string = build_string (prefix_len + 1, profile_data_prefix);
+      const char null_prefix[] = "\0";
+      const char *prefix = null_prefix;
+      prefix_len = 0;
+      if (profile_data_prefix)
+        {
+          prefix_len = strlen (profile_data_prefix);
+          prefix = profile_data_prefix;
+        }
+      prefix_string = build_string (prefix_len + 1, prefix);
       TREE_TYPE (prefix_string) = build_array_type
           (char_type_node, build_index_type
            (build_int_cst (NULL_TREE, prefix_len)));