Patchwork Fix gdb.ada/bp_c_mixed_case.exp (PR gdb/22670) (Re: [PATCH 3/3] Add new gdb.ada/bp_c_mixed_case testcase for PR gdb/22670)

login
register
mail settings
Submitter Pedro Alves
Date Jan. 9, 2018, 4:45 p.m.
Message ID <619dda09-5c48-447e-ead9-c9d6a5ac9743@redhat.com>
Download mbox | patch
Permalink /patch/25296/
State New
Headers show

Comments

Pedro Alves - Jan. 9, 2018, 4:45 p.m.
On 01/09/2018 02:59 PM, Pedro Alves wrote:
> On 01/09/2018 09:46 AM, Joel Brobecker wrote:

>> What surprises me is that, before your patch, we were finding
>> no symbol at all. So we were failing the lookup both with minimal
>> symbols, and within the partial/full symtab.
> 
> Indeed, good point.  I don't know what I did not think of that.

...

>> Does this make some kind of sense to you? 
> 
> Yes it does.  I played with this a bit, and am testing a patch.
> Stay tuned.

How about this?

The main idea behind making the name matcher be determined by
the symbol's language is so that C++ (etc.) wildmatching in
linespecs works even if the current language is not C++, as e.g.,
when you step through C or assembly code.

Ada's verbatim matching syntax however ("<...>") isn't quite
the same.  It is more a property of the current language than
of a particular symbol's language.  We want to support
this syntax when debugging an Ada program, but it's reason of
existence is to find non-Ada symbols.  This suggests going back
to enabling it depending on current language instead of language
of the symbol being matched.

I'm not entirely happy with the "current_language" reference
(though I think that it's harmless).  I think we could try storing the
current language in the lookup_name_info object, and then convert
a bunch of functions more to pass around lookup_name_info objects
instead of "const char *" names.  I.e., build the lookup_name_info
higher up.  I'm not sure about that, I'll have to think more
about it.  Maybe something different will be better.  It doesn't
help that I'm not used to debugging Ada code, but the recent
testcase additions surely have helped understand better the
intended use cases.  Thanks much for those.

Meanwhile, this looks small- and safe-enough for 8.1, to me.
WDYT?

I'd extended the testcase to also exercise a no-debug-info
function, for extra coverage of the minsyms-only paths.

Thanks,
Pedro Alves
Pedro Alves - Jan. 9, 2018, 5:22 p.m.
On 01/09/2018 04:45 PM, Pedro Alves wrote:
> On 01/09/2018 02:59 PM, Pedro Alves wrote:
>> On 01/09/2018 09:46 AM, Joel Brobecker wrote:
> 
>>> What surprises me is that, before your patch, we were finding
>>> no symbol at all. So we were failing the lookup both with minimal
>>> symbols, and within the partial/full symtab.
>>
>> Indeed, good point.  I don't know what I did not think of that.
> 
> ...
> 
>>> Does this make some kind of sense to you? 
>>
>> Yes it does.  I played with this a bit, and am testing a patch.
>> Stay tuned.
> 
> How about this?

I pushed this to the users/palves/literal-matching branch too,
btw, for easier testing, along with a follow up patch to rename
language_get_symbol_name_matcher -> get_symbol_name_matcher,
since the function is no longer a straight "language method".
WDYT?

Thanks,
Pedro Alves
Joel Brobecker - Jan. 10, 2018, 3:36 a.m.
Hi Pedro,

> The main idea behind making the name matcher be determined by
> the symbol's language is so that C++ (etc.) wildmatching in
> linespecs works even if the current language is not C++, as e.g.,
> when you step through C or assembly code.
> 
> Ada's verbatim matching syntax however ("<...>") isn't quite
> the same.  It is more a property of the current language than
> of a particular symbol's language.  We want to support
> this syntax when debugging an Ada program, but it's reason of
> existence is to find non-Ada symbols.  This suggests going back
> to enabling it depending on current language instead of language
> of the symbol being matched.

That's a good way of describing the situation.

> I'm not entirely happy with the "current_language" reference
> (though I think that it's harmless).  I think we could try storing the
> current language in the lookup_name_info object, and then convert
> a bunch of functions more to pass around lookup_name_info objects
> instead of "const char *" names.  I.e., build the lookup_name_info
> higher up.  I'm not sure about that, I'll have to think more
> about it.  Maybe something different will be better.

I understand. I have the same feeling in general.

> It doesn't help that I'm not used to debugging Ada code, but the
> recent testcase additions surely have helped understand better the
> intended use cases.  Thanks much for those.

My pleasure. I also have the same feeling, but with debugging
C++ right now, so I can certainly relate!

> Meanwhile, this looks small- and safe-enough for 8.1, to me.
> WDYT?
> 
> I'd extended the testcase to also exercise a no-debug-info
> function, for extra coverage of the minsyms-only paths.

Thanks! The patch looks good to me. A few minor typos below...

> >From d1f377264f278d2839c3cf331e931c7382114e11 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Tue, 9 Jan 2018 14:48:55 +0000
> Subject: [PATCH] Ada: make verbatim matcher override other language matchers
>  (PR gdb/22670)
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	PR gdb/22670
> 	* dwarf2read.c
> 	(gdb_index_symbol_name_matcher::gdb_index_symbol_name_matcher):
> 	Adjust to use language_get_symbol_name_matcher instead of
> 	language_defn::la_get_symbol_name_matcher.
> 	* language.c (language_get_symbol_name_matcher): In in Ada mode
                                                         ^^^^^
                                                         |||||


> 	and the lookup name is a verbatim match, return Ada's matcher.
> 	* language.h (language_get_symbol_name_matcher): Adjust comment.
> 	(ada_lookup_name_info::verbatim_p):: New method.
> 
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	PR gdb/22670
> 	* gdb.ada/bp_c_mixed_case.exp: Add intro comment.  Test printing C
> 	functions too.  Test setting breakpoints and printing C functions
> 	with no debug info too.
> 	* gdb.ada/bp_c_mixed_case/qux.c: New file.
> +# Try printing again using the "<...>" notation.  This shouldn work
                                                          ^^^^^^^
                                                          |||||||

I tested this patch on x86_64-linux, using both the official testsuite
as well as AdaCore's testsuite. No regression :).

Thanks, Pedro!
Pedro Alves - Jan. 10, 2018, 11:41 p.m.
On 01/10/2018 03:36 AM, Joel Brobecker wrote:
> Thanks! The patch looks good to me. A few minor typos below...

...

> 
> I tested this patch on x86_64-linux, using both the official testsuite
> as well as AdaCore's testsuite. No regression :).

Hurray! :-)  

I fixed the typos and pushed all the pending patches in.  I've pushed
the rename patch as well, both to master and branch, the latter just to
make it easier to backport further related patches, if we need to:

 https://sourceware.org/ml/gdb-patches/2018-01/msg00207.html

Thanks,
Pedro Alves
Joel Brobecker - Jan. 11, 2018, 4 a.m.
> I fixed the typos and pushed all the pending patches in.  I've pushed
> the rename patch as well, both to master and branch, the latter just to
> make it easier to backport further related patches, if we need to:
> 
>  https://sourceware.org/ml/gdb-patches/2018-01/msg00207.html

Hooray! Thanks a lot, Pedro.

Patch

From d1f377264f278d2839c3cf331e931c7382114e11 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 9 Jan 2018 14:48:55 +0000
Subject: [PATCH] Ada: make verbatim matcher override other language matchers
 (PR gdb/22670)

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	PR gdb/22670
	* dwarf2read.c
	(gdb_index_symbol_name_matcher::gdb_index_symbol_name_matcher):
	Adjust to use language_get_symbol_name_matcher instead of
	language_defn::la_get_symbol_name_matcher.
	* language.c (language_get_symbol_name_matcher): In in Ada mode
	and the lookup name is a verbatim match, return Ada's matcher.
	* language.h (language_get_symbol_name_matcher): Adjust comment.
	(ada_lookup_name_info::verbatim_p):: New method.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	PR gdb/22670
	* gdb.ada/bp_c_mixed_case.exp: Add intro comment.  Test printing C
	functions too.  Test setting breakpoints and printing C functions
	with no debug info too.
	* gdb.ada/bp_c_mixed_case/qux.c: New file.
---
 gdb/dwarf2read.c                                   | 32 ++++++------
 gdb/language.c                                     |  7 +++
 gdb/language.h                                     |  5 +-
 gdb/symtab.h                                       |  6 ++-
 gdb/testsuite/gdb.ada/bp_c_mixed_case.exp          | 58 +++++++++++++++++++++-
 .../gdb.ada/bp_c_mixed_case/foo_h731_021.adb       |  3 ++
 gdb/testsuite/gdb.ada/bp_c_mixed_case/qux.c        | 21 ++++++++
 7 files changed, 111 insertions(+), 21 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/bp_c_mixed_case/qux.c

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index a3028e5c52f..8bdac576bde 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -4529,23 +4529,21 @@  gdb_index_symbol_name_matcher::gdb_index_symbol_name_matcher
   for (int i = 0; i < nr_languages; i++)
     {
       const language_defn *lang = language_def ((enum language) i);
-      if (lang->la_get_symbol_name_matcher != NULL)
-	{
-	  symbol_name_matcher_ftype *name_matcher
-	    = lang->la_get_symbol_name_matcher (m_lookup_name);
-
-	  /* Don't insert the same comparison routine more than once.
-	     Note that we do this linear walk instead of a cheaper
-	     sorted insert, or use a std::set or something like that,
-	     because relative order of function addresses is not
-	     stable.  This is not a problem in practice because the
-	     number of supported languages is low, and the cost here
-	     is tiny compared to the number of searches we'll do
-	     afterwards using this object.  */
-	  if (std::find (matchers.begin (), matchers.end (), name_matcher)
-	      == matchers.end ())
-	    matchers.push_back (name_matcher);
-	}
+      symbol_name_matcher_ftype *name_matcher
+	= language_get_symbol_name_matcher (lang, m_lookup_name);
+
+      /* Don't insert the same comparison routine more than once.
+	 Note that we do this linear walk instead of a seemingly
+	 cheaper sorted insert, or use a std::set or something like
+	 that, because relative order of function addresses is not
+	 stable.  This is not a problem in practice because the number
+	 of supported languages is low, and the cost here is tiny
+	 compared to the number of searches we'll do afterwards using
+	 this object.  */
+      if (name_matcher != default_symbol_name_matcher
+	  && (std::find (matchers.begin (), matchers.end (), name_matcher)
+	      == matchers.end ()))
+	matchers.push_back (name_matcher);
     }
 }
 
diff --git a/gdb/language.c b/gdb/language.c
index cacaf3f7d1d..c80237e6f9d 100644
--- a/gdb/language.c
+++ b/gdb/language.c
@@ -730,6 +730,13 @@  symbol_name_matcher_ftype *
 language_get_symbol_name_matcher (const language_defn *lang,
 				  const lookup_name_info &lookup_name)
 {
+  /* If currently in Ada mode, and the lookup name is wrapped in
+     '<...>', hijack all symbol name comparisons using the Ada
+     matcher, which handles the verbatim matching.  */
+  if (current_language->la_language == language_ada
+      && lookup_name.ada ().verbatim_p ())
+    return current_language->la_get_symbol_name_matcher (lookup_name);
+
   if (lang->la_get_symbol_name_matcher != nullptr)
     return lang->la_get_symbol_name_matcher (lookup_name);
   return default_symbol_name_matcher;
diff --git a/gdb/language.h b/gdb/language.h
index 49828f3aee7..50610959715 100644
--- a/gdb/language.h
+++ b/gdb/language.h
@@ -633,7 +633,10 @@  extern bool default_symbol_name_matcher
    completion_match_result *comp_match_res);
 
 /* Get LANG's symbol_name_matcher method for LOOKUP_NAME.  Returns
-   default_symbol_name_matcher if not set.  */
+   default_symbol_name_matcher if not set.  LANG is used as a hint;
+   the function may ignore it depending on the current language and
+   LOOKUP_NAME.  Specifically, if the current language is Ada, this
+   may return an Ada matcher regardless of LANG.  */
 symbol_name_matcher_ftype *language_get_symbol_name_matcher
   (const language_defn *lang, const lookup_name_info &lookup_name);
 
diff --git a/gdb/symtab.h b/gdb/symtab.h
index a7b1ed2131c..f9d52e76979 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -110,7 +110,11 @@  class ada_lookup_name_info final
   bool standard_p () const
   { return m_standard_p; }
 
- private:
+  /* Return true if doing a verbatim match.  */
+  bool verbatim_p () const
+  { return m_verbatim_p; }
+
+private:
   /* The Ada-encoded lookup name.  */
   std::string m_encoded_name;
 
diff --git a/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp b/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp
index 7787646c67f..048a13ba976 100644
--- a/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp
+++ b/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp
@@ -13,6 +13,11 @@ 
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+# Test setting breakpoints in C functions with some uppercase letters
+# in their name, using the "<...>" notation.  See gdb/22670.  While at
+# it, also try evaluating expressions involving calls to such
+# functions.
+
 load_lib "ada.exp"
 
 standard_ada_testfile foo_h731_021
@@ -21,8 +26,19 @@  set cfile "bar"
 set csrcfile ${srcdir}/${subdir}/${testdir}/${cfile}.c
 set cobject [standard_output_file ${cfile}.o]
 
+set cfile2 "qux"
+set csrcfile2 ${srcdir}/${subdir}/${testdir}/${cfile2}.c
+set cobject2 [standard_output_file ${cfile2}.o]
+
 gdb_compile "${csrcfile}" "${cobject}" object [list debug]
-if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug additional_flags=-largs additional_flags=${cobject} additional_flags=-margs]] != "" } {
+gdb_compile "${csrcfile2}" "${cobject2}" object ""
+
+set options [list debug \
+		 additional_flags=-largs \
+		 additional_flags=${cobject} \
+		 additional_flags=${cobject2} \
+		 additional_flags=-margs]
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable $options] != "" } {
   return -1
 }
 
@@ -37,14 +53,52 @@  if ![runto "foo_h731_021"] then {
 gdb_test "show lang" \
          "\"auto; currently ada\"\\."
 
+# Before running to the C function (and thus switching out of Ada
+# mode), try printing the function using the "<...>" notation.
+gdb_test "p <MixedCaseFunc>" \
+         " = void" \
+         "p <MixedCaseFunc>, in Ada"
+
+gdb_test "p <NoDebugMixedCaseFunc>" \
+         " = {<text variable, no debug info>} $hex <NoDebugMixedCaseFunc>" \
+         "p <NoDebugMixedCaseFunc>, in Ada"
+
 # Try inserting a breakpoint inside a C function. Because the function's
 # name has some uppercase letters, we need to use the "<...>" notation.
 # The purpose of this testcase is to verify that we can in fact do so
-# and that it inserts the breakpoint at the expected location.  See gdb/22670.
+# and that it inserts the breakpoint at the expected location.
 gdb_test "break <MixedCaseFunc>" \
          "Breakpoint $decimal at $hex: file .*bar.c, line $decimal\\."
 
+# Same, but this time on the function with no debug info.
+gdb_test "break <NoDebugMixedCaseFunc>" \
+         "Breakpoint $decimal at $hex"
+
 # Resume the program's execution, verifying that it lands at the expected
 # location.
 gdb_test "continue" \
          "Breakpoint $decimal, MixedCaseFunc \\(\\) at .*bar\\.c:$decimal.*"
+
+# Try printing again using the "<...>" notation.  This shouldn work
+# now, since the current frame is a C function.
+gdb_test "p <MixedCaseFunc>" \
+         "A syntax error in expression, near `<MixedCaseFunc>'\\." \
+         "p <MixedCaseFunc>, in C"
+
+gdb_test "p <NoDebugMixedCaseFunc>" \
+         "A syntax error in expression, near `<NoDebugMixedCaseFunc>'\\." \
+         "p <NoDebugMixedCaseFunc>, in C"
+
+set test "break <MixedCaseFunc>, in C"
+gdb_test_multiple "break <MixedCaseFunc>" $test {
+	-re "Function \"<MixedCaseFunc>\" not defined\..*Make breakpoint pending on future shared library load.*y or .n.. $" {
+		gdb_test_no_output "n" $test
+	}
+}
+
+set test "break <NoDebugMixedCaseFunc>, in C"
+gdb_test_multiple "break <NoDebugMixedCaseFunc>" $test {
+	-re "Function \"<NoDebugMixedCaseFunc>\" not defined\..*Make breakpoint pending on future shared library load.*y or .n.. $" {
+		gdb_test_no_output "n" $test
+	}
+}
diff --git a/gdb/testsuite/gdb.ada/bp_c_mixed_case/foo_h731_021.adb b/gdb/testsuite/gdb.ada/bp_c_mixed_case/foo_h731_021.adb
index 88e0c319be4..7cd17c24cee 100644
--- a/gdb/testsuite/gdb.ada/bp_c_mixed_case/foo_h731_021.adb
+++ b/gdb/testsuite/gdb.ada/bp_c_mixed_case/foo_h731_021.adb
@@ -15,7 +15,10 @@ 
 
 procedure Foo_H731_021 is
    Procedure C_Func;
+   Procedure C_FuncNoDebug;
    pragma Import (C, C_Func, "MixedCaseFunc");
+   pragma Import (C, C_FuncNoDebug, "NoDebugMixedCaseFunc");
 begin
    C_Func;
+   C_FuncNoDebug;
 end Foo_H731_021;
diff --git a/gdb/testsuite/gdb.ada/bp_c_mixed_case/qux.c b/gdb/testsuite/gdb.ada/bp_c_mixed_case/qux.c
new file mode 100644
index 00000000000..eea43041bf0
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_c_mixed_case/qux.c
@@ -0,0 +1,21 @@ 
+/* Copyright 2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+void
+NoDebugMixedCaseFunc (void)
+{
+}
-- 
2.14.3