| Message ID | CAAe5K+WKck0n-pfgztGHX_JnfuUOKwJFAOMzn6UfA0cFUjjgPw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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)));