diff mbox

Add Rust support to source highlighting

Message ID 20190624220539.16360-1-tom@tromey.com
State New
Headers show

Commit Message

Tom Tromey June 24, 2019, 10:05 p.m. UTC
Currently, no release of GNU Source Highlight supports Rust.  However,
I've checked in a patch to do so there, and I plan to make a new
release sometime this summer.

This patch prepares gdb for that by adding support for Rust to the
source highlighting code.

Because Source Highlight will throw an exception if the language is
unrecognized, this also changes gdb to ignore exceptions here.  This
will cause gdb to fall back to un-highlighted source text.

Tested with the current and development versions of Source Highlight.

gdb/ChangeLog
2019-06-24  Tom Tromey  <tom@tromey.com>

	* source-cache.c (get_language_name): Handle rust.
	(source_cache::get_source_lines): Ignore highlighting exceptions.
---
 gdb/ChangeLog      |  5 +++++
 gdb/source-cache.c | 33 ++++++++++++++++++++++-----------
 2 files changed, 27 insertions(+), 11 deletions(-)

Comments

Tom Tromey June 28, 2019, 2:12 p.m. UTC | #1
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> Currently, no release of GNU Source Highlight supports Rust.  However,
Tom> I've checked in a patch to do so there, and I plan to make a new
Tom> release sometime this summer.

Tom> This patch prepares gdb for that by adding support for Rust to the
Tom> source highlighting code.

Tom> Because Source Highlight will throw an exception if the language is
Tom> unrecognized, this also changes gdb to ignore exceptions here.  This
Tom> will cause gdb to fall back to un-highlighted source text.

Tom> Tested with the current and development versions of Source Highlight.

This patch works ok on Fedora 28, but on Fedora 29 it causes gdb to
crash.

I don't think there's anything wrong with the patch, so I guess this is
a Fedora bug somehow.  I will see if I can write a test case and file
it.

Meanwhile I think this patch should not go in :-(

Tom
Tom Tromey July 1, 2019, 3:40 p.m. UTC | #2
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> I don't think there's anything wrong with the patch, so I guess this is
Tom> a Fedora bug somehow.  I will see if I can write a test case and file
Tom> it.

I debugged this a bit today, and the problem comes from the use of
"-static-libstdc++ -static-libgcc" when linking.  Using just one of
these makes it work fine.

Are these necessary for some reason?

Tom
Simon Marchi July 2, 2019, 12:29 a.m. UTC | #3
On 2019-07-01 11:40 a.m., Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
> Tom> I don't think there's anything wrong with the patch, so I guess this is
> Tom> a Fedora bug somehow.  I will see if I can write a test case and file
> Tom> it.
> 
> I debugged this a bit today, and the problem comes from the use of
> "-static-libstdc++ -static-libgcc" when linking.  Using just one of
> these makes it work fine.
> 
> Are these necessary for some reason?

I never really knew the reason of using these in the first place.

Simon
Pedro Alves July 2, 2019, 9:18 a.m. UTC | #4
On 7/2/19 1:29 AM, Simon Marchi wrote:
> On 2019-07-01 11:40 a.m., Tom Tromey wrote:
>>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
>>
>> Tom> I don't think there's anything wrong with the patch, so I guess this is
>> Tom> a Fedora bug somehow.  I will see if I can write a test case and file
>> Tom> it.
>>
>> I debugged this a bit today, and the problem comes from the use of
>> "-static-libstdc++ -static-libgcc" when linking.  Using just one of
>> these makes it work fine.
>>
>> Are these necessary for some reason?
> 
> I never really knew the reason of using these in the first place.
> 

IIRC, they're there for gcc, so that a newly built gcc does not depend
on system libstdc++/libgcc or something along those lines.
I.e., it's mainly a consequence of a shared top level.  I think that
the ideal would be if linking with static-* was controllable some
configure switch, and, if it was only enabled by default if building gcc.
Combined gdb + gcc builds must be considered as well, since some people
still do that.

Thanks,
Pedro Alves
Tom Tromey July 13, 2019, 4:06 p.m. UTC | #5
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> IIRC, they're there for gcc, so that a newly built gcc does not depend
Pedro> on system libstdc++/libgcc or something along those lines.
Pedro> I.e., it's mainly a consequence of a shared top level.  I think that
Pedro> the ideal would be if linking with static-* was controllable some
Pedro> configure switch, and, if it was only enabled by default if building gcc.
Pedro> Combined gdb + gcc builds must be considered as well, since some people
Pedro> still do that.

It seems like a bit of a pain to fix at top-level.  These flags are put
into the default LDFLAGS for stage1, or when not bootstrapping.

How about just filtering them out in the gdb build?  We could add a flag
to gdb's configure script to allow them to be added back in, on the off
chance that someone actually wants them.

Tom
Pedro Alves July 16, 2019, 7:19 p.m. UTC | #6
On 7/13/19 5:06 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> IIRC, they're there for gcc, so that a newly built gcc does not depend
> Pedro> on system libstdc++/libgcc or something along those lines.
> Pedro> I.e., it's mainly a consequence of a shared top level.  I think that
> Pedro> the ideal would be if linking with static-* was controllable some
> Pedro> configure switch, and, if it was only enabled by default if building gcc.
> Pedro> Combined gdb + gcc builds must be considered as well, since some people
> Pedro> still do that.
> 
> It seems like a bit of a pain to fix at top-level.  These flags are put
> into the default LDFLAGS for stage1, or when not bootstrapping.
> 

Can you expand on why is it a pain?  I was imagining that the top-level
script would take in consideration whether a gcc/ subdir exists, in
addition to checking some --{enable,disable}-static-runtime or some such,
where it adds the flags to LDFLAGS.

> How about just filtering them out in the gdb build?  We could add a flag
> to gdb's configure script to allow them to be added back in, on the off
> chance that someone actually wants them.
I assume it is put in LDFLAGS for the whole tree in order to
use -static-libcc consistently for both gcc and the libraries it
depends on (like libiberty).  (It'd be interesting to find the
rationale in the original mailing list post/patch that added it to
be sure.)

With what you're suggesting it sounds like we'd build libiberty/, libbfd/,
etc. with -static-libgcc and gdb/ without?  That sounds like something
we shouldn't be doing either.

Thanks,
Pedro Alves
Tom Tromey July 17, 2019, 5:50 p.m. UTC | #7
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> It seems like a bit of a pain to fix at top-level.  These flags are put
>> into the default LDFLAGS for stage1, or when not bootstrapping.

Pedro> Can you expand on why is it a pain?  I was imagining that the top-level
Pedro> script would take in consideration whether a gcc/ subdir exists, in
Pedro> addition to checking some --{enable,disable}-static-runtime or some such,
Pedro> where it adds the flags to LDFLAGS.

The main problem is that the flags are passed down from the top-level
Makefile, so it would need extra top-level Makefile.* hacking.

Pedro> I assume it is put in LDFLAGS for the whole tree in order to
Pedro> use -static-libcc consistently for both gcc and the libraries it
Pedro> depends on (like libiberty).  (It'd be interesting to find the
Pedro> rationale in the original mailing list post/patch that added it to
Pedro> be sure.)

Pedro> With what you're suggesting it sounds like we'd build libiberty/, libbfd/,
Pedro> etc. with -static-libgcc and gdb/ without?  That sounds like something
Pedro> we shouldn't be doing either.

Are those even useful for libiberty or bfd?  I thought those only
affected the link.

Or do people build a shared libiberty and/or bfd?  That seems bad.

I tend to think -static-* is not ever correct for gdb, or at least is
incompatible with Source Highlight.

Tom
diff mbox

Patch

diff --git a/gdb/source-cache.c b/gdb/source-cache.c
index 86efe83bf9a..72211aa1c34 100644
--- a/gdb/source-cache.c
+++ b/gdb/source-cache.c
@@ -153,8 +153,7 @@  get_language_name (enum language lang)
       break;
 
     case language_rust:
-      /* Not handled by Source Highlight.  */
-      break;
+      return "rust.lang";
 
     case language_ada:
       return "ada.lang";
@@ -223,18 +222,30 @@  source_cache::get_source_lines (struct symtab *s, int first_line,
 		  highlighter->setStyleFile ("esc.style");
 		}
 
-	      std::ostringstream output;
-	      highlighter->highlight (input, output, lang_name, fullname);
+	      try
+		{
+		  std::ostringstream output;
+		  highlighter->highlight (input, output, lang_name, fullname);
 
-	      source_text result = { fullname, output.str () };
-	      m_source_map.push_back (std::move (result));
+		  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 ());
+		  if (m_source_map.size () > MAX_ENTRIES)
+		    m_source_map.erase (m_source_map.begin ());
 
-	      *lines = extract_lines (m_source_map.back (), first_line,
-				      last_line);
-	      return true;
+		  *lines = extract_lines (m_source_map.back (), first_line,
+					  last_line);
+		  return true;
+		}
+	      catch (...)
+		{
+		  /* Source Highlight will throw an exception if
+		     highlighting fails.  One possible reason it can
+		     fail is if the language is unknown -- which
+		     matters to gdb because Rust support wasn't added
+		     until after 3.1.8.  Ignore exceptions here an
+		     fall back to un-highlighted text. */
+		}
 	    }
 	}
     }