Re: [GOOGLE] Fix gcda build info support

[Teresa Johnson <tejohnson at google dot com>]

On Thu, Sep 11, 2014 at 10:17 AM, Xinliang David Li <davidxl@google.com> wrote:
> Yes, that is what I meant.
>
> David
>
> On Thu, Sep 11, 2014 at 10:09 AM, Teresa Johnson <tejohnson@google.com> wrote:
>> On Wed, Sep 10, 2014 at 3:31 PM, Xinliang David Li <davidxl@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 <tejohnson@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  <tejohnson@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 ""