[2/4] Save plain text in the source cache

Message ID 20190726133422.5896-3-tromey@adacore.com
State New
Headers show
Series
  • Fewer calls to "open" when stepping
Related show

Commit Message

Tom Tromey July 26, 2019, 1:34 p.m.
Currently the source cache will only store highlighted text.  However,
there's no reason it could not also store plain text, when styling is
turned off.

This patch makes this change.  This also simplifies the source cache
code somewhat.

gdb/ChangeLog
2019-07-26  Tom Tromey  <tromey@adacore.com>

	* source-cache.c (source_cache::get_plain_source_lines):
	Remove "first_line" and "last_line" parameters.
	(source_cache::get_source_lines): Cache plain text.
	* source-cache.h (class source_cache)
	<get_plain_source_lines>: Update.
---
 gdb/ChangeLog      |   8 ++++
 gdb/source-cache.c | 114 +++++++++++++++++----------------------------
 gdb/source-cache.h |   9 ++--
 3 files changed, 55 insertions(+), 76 deletions(-)

-- 
2.20.1

Comments

H.J. Lu Oct. 22, 2019, 11:39 p.m. | #1
On Fri, Jul 26, 2019 at 6:34 AM Tom Tromey <tromey@adacore.com> wrote:
>

> Currently the source cache will only store highlighted text.  However,

> there's no reason it could not also store plain text, when styling is

> turned off.

>

> This patch makes this change.  This also simplifies the source cache

> code somewhat.

>

> gdb/ChangeLog

> 2019-07-26  Tom Tromey  <tromey@adacore.com>

>

>         * source-cache.c (source_cache::get_plain_source_lines):

>         Remove "first_line" and "last_line" parameters.

>         (source_cache::get_source_lines): Cache plain text.

>         * source-cache.h (class source_cache)

>         <get_plain_source_lines>: Update.


This caused:

https://sourceware.org/bugzilla/show_bug.cgi?id=25126


-- 
H.J.
H.J. Lu Oct. 23, 2019, 6:55 p.m. | #2
On Tue, Oct 22, 2019 at 4:39 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>

> On Fri, Jul 26, 2019 at 6:34 AM Tom Tromey <tromey@adacore.com> wrote:

> >

> > Currently the source cache will only store highlighted text.  However,

> > there's no reason it could not also store plain text, when styling is

> > turned off.

> >

> > This patch makes this change.  This also simplifies the source cache

> > code somewhat.

> >

> > gdb/ChangeLog

> > 2019-07-26  Tom Tromey  <tromey@adacore.com>

> >

> >         * source-cache.c (source_cache::get_plain_source_lines):

> >         Remove "first_line" and "last_line" parameters.

> >         (source_cache::get_source_lines): Cache plain text.

> >         * source-cache.h (class source_cache)

> >         <get_plain_source_lines>: Update.

>

> This caused:

>

> https://sourceware.org/bugzilla/show_bug.cgi?id=25126

>

>


This works for my test case.


-- 
H.J.
---
diff --git a/gdb/symfile.c b/gdb/symfile.c
index f74c6de596..95dba37eee 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2518,6 +2518,9 @@ reread_symbols (void)
        automatically recreated by sym_read.  */
     free_objfile_separate_debug (objfile);

+    /* Clear the staled source cache.  */
+    forget_cached_source_info ();
+
     /* Remove any references to this objfile in the global
        value lists.  */
     preserve_values (objfile);
Andreas Schwab Oct. 24, 2019, 8:17 a.m. | #3
On Okt 23 2019, H.J. Lu wrote:

> +    /* Clear the staled source cache.  */


s/staled/stale/

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Tom Tromey Oct. 24, 2019, 6:59 p.m. | #4
> This works for my test case.


Thank you.

This is ok with a ChangeLog entry, and with a fix for the typo that
Andreas pointed out.

Tom
H.J. Lu Oct. 24, 2019, 10:46 p.m. | #5
On Thu, Oct 24, 2019 at 11:59 AM Tom Tromey <tromey@adacore.com> wrote:
>

> > This works for my test case.

>

> Thank you.

>

> This is ok with a ChangeLog entry, and with a fix for the typo that

> Andreas pointed out.

>

> Tom


This is what I checked in.

Thanks.

-- 
H.J.
From 7b71fc971bd31b27cce8b883007ad467a7567499 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 24 Oct 2019 15:43:21 -0700
Subject: [PATCH] Call forget_cached_source_info to clear the stale source
 cache

Clear the stale source cache when re-reading symbols.

	PR gdb/25126
	* symfile.c (reread_symbols): Call forget_cached_source_info to
	clear the stale source cache.
---
 gdb/ChangeLog | 6 ++++++
 gdb/symfile.c | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 664000add4..8447542d8e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-10-24  H.J. Lu  <hongjiu.lu@intel.com>
+
+	PR gdb/25126
+	* symfile.c (reread_symbols): Call forget_cached_source_info to
+	clear the stale source cache.
+
 2019-10-24  Christian Biesinger  <cbiesinger@google.com>
 
 	* configure: Regenerate.
diff --git a/gdb/symfile.c b/gdb/symfile.c
index f74c6de596..d2ed1ccacd 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2518,6 +2518,9 @@ reread_symbols (void)
 	     automatically recreated by sym_read.  */
 	  free_objfile_separate_debug (objfile);
 
+	  /* Clear the stale source cache.  */
+	  forget_cached_source_info ();
+
 	  /* Remove any references to this objfile in the global
 	     value lists.  */
 	  preserve_values (objfile);

Patch

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index f0cb6b80059..0cc2076258c 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -31,7 +31,6 @@ 
    when GDB is linked.  Happens, e.g., in the MinGW build.  */
 #undef open
 #undef close
-#include <fstream>
 #include <sstream>
 #include <srchilite/sourcehighlight.h>
 #include <srchilite/langmap.h>
@@ -48,33 +47,19 @@  source_cache g_source_cache;
 /* See source-cache.h.  */
 
 bool
-source_cache::get_plain_source_lines (struct symtab *s, int first_line,
-				      int last_line, std::string *lines)
+source_cache::get_plain_source_lines (struct symtab *s, std::string *lines)
 {
   scoped_fd desc (open_source_file_with_line_charpos (s));
   if (desc.get () < 0)
     return false;
 
-  if (first_line < 1 || first_line > s->nlines || last_line < 1)
-    return false;
+  struct stat st;
 
-  if (lseek (desc.get (), s->line_charpos[first_line - 1], SEEK_SET) < 0)
+  if (fstat (desc.get (), &st) < 0)
     perror_with_name (symtab_to_filename_for_display (s));
 
-  int last_charpos;
-  if (last_line >= s->nlines)
-    {
-      struct stat st;
-
-      if (fstat (desc.get (), &st) < 0)
-	perror_with_name (symtab_to_filename_for_display (s));
-      /* We could cache this in line_charpos... */
-      last_charpos = st.st_size;
-    }
-  else
-    last_charpos = s->line_charpos[last_line];
-
-  lines->resize (last_charpos - s->line_charpos[first_line - 1]);
+  /* We could cache this in line_charpos... */
+  lines->resize (st.st_size);
   if (myread (desc.get (), &(*lines)[0], lines->size ()) < 0)
     perror_with_name (symtab_to_filename_for_display (s));
 
@@ -182,70 +167,57 @@  source_cache::get_source_lines (struct symtab *s, int first_line,
   if (first_line < 1 || last_line < 1 || first_line > last_line)
     return false;
 
-#ifdef HAVE_SOURCE_HIGHLIGHT
-  if (source_styling && gdb_stdout->can_emit_style_escape ())
-    {
-      const char *fullname = symtab_to_fullname (s);
+  std::string fullname = symtab_to_fullname (s);
 
-      for (const auto &item : m_source_map)
+  for (const auto &item : m_source_map)
+    {
+      if (item.fullname == fullname)
 	{
-	  if (item.fullname == fullname)
-	    {
-	      *lines = extract_lines (item.contents, first_line, last_line);
-	      return true;
-	    }
+	  *lines = extract_lines (item.contents, first_line, last_line);
+	  return true;
 	}
+    }
 
+  std::string contents;
+  if (!get_plain_source_lines (s, &contents))
+    return false;
+
+#ifdef HAVE_SOURCE_HIGHLIGHT
+  if (source_styling && gdb_stdout->can_emit_style_escape ())
+    {
       const char *lang_name = get_language_name (SYMTAB_LANGUAGE (s));
       if (lang_name != nullptr)
 	{
-	  std::ifstream input (fullname);
-	  if (input.is_open ())
+	  /* The global source highlight object, or null if one was
+	     never constructed.  This is stored here rather than in
+	     the class so that we don't need to include anything or do
+	     conditional compilation in source-cache.h.  */
+	  static srchilite::SourceHighlight *highlighter;
+
+	  if (highlighter == nullptr)
 	    {
-	      /* The global source highlight object, or null if one
-		 was never constructed.  This is stored here rather
-		 than in the class so that we don't need to include
-		 anything or do conditional compilation in
-		 source-cache.h.  */
-	      static srchilite::SourceHighlight *highlighter;
-
-	      if (s->line_charpos == 0)
-		{
-		  scoped_fd desc (open_source_file_with_line_charpos (s));
-		  if (desc.get () < 0)
-		    return false;
-
-		  /* FULLNAME points to a value owned by the symtab
-		     (symtab::fullname).  Calling open_source_file reallocates
-		     that value, so we must refresh FULLNAME to avoid a
-		     use-after-free.  */
-		  fullname = symtab_to_fullname (s);
-		}
-
-	      if (highlighter == nullptr)
-		{
-		  highlighter = new srchilite::SourceHighlight ("esc.outlang");
-		  highlighter->setStyleFile ("esc.style");
-		}
-
-	      std::ostringstream output;
-	      highlighter->highlight (input, output, lang_name, fullname);
-
-	      source_text result = { fullname, output.str () };
-	      m_source_map.push_back (std::move (result));
-
-	      if (m_source_map.size () > MAX_ENTRIES)
-		m_source_map.erase (m_source_map.begin ());
-
-	      *lines = extract_lines (m_source_map.back ().contents,
-				      first_line, last_line);
-	      return true;
+	      highlighter = new srchilite::SourceHighlight ("esc.outlang");
+	      highlighter->setStyleFile ("esc.style");
 	    }
+
+	  std::istringstream input (contents);
+	  std::ostringstream output;
+	  highlighter->highlight (input, output, lang_name, fullname);
+
+	  contents = output.str ();
 	}
     }
 #endif /* HAVE_SOURCE_HIGHLIGHT */
 
-  return get_plain_source_lines (s, first_line, last_line, lines);
+  source_text result = { std::move (fullname), std::move (contents) };
+  m_source_map.push_back (std::move (result));
+
+  if (m_source_map.size () > MAX_ENTRIES)
+    m_source_map.erase (m_source_map.begin ());
+
+  *lines = extract_lines (m_source_map.back ().contents,
+			  first_line, last_line);
+  return true;
 }
 
 #if GDB_SELF_TEST
diff --git a/gdb/source-cache.h b/gdb/source-cache.h
index a00efbf3fba..0c8b14e483e 100644
--- a/gdb/source-cache.h
+++ b/gdb/source-cache.h
@@ -58,11 +58,10 @@  private:
     std::string contents;
   };
 
-  /* A helper function for get_source_lines that is used when the
-     source lines are not highlighted.  The arguments and return value
-     are as for get_source_lines.  */
-  bool get_plain_source_lines (struct symtab *s, int first_line,
-			       int last_line, std::string *lines_out);
+  /* A helper function for get_source_lines reads a source file.
+     Returns false on error.  If no error, the contents of the file
+     are put into *LINES_OUT, and returns true.  */
+  bool get_plain_source_lines (struct symtab *s, std::string *lines_out);
 
   /* The contents of the cache.  */
   std::vector<source_text> m_source_map;