ok.
David
On Thu, Sep 11, 2014 at 11:49 AM, Teresa Johnson <tejohn...@google.com> wrote:
> On Thu, Sep 11, 2014 at 10:17 AM, Xinliang David Li <davi...@google.com>
> wrote:
>> Yes, that is what I meant.
>>
>> David
>>
>> On Thu, Sep 11, 2014 at 10:09 AM, Teresa Johnson <tejohn...@google.com>
>> wrote:
>>> On Wed, Sep 10, 2014 at 3:31 PM, Xinliang David Li <davi...@google.com>
>>> wrote:
>>>> Can you share the buildinfo reader code with the merger by defininig
>>>> some hooks for different callbacks?
>>>
>>> Do you mean the two blobs of code guarded by 'if (tag ==
>>> GCOV_TAG_BUILD_INFO)' that I added here and the existing one in
>>> gcov_exit_merge_gcda further down in the same file? Sure, I could
>>> outline that and pass in the gi_ptr for the merger case. Let me know
>>> if you meant something else.
>>>
>>> Teresa
>>>
>>>>
>>>> David
>>>>
>>>> On Wed, Sep 10, 2014 at 10:24 AM, Teresa Johnson <tejohn...@google.com>
>>>> wrote:
>>>>> While porting recent support for a build info section in the gcda from
>>>>> google/4_8 to 4_9 and doing manual testing, I discovered that it does
>>>>> not interact well with the COMDAT fixup handling. This patch fixes the
>>>>> issue, and adds a test case that exposes the problem without the fix.
>>>>>
>>>>> Here is the google/4_8 patch - I plan to commit there first then port
>>>>> it along with the original build info patch to 4_9.
>>>>>
>>>>> Passes regression tests - ok for google branches?
>>>>>
>>>>> Thanks,
>>>>> Teresa
>
> Updated patch below. Ok for google branches?
>
> 2014-09-11 Teresa Johnson <tejohn...@google.com>
>
> libgcc:
> * libgcc/libgcov-driver.c (scan_build_info): New function.
> * libgcov-driver.c (gcov_scan_to_function_data): Rename from
> gcov_scan_summary_end, scan past BUILD_INFO section.
> (gcov_dump_module_info): Rename gcov_scan_summary_end to
> gcov_scan_to_function_data, outline BUILD_INFO scan.
>
> gcc/testsuite:
> * g++.dg/tree-prof/lipo/buildinfo.txt: Input for
> -fprofile-generate-buildinfo option.
> * g++.dg/tree-prof/lipo/comdat_fixup_0.C: New test.
> * g++.dg/tree-prof/lipo/comdat_fixup_1.C: Ditto.
> * g++.dg/tree-prof/lipo/comdat_fixup_2.C: Ditto.
> * g++.dg/tree-prof/lipo/comdat_fixup.h: Ditto.
> * lib/profopt.exp: Declare srcdir for use in test options.
>
> Index: libgcc/libgcov-driver.c
> ===================================================================
> --- libgcc/libgcov-driver.c (revision 214976)
> +++ libgcc/libgcov-driver.c (working copy)
> @@ -427,12 +427,53 @@ struct gcov_filename_aux{
> /* Including system dependent components. */
> #include "libgcov-driver-system.c"
>
> +/* Scan through the build info section at the current position in the
> + open gcda file, assuming its tag has already been written.
> + Return 0 on success, -1 on error. */
> +static int
> +scan_build_info (struct gcov_info *gi_ptr)
> +{
> + gcov_unsigned_t i, length;
> + gcov_unsigned_t num_strings = 0;
> +
> + length = gcov_read_unsigned ();
> + char **build_info_strings = gcov_read_build_info (length, &num_strings);
> + if (!build_info_strings)
> + {
> + gcov_error ("profiling:%s:Error reading build info\n", gi_filename);
> + return -1;
> + }
> + if (!gi_ptr->build_info)
> + {
> + gcov_error ("profiling:%s:Mismatched build info sections, expected "
> + "none, found %u strings)\n", gi_filename, num_strings);
> + return -1;
> + }
> +
> + for (i = 0; i < num_strings; i++)
> + {
> + if (strcmp (build_info_strings[i], gi_ptr->build_info[i]))
> + {
> + gcov_error ("profiling:%s:Mismatched build info string "
> + "(expected %s, read %s)\n",
> + gi_filename, gi_ptr->build_info[i],
> + build_info_strings[i]);
> + return -1;
> + }
> + free (build_info_strings[i]);
> + }
> + free (build_info_strings);
> + return 0;
> +}
> +
> /* Scan through the current open gcda file corresponding to GI_PTR
> - to locate the end position of the last summary, returned in
> - SUMMARY_END_POS_P. Return 0 on success, -1 on error. */
> + to locate the end position just before function data should be rewritten,
> + returned in SUMMARY_END_POS_P. E.g. scan past the last summary and other
> + sections that won't be rewritten, like the build info. Return 0 on
> success,
> + -1 on error. */
> static int
> -gcov_scan_summary_end (struct gcov_info *gi_ptr,
> - gcov_position_t *summary_end_pos_p)
> +gcov_scan_to_function_data (struct gcov_info *gi_ptr,
> + gcov_position_t *summary_end_pos_p)
> {
> gcov_unsigned_t tag, version, stamp;
> tag = gcov_read_unsigned ();
> @@ -467,6 +508,18 @@ static int
> return -1;
> }
>
> + /* If there is a build info section, scan past it as well. */
> + if (tag == GCOV_TAG_BUILD_INFO)
> + {
> + if (scan_build_info (gi_ptr) < 0)
> + return -1;
> +
> + *summary_end_pos_p = gcov_position ();
> + tag = gcov_read_unsigned ();
> + }
> + /* The next section should be the function counters. */
> + gcc_assert (tag == GCOV_TAG_FUNCTION);
> +
> return 0;
> }
>
> @@ -484,7 +537,7 @@ gcov_exit_merge_gcda (struct gcov_info *gi_ptr,
> struct gcov_parameter_value **merged_parameters)
> {
> gcov_unsigned_t tag, length, version, stamp;
> - unsigned t_ix, f_ix, i;
> + unsigned t_ix, f_ix;
> int error = 0;
> struct gcov_summary_buffer **sum_tail = &sum_buffer;
>
> @@ -539,35 +592,9 @@ gcov_exit_merge_gcda (struct gcov_info *gi_ptr,
>
> if (tag == GCOV_TAG_BUILD_INFO)
> {
> - length = gcov_read_unsigned ();
> - gcov_unsigned_t num_strings = 0;
> - char **build_info_strings = gcov_read_build_info (length,
> &num_strings);
> - if (!build_info_strings)
> - {
> - gcov_error ("profiling:%s:Error reading build info\n",
> gi_filename);
> - return -1;
> - }
> - if (!gi_ptr->build_info)
> - {
> - gcov_error ("profiling:%s:Mismatched build info sections, expected
> "
> - "none, found %u strings)\n", gi_filename, num_strings);
> - return -1;
> - }
> + if (scan_build_info (gi_ptr) < 0)
> + return -1;
>
> - for (i = 0; i < num_strings; i++)
> - {
> - if (strcmp (build_info_strings[i], gi_ptr->build_info[i]))
> - {
> - gcov_error ("profiling:%s:Mismatched build info string "
> - "(expected %s, read %s)\n",
> - gi_filename, gi_ptr->build_info[i],
> - build_info_strings[i]);
> - return -1;
> - }
> - free (build_info_strings[i]);
> - }
> - free (build_info_strings);
> -
> /* Since the stamps matched if we got here, this should be from
> the same compilation and the build info strings should match. */
> tag = gcov_read_unsigned ();
> @@ -1031,10 +1058,10 @@ gcov_dump_module_info (struct gcov_filename_aux *g
>
> if (changed)
> {
> - /* Scan file to find the end of the summary section, which is
> + /* Scan file to find the start of the function section, which is
> where we will start re-writing the counters. */
> gcov_position_t summary_end_pos;
> - if (gcov_scan_summary_end (gi_ptr, &summary_end_pos) == -1)
> + if (gcov_scan_to_function_data (gi_ptr, &summary_end_pos) == -1)
> gcov_error ("profiling:%s:Error scanning summaries\n",
> gi_filename);
> else
> Index: gcc/testsuite/g++.dg/tree-prof/lipo/buildinfo.txt
> ===================================================================
> --- gcc/testsuite/g++.dg/tree-prof/lipo/buildinfo.txt (revision 0)
> +++ gcc/testsuite/g++.dg/tree-prof/lipo/buildinfo.txt (revision 0)
> @@ -0,0 +1 @@
> +Test -fprofile-generate-buildinfo option
> Index: gcc/testsuite/g++.dg/tree-prof/lipo/comdat_fixup_0.C
> ===================================================================
> --- gcc/testsuite/g++.dg/tree-prof/lipo/comdat_fixup_0.C (revision 0)
> +++ gcc/testsuite/g++.dg/tree-prof/lipo/comdat_fixup_0.C (revision 0)
> @@ -0,0 +1,9 @@
> +/* { dg-options "-O2 -fno-inline
> -fprofile-generate-buildinfo=$srcdir/g++.dg/tree-prof/lipo/buildinfo.txt"
> } */
> +#include <stdio.h>
> +
> +extern int foo1(int x);
> +extern int foo2(int x);
> +int main()
> +{
> + printf ("Result = %d\n", foo1(1) + foo2(1));
> +}
> Index: gcc/testsuite/g++.dg/tree-prof/lipo/comdat_fixup_1.C
> ===================================================================
> --- gcc/testsuite/g++.dg/tree-prof/lipo/comdat_fixup_1.C (revision 0)
> +++ gcc/testsuite/g++.dg/tree-prof/lipo/comdat_fixup_1.C (revision 0)
> @@ -0,0 +1,7 @@
> +/* { dg-options "-O2 -fno-inline" } */
> +#include "comdat_fixup.h"
> +int foo1(int x)
> +{
> + Foo f;
> + return f.foo(x);
> +}
> Index: gcc/testsuite/g++.dg/tree-prof/lipo/comdat_fixup_2.C
> ===================================================================
> --- gcc/testsuite/g++.dg/tree-prof/lipo/comdat_fixup_2.C (revision 0)
> +++ gcc/testsuite/g++.dg/tree-prof/lipo/comdat_fixup_2.C (revision 0)
> @@ -0,0 +1,7 @@
> +/* { dg-options "-O2 -fno-inline" } */
> +#include "comdat_fixup.h"
> +int foo2(int x)
> +{
> + Foo f;
> + return f.foo(x);
> +}
> Index: gcc/testsuite/g++.dg/tree-prof/lipo/comdat_fixup.h
> ===================================================================
> --- gcc/testsuite/g++.dg/tree-prof/lipo/comdat_fixup.h (revision 0)
> +++ gcc/testsuite/g++.dg/tree-prof/lipo/comdat_fixup.h (revision 0)
> @@ -0,0 +1,5 @@
> +class Foo
> +{
> + public:
> + int foo(int x) { return x; }
> +};
> Index: gcc/testsuite/lib/profopt.exp
> ===================================================================
> --- gcc/testsuite/lib/profopt.exp (revision 214976)
> +++ gcc/testsuite/lib/profopt.exp (working copy)
> @@ -169,6 +169,8 @@ proc profopt-final-code { which final_code name }
> # SRC is the full pathname of the testcase.
> #
> proc profopt-get-options { src } {
> + global srcdir
> +
> # dg-options sets a variable called dg-extra-tool-flags.
> set dg-extra-tool-flags ""
