Instantiate a single source highlighter
Commit Message
It occurred to me that there's no reason to make a new source
highlighter each time gdb needs to highlight some source code.
Instead, a single one can be created and then simply reused each time.
This patch implements this idea. Tested on x86-64 Fedora 29.
gdb/ChangeLog
2019-06-18 Tom Tromey <tromey@adacore.com>
* source-cache.c (highlighter): New global.
(source_cache::get_source_lines): Create a highlighter on demand.
---
gdb/ChangeLog | 5 +++++
gdb/source-cache.c | 19 ++++++++++++++++---
2 files changed, 21 insertions(+), 3 deletions(-)
Comments
>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:
Tom> +#include <srchilite/parserexception.h>
Oops, I didn't mean to include this hunk.
This came from an attempt to let gdb try to use the rust language from
source highlight (new in git -- not in a release), and then fall back if
it isn't found. However, that's causing crashes in the library and so
isn't quite ready...
Meanwhile I've removed this include from my patch.
Tom
On 6/18/19 8:40 PM, Tom Tromey wrote:
> It occurred to me that there's no reason to make a new source
> highlighter each time gdb needs to highlight some source code.
> Instead, a single one can be created and then simply reused each time.
>
> This patch implements this idea. Tested on x86-64 Fedora 29.
>
Assuming we won't need this from different threads anytime soon,
seems fine. Comments below.
> gdb/ChangeLog
> 2019-06-18 Tom Tromey <tromey@adacore.com>
>
> * source-cache.c (highlighter): New global.
> (source_cache::get_source_lines): Create a highlighter on demand.
> ---
> gdb/ChangeLog | 5 +++++
> gdb/source-cache.c | 19 ++++++++++++++++---
> 2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/source-cache.c b/gdb/source-cache.c
> index 2d5b549d971..0fa456116b4 100644
> --- a/gdb/source-cache.c
> +++ b/gdb/source-cache.c
> @@ -33,6 +33,7 @@
> #include <sstream>
> #include <srchilite/sourcehighlight.h>
> #include <srchilite/langmap.h>
> +#include <srchilite/parserexception.h>
> #endif
>
> /* The number of source files we'll cache. */
> @@ -43,6 +44,14 @@
>
> source_cache g_source_cache;
>
> +/* 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. */
> +#ifdef HAVE_SOURCE_HIGHLIGHT
> +static srchilite::SourceHighlight *highlighter;
> +#endif
Should it be a unique_ptr so that valgrind doesn't complain about
it leaking when gdb exits?
> +
> /* See source-cache.h. */
>
> bool
> @@ -209,11 +218,15 @@ source_cache::get_source_lines (struct symtab *s, int first_line,
> use-after-free. */
> fullname = symtab_to_fullname (s);
> }
> - srchilite::SourceHighlight highlighter ("esc.outlang");
> - highlighter.setStyleFile("esc.style");
> +
> + if (highlighter == nullptr)
> + {
> + highlighter = new srchilite::SourceHighlight ("esc.outlang");
> + highlighter->setStyleFile("esc.style");
Preexisting, but missing space before parens.
> + }
To keep the variable's definition and initialization close by,
I'd add a get_highlighter function:
static std::unique_ptr<srchilite::SourceHighlight> highlighter;
static srchilite::SourceHighlight *
get_highlighter ()
{
if (highlighter == nullptr)
{
highlighter = new srchilite::SourceHighlight ("esc.outlang");
highlighter->setStyleFile ("esc.style");
}
return highlighter.get ();
}
>
> std::ostringstream output;
> - highlighter.highlight (input, output, lang_name, fullname);
> + highlighter->highlight (input, output, lang_name, fullname);
>
> source_text result = { fullname, output.str () };
> m_source_map.push_back (std::move (result));
Thanks,
Pedro Alves
On Tue, 2019-06-18 at 21:20 +0100, Pedro Alves wrote:
> > +/* 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. */
> > +#ifdef HAVE_SOURCE_HIGHLIGHT
> > +static srchilite::SourceHighlight *highlighter;
> > +#endif
>
> Should it be a unique_ptr so that valgrind doesn't complain about
> it leaking when gdb exits?
As highlighter is a global static variable,
when GDB exits, valgrind will consider the memory as reachable
and not a leak.
So, from only a valgrind point of view, there is no reason to have
GDB releasing this memory automatically when exiting.
Philippe
@@ -33,6 +33,7 @@
#include <sstream>
#include <srchilite/sourcehighlight.h>
#include <srchilite/langmap.h>
+#include <srchilite/parserexception.h>
#endif
/* The number of source files we'll cache. */
@@ -43,6 +44,14 @@
source_cache g_source_cache;
+/* 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. */
+#ifdef HAVE_SOURCE_HIGHLIGHT
+static srchilite::SourceHighlight *highlighter;
+#endif
+
/* See source-cache.h. */
bool
@@ -209,11 +218,15 @@ source_cache::get_source_lines (struct symtab *s, int first_line,
use-after-free. */
fullname = symtab_to_fullname (s);
}
- srchilite::SourceHighlight highlighter ("esc.outlang");
- highlighter.setStyleFile("esc.style");
+
+ if (highlighter == nullptr)
+ {
+ highlighter = new srchilite::SourceHighlight ("esc.outlang");
+ highlighter->setStyleFile("esc.style");
+ }
std::ostringstream output;
- highlighter.highlight (input, output, lang_name, fullname);
+ highlighter->highlight (input, output, lang_name, fullname);
source_text result = { fullname, output.str () };
m_source_map.push_back (std::move (result));