From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 127768 invoked by alias); 18 Sep 2019 22:16:01 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 127755 invoked by uid 89); 18 Sep 2019 22:16:01 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.1 spammy=sk:without X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 18 Sep 2019 22:15:51 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 4F923B621; Wed, 18 Sep 2019 22:15:49 +0000 (UTC) From: Tom de Vries Subject: Re: [PATCH v2 2/2] Add Rust support to source highlighting To: Tom Tromey Cc: gdb-patches@sourceware.org, Pedro Alves References: <20190727155155.32417-1-tom@tromey.com> <20190727155155.32417-3-tom@tromey.com> <332da42c-ae75-a148-221f-4f7bb815f40f@suse.de> <87v9u05en8.fsf@tromey.com> <87pnjywzmz.fsf@tromey.com> Openpgp: preference=signencrypt Message-ID: <44701c2c-21a7-0211-cc45-b6214ecfb165@suse.de> Date: Wed, 18 Sep 2019 22:16:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <87pnjywzmz.fsf@tromey.com> Content-Type: multipart/mixed; boundary="------------CCA222DC6FF9F9A93B2EE558" X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00354.txt.bz2 This is a multi-part message in MIME format. --------------CCA222DC6FF9F9A93B2EE558 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-length: 1825 On 17-09-19 20:23, Tom Tromey wrote: >>>>>> "Tom" == Tom de Vries writes: > > Tom> Tested on openSUSE Leap 15.1, both with and without source-highlight > Tom> package installed. > > Tom> OK for 8.3 branch? > > This looks fine to me, but I had two comments. > > First, Joel pointed out earlier that backports require a tracking PR, so > be sure to file one and mention it in the commit. > Ack. Filed https://sourceware.org/bugzilla/show_bug.cgi?id=25009 "terminate called after throwing an instance of 'srchilite::ParserException'". > Second, up-thread you said: > > Tom> This expection happens when the library attempts to access > Tom> /usr/share/source-highlight/esc.outlang, which is not there, because > Tom> it's contained in another package (source-highlight). > > ... but the patch does: > > Tom> srchilite::SourceHighlight highlighter ("esc.outlang"); > Tom> highlighter.setStyleFile("esc.style"); > > Tom> - std::ostringstream output; > Tom> - highlighter.highlight (input, output, lang_name, fullname); > Tom> + try > Tom> + { > Tom> + std::ostringstream output; > Tom> + highlighter.highlight (input, output, lang_name, fullname); > > I am wondering if the "try" should encompass the construction of > "highlighter". It's possible that it works fine as-is, since maybe the > argument isn't used until highlighting is attempted -- but I figured I > would ask just to be sure. > Well, it does work, but your question makes me realize that that's an implementation artefact of the library, which could change for newer version (or perhaps there are older versions where that differs). So it should be safer to wrap highlighter construction as well. Updated the patch accordingly, and also added a PR number to both patches. Thanks, - Tom --------------CCA222DC6FF9F9A93B2EE558 Content-Type: text/x-patch; name="0001-Add-with-static-standard-libraries-to-the-top-level.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename*0="0001-Add-with-static-standard-libraries-to-the-top-level.pat"; filename*1="ch" Content-length: 4766 Add --with-static-standard-libraries to the top level [ Backport of master commit c1a5d03a89. ] gdb should normally not be linked with -static-libstdc++. Currently this has not caused problems, but it's incompatible with catching an exception thrown from a shared library -- and a subsequent patch changes gdb to do just this. This patch adds a new --with-static-standard-libraries flag to the top-level configure. It defaults to "auto", which means enabled if gcc is being built, and disabled otherwise. ChangeLog 2019-08-19 Tom Tromey PR gdb/25009 * configure: Rebuild. * configure.ac: Add --with-static-standard-libraries. --- ChangeLog | 6 ++++++ configure | 24 +++++++++++++++++++++++- configure.ac | 16 +++++++++++++++- 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index cd631a15b6..2441f4b014 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2019-08-19 Tom Tromey + + PR gdb/25009 + * configure: Rebuild. + * configure.ac: Add --with-static-standard-libraries. + 2018-06-24 Nick Clifton 2.32 branch created. diff --git a/configure b/configure index 3747645961..0a500810e3 100755 --- a/configure +++ b/configure @@ -802,6 +802,7 @@ with_gmp with_gmp_include with_gmp_lib with_stage1_libs +with_static_standard_libraries with_stage1_ldflags with_boot_libs with_boot_ldflags @@ -1572,6 +1573,9 @@ Optional Packages: --with-gmp-include=PATH specify directory for installed GMP include files --with-gmp-lib=PATH specify directory for the installed GMP library --with-stage1-libs=LIBS libraries for stage1 + --with-static-standard-libraries + use -static-libstdc++ and -static-libgcc + (default=auto) --with-stage1-ldflags=FLAGS linker flags for stage1 --with-boot-libs=LIBS libraries for stage2 and later @@ -5824,6 +5828,23 @@ fi +# Whether or not to use -static-libstdc++ and -static-libgcc. The +# default is yes if gcc is being built; no otherwise. The reason for +# this default is that gdb is sometimes linked against GNU Source +# Highlight, which is a shared library that uses C++ exceptions. In +# this case, -static-libstdc++ will cause crashes. + +# Check whether --with-static-standard-libraries was given. +if test "${with_static_standard_libraries+set}" = set; then : + withval=$with_static_standard_libraries; +else + with_static_standard_libraries=auto +fi + +if test "$with_static_standard_libraries" = auto; then + with_static_standard_libraries=$have_compiler +fi + # Linker flags to use for stage1 or when not bootstrapping. # Check whether --with-stage1-ldflags was given. @@ -5838,7 +5859,8 @@ 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 "$with_static_standard_libraries" = yes -a "$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 46501c2882..6cbee2ae2c 100644 --- a/configure.ac +++ b/configure.ac @@ -1602,6 +1602,19 @@ AC_ARG_WITH(stage1-libs, [stage1_libs=]) AC_SUBST(stage1_libs) +# Whether or not to use -static-libstdc++ and -static-libgcc. The +# default is yes if gcc is being built; no otherwise. The reason for +# this default is that gdb is sometimes linked against GNU Source +# Highlight, which is a shared library that uses C++ exceptions. In +# this case, -static-libstdc++ will cause crashes. +AC_ARG_WITH(static-standard-libraries, +[AS_HELP_STRING([--with-static-standard-libraries], + [use -static-libstdc++ and -static-libgcc (default=auto)])], +[], [with_static_standard_libraries=auto]) +if test "$with_static_standard_libraries" = auto; then + with_static_standard_libraries=$have_compiler +fi + # Linker flags to use for stage1 or when not bootstrapping. AC_ARG_WITH(stage1-ldflags, [AS_HELP_STRING([--with-stage1-ldflags=FLAGS], [linker flags for stage1])], @@ -1614,7 +1627,8 @@ 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 "$with_static_standard_libraries" = yes -a "$stage1_libs" = "" \ + -a "$have_static_libs" = yes; then stage1_ldflags="-static-libstdc++ -static-libgcc" fi]) AC_SUBST(stage1_ldflags) --------------CCA222DC6FF9F9A93B2EE558 Content-Type: text/x-patch; name="0002-Add-Rust-support-to-source-highlighting.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="0002-Add-Rust-support-to-source-highlighting.patch" Content-length: 5302 Add Rust support to source highlighting [ Backport of master commit d806ea2d0e. ] 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. This updates gdb's configure script to reject the combination of Source Highlight and -static-libstdc++. This is done because it's not possible to use -static-libstdc++ and then catch exceptions from a shared library. Tested with the current and development versions of Source Highlight. gdb/ChangeLog 2019-08-19 Tom Tromey PR gdb/25009 * configure: Rebuild. * configure.ac: Disallow the combination of -static-libstdc++ and source highlight. * source-cache.c (get_language_name): Handle rust. (source_cache::get_source_lines): Ignore highlighting exceptions. --- gdb/ChangeLog | 9 +++++++++ gdb/configure | 6 ++++++ gdb/configure.ac | 8 ++++++++ gdb/source-cache.c | 37 ++++++++++++++++++++++++------------- 4 files changed, 47 insertions(+), 13 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 969515a65e..765e2c95e1 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,12 @@ +2019-08-19 Tom Tromey + + PR gdb/25009 + * configure: Rebuild. + * configure.ac: Disallow the combination of -static-libstdc++ and + source highlight. + * source-cache.c (get_language_name): Handle rust. + (source_cache::get_source_lines): Ignore highlighting exceptions. + 2019-06-28 Sergio Durigan Junior PR breakpoints/24541 diff --git a/gdb/configure b/gdb/configure index 854837c50a..866564fbe6 100755 --- a/gdb/configure +++ b/gdb/configure @@ -11503,6 +11503,12 @@ $as_echo "no - pkg-config not found" >&6; } as_fn_error $? "pkg-config was not found in your system" "$LINENO" 5 fi else + case "$LDFLAGS" in + *static-libstdc*) + as_fn_error $? "source highlight is incompatible with -static-libstdc++; either use --disable-source-highlight or --without-static-standard-libraries" "$LINENO" 5 + ;; + esac + if ${pkg_config_prog_path} --exists source-highlight; then SRCHIGH_CFLAGS=`${pkg_config_prog_path} --cflags source-highlight` SRCHIGH_LIBS=`${pkg_config_prog_path} --libs source-highlight` diff --git a/gdb/configure.ac b/gdb/configure.ac index 1527585839..0805827adf 100644 --- a/gdb/configure.ac +++ b/gdb/configure.ac @@ -1251,6 +1251,14 @@ if test "${enable_source_highlight}" != "no"; then AC_MSG_ERROR([pkg-config was not found in your system]) fi else + case "$LDFLAGS" in + *static-libstdc*) + AC_MSG_ERROR([source highlight is incompatible with -static-libstdc++; dnl +either use --disable-source-highlight or dnl +--without-static-standard-libraries]) + ;; + esac + if ${pkg_config_prog_path} --exists source-highlight; then SRCHIGH_CFLAGS=`${pkg_config_prog_path} --cflags source-highlight` SRCHIGH_LIBS=`${pkg_config_prog_path} --libs source-highlight` diff --git a/gdb/source-cache.c b/gdb/source-cache.c index 5eae02082d..d443c5e7fb 100644 --- a/gdb/source-cache.c +++ b/gdb/source-cache.c @@ -156,8 +156,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"; @@ -213,21 +212,33 @@ 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"); + try + { + srchilite::SourceHighlight highlighter ("esc.outlang"); + highlighter.setStyleFile("esc.style"); - std::ostringstream output; - highlighter.highlight (input, output, lang_name, fullname); + 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 and fall back to + un-highlighted text. */ + } } } } --------------CCA222DC6FF9F9A93B2EE558--