Re: [PATCH 1/3] gcov: Cache source files

Jørgen Kvalsvik Fri, 23 Aug 2024 00:49:54 -0700

On 8/23/24 09:39, Jan Hubicka wrote:

Hi,

         1:    4:int notmain(const char *entity)
         -: == inlined from hello.h ==
         1:    6:  if (s)
branch  0 taken 0 (fallthrough)
branch  1 taken 1
     #####:    7:    printf ("hello, %s!\n", s);
     %%%%%:    7-block 3
call    0 never executed
         -:    8:  else
         1:    9:    printf ("hello, world!\n");
         1:    9-block 4
call    0 returned 1
         1:   10:  return 0;
         1:   10-block 5
         -: == inlined from hello.h (end) ==
         -:    5:{
         1:    6:  return hello (entity);
         1:    6-block 7
         -:    7:}


This indeed looks like a reasonable goal.

gcc/ChangeLog:

        * gcov.cc (release_structures): Release source_lines.
        (slurp): New function.
        (output_lines): Read sources with slurp.
---
  gcc/gcov.cc | 70 ++++++++++++++++++++++++++++++++++++++++-------------
  1 file changed, 53 insertions(+), 17 deletions(-)

diff --git a/gcc/gcov.cc b/gcc/gcov.cc
index e76a314041c..19019f404ee 100644
--- a/gcc/gcov.cc
+++ b/gcc/gcov.cc
@@ -550,6 +550,11 @@ static vector<name_map> names;
     a file being read multiple times.  */
  static vector<char *> processed_files;

+/* The contents of a source file. The nth SOURCE_LINES entry is the

+   contents of the nth SOURCES, or empty if it has not or could not be
+   read.  */
+static vector<vector<const char *>*> source_lines;
+
  /* This holds data summary information.  */

static unsigned object_runs;

@@ -762,6 +767,8 @@ static string make_gcov_file_name (const char *, const char 
*);
  static char *mangle_name (const char *);
  static void release_structures (void);
  extern int main (int, char **);
+static const vector<const char *>&
+slurp (const source_info &src, FILE *gcov_file, const char *line_start);

function_info::function_info (): m_name (NULL), m_demangled_name (NULL),

    ident (0), lineno_checksum (0), cfg_checksum (0), has_catch (0),
@@ -1804,6 +1811,15 @@ release_structures (void)
         it != functions.end (); it++)
      delete (*it);

+ for (vector<const char *> *lines : source_lines)

+    {
+      if (lines)
+       for (const char *line : *lines)
+         free (const_cast <char*> (line));
+      delete (lines);
+    }
+  source_lines.resize (0);
+
    for (fnfilter &filter : filters)
      regfree (&filter.regex);

@@ -3246,6 +3262,41 @@ read_line (FILE *file)

    return pos ? string : NULL;
  }

+/* Get the vector with the contents SRC, possibly from a cache. If

+   the reading fails, a message prefixed with LINE_START is written to
+   GCOV_FILE.  */
+static const vector<const char *>&
+slurp (const source_info &src, FILE *gcov_file,
+       const char *line_start)
+{
+  if (source_lines.size () <= src.index)
+    source_lines.resize (src.index + 1);
+
+  /* Store vector pointers so that the returned references remain
+     stable and won't be broken by successive calls to slurp.  */
+  if (!source_lines[src.index])
+    source_lines[src.index] = new vector<const char *> ();
+
+  if (!source_lines[src.index]->empty ())
+    return *source_lines[src.index];
+
+  FILE *source_file = fopen (src.name, "r");
+  if (!source_file)
+    fnotice (stderr, "Cannot open source file %s\n", src.name);
+  else if (src.file_time == 0)
+    fprintf (gcov_file, "%sSource is newer than graph\n", line_start);
+
+  const char *retval;
+  vector<const char *> &lines = *source_lines[src.index];
+  if (source_file)
+    while ((retval = read_line (source_file)))
+      lines.push_back (xstrdup (retval));
+
+  if (source_file)
+    fclose (source_file);
+  return lines;

So this is basically going to read all sources needed for single
compilation unit (.cc file) into memory at once.  I guess that is OK.
I wonder if we don't want to do something like mmap the source and then
set up source_lines array instead of reading every like into spearate
xstrduped chunk?
But that could be done incrementally, so th epatch is OK.

Yes, which is what it did anyway (if not on purpose). Moving to mmapmight also be a nice change, and maybe use a separate string_view likeindex to easily move between the lines. But as you say, that is anincremental change.


Thanks,
Jørgen

Honza

+}
+
  /* Pad string S with spaces from left to have total width equal to 9.  */

static void

@@ -3435,9 +3486,6 @@ output_lines (FILE *gcov_file, const source_info *src)
  #define  DEFAULT_LINE_START "        -:    0:"
  #define FN_SEPARATOR "------------------\n"

- FILE *source_file;

-  const char *retval;
-
    /* Print colorization legend.  */
    if (flag_use_colors)
      fprintf (gcov_file, "%s",
@@ -3464,17 +3512,8 @@ output_lines (FILE *gcov_file, const source_info *src)
        fprintf (gcov_file, DEFAULT_LINE_START "Runs:%u\n", object_runs);
      }

- source_file = fopen (src->name, "r");

-  if (!source_file)
-    fnotice (stderr, "Cannot open source file %s\n", src->name);
-  else if (src->file_time == 0)
-    fprintf (gcov_file, DEFAULT_LINE_START "Source is newer than graph\n");
-
-  vector<const char *> source_lines;
-  if (source_file)
-    while ((retval = read_line (source_file)) != NULL)
-      source_lines.push_back (xstrdup (retval));
-
+  const vector<const char *> &source_lines = slurp (*src, gcov_file,
+                                                   DEFAULT_LINE_START);
    unsigned line_start_group = 0;
    vector<function_info *> *fns;
    unsigned filtered_line_end = !filters.empty () ? 0 : source_lines.size ();
@@ -3596,7 +3635,4 @@ output_lines (FILE *gcov_file, const source_info *src)
          line_start_group = 0;
        }
      }
-
-  if (source_file)
-    fclose (source_file);
  }
--
2.39.2

Re: [PATCH 1/3] gcov: Cache source files

Reply via email to