[PATCH] Add Rust support to source highlighting

[PATCH] Add Rust support to source highlighting

[Tom Tromey]

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(-)

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. */
+		}
 	    }
 	}
     }
-- 
2.17.2

Re: [PATCH] Add Rust support to source highlighting

[Tom Tromey]

>>>>> "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

Re: [PATCH] Add Rust support to source highlighting

[Tom Tromey]

>>>>> "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

Re: [PATCH] Add Rust support to source highlighting

[Simon Marchi]

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

Re: [PATCH] Add Rust support to source highlighting

[Pedro Alves]

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

Re: [PATCH] Add Rust support to source highlighting

[Tom Tromey]

>>>>> "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

Re: [PATCH] Add Rust support to source highlighting

[Pedro Alves]

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

Re: [PATCH] Add Rust support to source highlighting

[Tom Tromey]

>>>>> "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

Re: [PATCH] Add Rust support to source highlighting

[Pedro Alves]

On 7/17/19 6:50 PM, Tom Tromey wrote:
>>>>>> "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.

I'm not sure we're talking about the same thing.  See below for
what I had in mind.

> 
> 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.

I'm not sure they only affect the link, but I do think so.

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

We have things like this in the top level configure:

 # Sometimes we have special requirements for the host libiberty.
 extra_host_libiberty_configure_flags=
 extra_host_zlib_configure_flags=
 case " $configdirs " in
   *" lto-plugin "* | *" libcc1 "*)
     # When these are to be built as shared libraries, the same applies to
     # libiberty.
     extra_host_libiberty_configure_flags=--enable-shared
     ;;
   *" bfd "*)
     # When bfd is to be built as a shared library, the same applies to
     # zlib.
     if test "$enable_shared" = "yes"; then
       extra_host_zlib_configure_flags=--enable-host-shared
     fi
     ;;
 esac
 AC_SUBST(extra_host_libiberty_configure_flags)
 AC_SUBST(extra_host_zlib_configure_flags)

And:

 $ ./src/configure --help | grep host-shared
   --enable-host-shared    build host code as shared libraries

So it does looks like it people do that.

From a2d01e2138a28be2a32978f38298e6b7dd99e7d4 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 17 Jul 2019 16:07:20 +0100
Subject: [PATCH] gcc

---
 configure    | 2 +-
 configure.ac | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 4d111486926..f1cc1cf9e56 100755
--- a/configure
+++ b/configure
@@ -5838,7 +5838,7 @@ else
  # In stage 1, default to linking libstdc++ and libgcc statically with GCC
  # if supported.  But if the user explicitly specified the libraries to use,
  # trust that they are doing what they want.
- if test "$stage1_libs" = "" -a "$have_static_libs" = yes; then
+ if test -d ${srcdir}/gcc -a test "$stage1_libs" = "" -a "$have_static_libs" = yes; then
    stage1_ldflags="-static-libstdc++ -static-libgcc"
  fi
 fi
diff --git a/configure.ac b/configure.ac
index 854f71a34e5..2c85b8d7374 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1614,7 +1614,7 @@ AC_ARG_WITH(stage1-ldflags,
  # In stage 1, default to linking libstdc++ and libgcc statically with GCC
  # if supported.  But if the user explicitly specified the libraries to use,
  # trust that they are doing what they want.
- if test "$stage1_libs" = "" -a "$have_static_libs" = yes; then
+ if test -d ${srcdir}/gcc -a test "$stage1_libs" = "" -a "$have_static_libs" = yes; then
    stage1_ldflags="-static-libstdc++ -static-libgcc"
  fi])
 AC_SUBST(stage1_ldflags)
-- 
2.14.5

Re: [PATCH] Add Rust support to source highlighting

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I'm not sure we're talking about the same thing.  See below for
Pedro> what I had in mind.

Yeah, ok.  I can do this.

I think this means changing gdb's configure to disable source highlight
when we see -static-lib*, because those are simply incompatible.

Tom