FYI/pushed: Additional tests showing regression post C++ wild matching

FYI/pushed: Additional tests showing regression post C++ wild matching

[Joel Brobecker]

Hello,

I have just pushed the following commits to master, that highlight
other situations where the C++ wild matching patch series introduces
a regression for Ada.

  * [PATCH 1/3] Add gdb.ada/info_addr_mixed_case new testcase
  * [PATCH 2/3] Add "complete break ada" test to gdb.ada/complete.exp
  * [PATCH 3/3] Add new gdb.ada/bp_c_mixed_case testcase for PR gdb/22670

Those tests should all be KFAIL-ed for now, so as to avoid generating
new FAILs.

I wanted to have those in sooner rather than later, to faciliate
coordination with Pedro. Reviews are still appreciated, and will
be taken into account without delay.

Thanks!
-- 
Joel

[PATCH 1/3] Add gdb.ada/info_addr_mixed_case new testcase

[Joel Brobecker]

This patch adds a new testcase to demonstrate a regression introduced by:

    commit b5ec771e60c1a0863e51eb491c85c674097e9e13
    Date:   Wed Nov 8 14:22:32 2017 +0000
    Subject: Introduce lookup_name_info and generalize Ada's FULL/WILD name matching

The purpose of the testcase is to verify that a user can use any
casing for an Ada symbol name passed to the "info address" command.
After the patch above was applied, GDB was no longer able to find
the symbol:

    (gdb) info address My_Table
    No symbol "My_Table" in current context.

gdb/testsuite/ChangeLog:

	PR gdb/22670
	* gdb.ada/info_addr_mixed_case: New testcase.

Tested on x86_64-linux, both before and after the patch.
---
 gdb/testsuite/ChangeLog                            |  5 +++
 gdb/testsuite/gdb.ada/info_addr_mixed_case.exp     | 42 ++++++++++++++++++++++
 gdb/testsuite/gdb.ada/info_addr_mixed_case/foo.adb | 21 +++++++++++
 gdb/testsuite/gdb.ada/info_addr_mixed_case/pck.adb | 24 +++++++++++++
 gdb/testsuite/gdb.ada/info_addr_mixed_case/pck.ads | 35 ++++++++++++++++++
 5 files changed, 127 insertions(+)
 create mode 100644 gdb/testsuite/gdb.ada/info_addr_mixed_case.exp
 create mode 100644 gdb/testsuite/gdb.ada/info_addr_mixed_case/foo.adb
 create mode 100644 gdb/testsuite/gdb.ada/info_addr_mixed_case/pck.adb
 create mode 100644 gdb/testsuite/gdb.ada/info_addr_mixed_case/pck.ads

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 500dbdd..b1be7e3 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2018-01-04  Joel Brobecker  <brobecker@adacore.com>
+
+	PR gdb/22670
+	* gdb.ada/info_addr_mixed_case: New testcase.
+
 2018-01-03  Xavier Roirand  <roirand@adacore.com>
 
 	* gdb.ada/excep_handle.exp: New testcase.
diff --git a/gdb/testsuite/gdb.ada/info_addr_mixed_case.exp b/gdb/testsuite/gdb.ada/info_addr_mixed_case.exp
new file mode 100644
index 0000000..e9fce0d
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/info_addr_mixed_case.exp
@@ -0,0 +1,42 @@
+# Copyright 2018 Free Software Foundation, Inc.
+#
+# 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/>.
+
+load_lib "ada.exp"
+
+standard_ada_testfile foo
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "START" ${testdir}/foo.adb]
+if ![runto "foo.adb:$bp_location" ] then {
+  perror "Couldn't run ${testfile}"
+  return
+}
+
+# The following test exercises the situation when uppercase letters
+# are used in the name of the symbol passed to the "info address"
+# command.  This should not make a difference, as the language is
+# Ada, and Ada is case-insensitive.
+
+# commit b5ec771e60c1a0863e51eb491c85c674097e9e13 (Introduce
+# lookup_name_info and generalize Ada's FULL/WILD name matching)
+# caused the following test to fail. KFAIL it while investigating...
+setup_kfail gdb/22670 "*-*-*"
+gdb_test "info address My_Table" \
+         "Symbol \"pck\\.my_table\" is static storage at address $hex\\."
diff --git a/gdb/testsuite/gdb.ada/info_addr_mixed_case/foo.adb b/gdb/testsuite/gdb.ada/info_addr_mixed_case/foo.adb
new file mode 100644
index 0000000..0028fda
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/info_addr_mixed_case/foo.adb
@@ -0,0 +1,21 @@
+--  Copyright 2018 Free Software Foundation, Inc.
+--
+--  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/>.
+
+with Pck; use Pck;
+
+procedure Foo is
+begin
+   Do_Nothing (My_Table'Address); --  START
+end Foo;
diff --git a/gdb/testsuite/gdb.ada/info_addr_mixed_case/pck.adb b/gdb/testsuite/gdb.ada/info_addr_mixed_case/pck.adb
new file mode 100644
index 0000000..2057040
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/info_addr_mixed_case/pck.adb
@@ -0,0 +1,24 @@
+--  Copyright 2018 Free Software Foundation, Inc.
+--
+--  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/>.
+
+package body Pck is
+
+   procedure Do_Nothing (A : System.Address) is
+   begin
+      null;
+   end Do_Nothing;
+
+end Pck;
+
diff --git a/gdb/testsuite/gdb.ada/info_addr_mixed_case/pck.ads b/gdb/testsuite/gdb.ada/info_addr_mixed_case/pck.ads
new file mode 100644
index 0000000..0a786cc
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/info_addr_mixed_case/pck.ads
@@ -0,0 +1,35 @@
+--  Copyright 2018 Free Software Foundation, Inc.
+--
+--  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/>.
+
+with System;
+
+package Pck is
+
+   type Float_Array is array (Integer range <>) of Integer;
+   type Float_Ptr   is access Float_Array;
+
+   type Table_Type is (One, Two, Three, Four, Five);
+   type New_Table_Array is array (Table_Type) of Float_Ptr;
+
+   My_Table : New_Table_Array := (others => new Float_Array'((4 => 16#DE#,
+                                                              5 => 16#AD#)));
+
+   My_F : Float_Ptr := new Float_Array'(4 => 16#BE#,
+                                        5 => 16#EF#);
+
+   procedure Do_Nothing (A : System.Address);
+
+end Pck;
+
-- 
2.1.4

Re: [PATCH 1/3] Add gdb.ada/info_addr_mixed_case new testcase

[Pedro Alves]

On 01/04/2018 08:35 AM, Joel Brobecker wrote:
> This patch adds a new testcase to demonstrate a regression introduced by:
> 
>     commit b5ec771e60c1a0863e51eb491c85c674097e9e13
>     Date:   Wed Nov 8 14:22:32 2017 +0000
>     Subject: Introduce lookup_name_info and generalize Ada's FULL/WILD name matching
> 
> The purpose of the testcase is to verify that a user can use any
> casing for an Ada symbol name passed to the "info address" command.
> After the patch above was applied, GDB was no longer able to find
> the symbol:
> 
>     (gdb) info address My_Table
>     No symbol "My_Table" in current context.

The mixed-case aspect is actually a red herring here.  Using
lowercase doesn't work either:

 (gdb) info address my_table
 No symbol "my_table" in current context.

I think the problem is instead that "info address" is doing a
symbol_name_match_type::FULL match, but the symbol's full name
is pck.my_table, which doesn't match.

If you pass the fully-qualified name, then it work, regardless
of casing:

 (gdb) info address pck.My_Table
 Symbol "pck.my_table" is static storage at address 0x6155e0.

 (gdb) info address pck.my_table
 Symbol "pck.my_table" is static storage at address 0x6155e0.

 (gdb) info address Pck.My_Table
 Symbol "pck.my_table" is static storage at address 0x6155e0.

Ada mode wants symbol names in expressions to be looked up
using wild matching, unlike other languages.  To handle that
I had added symbol_name_match_type::EXPRESSION:

  /* Expression matching.  The same as FULL matching in most
     languages.  The same as WILD matching in Ada.  */
  EXPRESSION,

IIRC, this is mainly used in the completion paths.

I think we'll need to make "info address" use it too, and
possibly other commands that accept an expression as argument.

Thanks,
Pedro Alves

Re: [PATCH 1/3] Add gdb.ada/info_addr_mixed_case new testcase

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 2575 bytes --]

On 01/04/2018 01:25 PM, Pedro Alves wrote:
> On 01/04/2018 08:35 AM, Joel Brobecker wrote:
>> This patch adds a new testcase to demonstrate a regression introduced by:
>>
>>     commit b5ec771e60c1a0863e51eb491c85c674097e9e13
>>     Date:   Wed Nov 8 14:22:32 2017 +0000
>>     Subject: Introduce lookup_name_info and generalize Ada's FULL/WILD name matching
>>
>> The purpose of the testcase is to verify that a user can use any
>> casing for an Ada symbol name passed to the "info address" command.
>> After the patch above was applied, GDB was no longer able to find
>> the symbol:
>>
>>     (gdb) info address My_Table
>>     No symbol "My_Table" in current context.
> 
> The mixed-case aspect is actually a red herring here.  Using
> lowercase doesn't work either:
> 
>  (gdb) info address my_table
>  No symbol "my_table" in current context.
> 
> I think the problem is instead that "info address" is doing a
> symbol_name_match_type::FULL match, but the symbol's full name
> is pck.my_table, which doesn't match.
> 
> If you pass the fully-qualified name, then it work, regardless
> of casing:
> 
>  (gdb) info address pck.My_Table
>  Symbol "pck.my_table" is static storage at address 0x6155e0.
> 
>  (gdb) info address pck.my_table
>  Symbol "pck.my_table" is static storage at address 0x6155e0.
> 
>  (gdb) info address Pck.My_Table
>  Symbol "pck.my_table" is static storage at address 0x6155e0.
> 
> Ada mode wants symbol names in expressions to be looked up
> using wild matching, unlike other languages.  To handle that
> I had added symbol_name_match_type::EXPRESSION:
> 
>   /* Expression matching.  The same as FULL matching in most
>      languages.  The same as WILD matching in Ada.  */
>   EXPRESSION,
> 
> IIRC, this is mainly used in the completion paths.
> 
> I think we'll need to make "info address" use it too, and
> possibly other commands that accept an expression as argument.
Turns out that the Ada symbol lookup machinery is sufficiently decoupled
from "regular" lookup that the problem is elsewhere.
While we may still want to consider migrating that hack towards
symbol_name_match_type::EXPRESSION, things should still work in
principle without doing that, AFAICT.

Whether to do a full or wild match is decided based on the lookup name
string (see name_match_type_from_name).  That was introduced basically
as a refactor of preexisting code, IIRC.

I traced the problem to the verbatim-wrapping hack in
ada_lookup_encoded_symbol.

See the attached patch.  It fixes gdb.ada/info_addr_mixed_case,
and introduces no regressions for me.  WDYT?

[-- Attachment #2: 0001-Fix-gdb.ada-info_addr_mixed_case.exp-PR-gdb-22670.patch --]
[-- Type: text/x-patch, Size: 4996 bytes --]

From 170182ca1b40c9d9d421ffd2542271561e263650 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 4 Jan 2018 17:20:10 +0000
Subject: [PATCH] Fix gdb.ada/info_addr_mixed_case.exp (PR gdb/22670)

The comments about mixed case in the testcase are actually a red
herring.  The problem here is that we'd get to
ada_lookup_encoded_symbol with "my_table", which wraps the looked up
name in "<>"s to force a verbatim match, and that in turn disables
wild matching.

Fix this by swapping around the internals of ada_lookup_encoded_symbol
and ada_lookup_symbol, thus avoiding the encoding and
verbatim-wrapping in the ada_lookup_symbol case, the case that starts
with a user-provided lookup name.

Ada encoding is still done of course, in the ada_lookup_name_info
ctor.  This could be also seen as avoiding the double-encoding problem
in a different way.

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

	PR gdb/22670
	* ada-lang.c (ada_lookup_encoded_symbol): Reimplement in terms of
	ada_lookup_symbol.
	(ada_lookup_symbol): Reimplement in terms of
	ada_lookup_symbol_list, bits factored out from
	ada_lookup_encoded_symbol.

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

	PR gdb/22670
	* gdb.ada/info_addr_mixed_case.exp: Remove kfail.  Extend test to
	exercise lower case too, and to exercise both full matching and
	wild matching.
---
 gdb/ada-lang.c                                 | 43 ++++++++++++--------------
 gdb/testsuite/gdb.ada/info_addr_mixed_case.exp | 15 +++++----
 2 files changed, 26 insertions(+), 32 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 4ecf7b0051c..5f03014bb84 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -5911,10 +5911,6 @@ ada_lookup_encoded_symbol (const char *name, const struct block *block,
 			   domain_enum domain,
 			   struct block_symbol *info)
 {
-  struct block_symbol *candidates;
-  int n_candidates;
-  struct cleanup *old_chain;
-
   /* Since we already have an encoded name, wrap it in '<>' to force a
      verbatim match.  Otherwise, if the name happens to not look like
      an encoded name (because it doesn't include a "__"),
@@ -5924,22 +5920,7 @@ ada_lookup_encoded_symbol (const char *name, const struct block *block,
   std::string verbatim = std::string ("<") + name + '>';
 
   gdb_assert (info != NULL);
-  memset (info, 0, sizeof (struct block_symbol));
-
-  n_candidates = ada_lookup_symbol_list (verbatim.c_str (), block,
-					 domain, &candidates);
-  old_chain = make_cleanup (xfree, candidates);
-
-  if (n_candidates == 0)
-    {
-      do_cleanups (old_chain);
-      return;
-    }
-
-  *info = candidates[0];
-  info->symbol = fixup_symbol_section (info->symbol, NULL);
-
-  do_cleanups (old_chain);
+  *info = ada_lookup_symbol (verbatim.c_str (), block, domain, NULL);
 }
 
 /* Return a symbol in DOMAIN matching NAME, in BLOCK0 and enclosing
@@ -5952,13 +5933,27 @@ struct block_symbol
 ada_lookup_symbol (const char *name, const struct block *block0,
                    domain_enum domain, int *is_a_field_of_this)
 {
-  struct block_symbol info;
-
   if (is_a_field_of_this != NULL)
     *is_a_field_of_this = 0;
 
-  ada_lookup_encoded_symbol (ada_encode (ada_fold_name (name)),
-			     block0, domain, &info);
+  struct block_symbol *candidates;
+  int n_candidates;
+  struct cleanup *old_chain;
+
+  n_candidates = ada_lookup_symbol_list (name, block0, domain, &candidates);
+  old_chain = make_cleanup (xfree, candidates);
+
+  if (n_candidates == 0)
+    {
+      do_cleanups (old_chain);
+      return {};
+    }
+
+  block_symbol info = candidates[0];
+  info.symbol = fixup_symbol_section (info.symbol, NULL);
+
+  do_cleanups (old_chain);
+
   return info;
 }
 
diff --git a/gdb/testsuite/gdb.ada/info_addr_mixed_case.exp b/gdb/testsuite/gdb.ada/info_addr_mixed_case.exp
index e9fce0d93c9..7840a434b37 100644
--- a/gdb/testsuite/gdb.ada/info_addr_mixed_case.exp
+++ b/gdb/testsuite/gdb.ada/info_addr_mixed_case.exp
@@ -31,12 +31,11 @@ if ![runto "foo.adb:$bp_location" ] then {
 
 # The following test exercises the situation when uppercase letters
 # are used in the name of the symbol passed to the "info address"
-# command.  This should not make a difference, as the language is
-# Ada, and Ada is case-insensitive.
+# command.  This should not make a difference, as the language is Ada,
+# and Ada is case-insensitive.  Also, exercise both fully-qualified
+# name matching and wild matching.
 
-# commit b5ec771e60c1a0863e51eb491c85c674097e9e13 (Introduce
-# lookup_name_info and generalize Ada's FULL/WILD name matching)
-# caused the following test to fail. KFAIL it while investigating...
-setup_kfail gdb/22670 "*-*-*"
-gdb_test "info address My_Table" \
-         "Symbol \"pck\\.my_table\" is static storage at address $hex\\."
+foreach sym {"my_table" "My_Table" "pck.my_table" "Pck.My_Table"} {
+    gdb_test "info address $sym" \
+	"Symbol \"pck\\.my_table\" is static storage at address $hex\\."
+}
-- 
2.14.3

Re: [PATCH 1/3] Add gdb.ada/info_addr_mixed_case new testcase

[Joel Brobecker]

> Turns out that the Ada symbol lookup machinery is sufficiently decoupled
> from "regular" lookup that the problem is elsewhere.
> While we may still want to consider migrating that hack towards
> symbol_name_match_type::EXPRESSION, things should still work in
> principle without doing that, AFAICT.
> 
> Whether to do a full or wild match is decided based on the lookup name
> string (see name_match_type_from_name).  That was introduced basically
> as a refactor of preexisting code, IIRC.
> 
> I traced the problem to the verbatim-wrapping hack in
> ada_lookup_encoded_symbol.
> 
> See the attached patch.  It fixes gdb.ada/info_addr_mixed_case,
> and introduces no regressions for me.  WDYT?

Nice!

> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	PR gdb/22670
> 	* ada-lang.c (ada_lookup_encoded_symbol): Reimplement in terms of
> 	ada_lookup_symbol.
> 	(ada_lookup_symbol): Reimplement in terms of
> 	ada_lookup_symbol_list, bits factored out from
> 	ada_lookup_encoded_symbol.
> 
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	PR gdb/22670
> 	* gdb.ada/info_addr_mixed_case.exp: Remove kfail.  Extend test to
> 	exercise lower case too, and to exercise both full matching and
> 	wild matching.

Patch looks good to me. Intuitively, it looks like a more logical
way to perform things too. And with the new SEARCH_SYMBOL searching
mechanism coming, I can see us being able later to to avoid the "<...>"
wrapping too.

Just in case, I tested that patch against our testsuite, and I can
confirm it fixes the issue without introducing regressions :).

Thanks for having looked into this, Pedro. I will focus my attention
on creating the branch, do a review or two, and then continue analyzing
my testsuite report.

-- 
Joel

Re: [PATCH 1/3] Add gdb.ada/info_addr_mixed_case new testcase

[Pedro Alves]

On 01/05/2018 03:21 AM, Joel Brobecker wrote:

> Patch looks good to me. Intuitively, it looks like a more logical
> way to perform things too. And with the new SEARCH_SYMBOL searching
> mechanism coming, I can see us being able later to to avoid the "<...>"
> wrapping too.
> 
> Just in case, I tested that patch against our testsuite, and I can
> confirm it fixes the issue without introducing regressions :).
> 
> Thanks for having looked into this, Pedro. I will focus my attention
> on creating the branch, do a review or two, and then continue analyzing
> my testsuite report.

Great, I've pushed this one in, both master and branch.
I have fixes for the other testcases / regressions.  I'll post them
in a bit.

Thanks,
Pedro Alves

[PATCH 3/3] Add new gdb.ada/bp_c_mixed_case testcase for PR gdb/22670

[Joel Brobecker]

This patch adds a new testcase to demonstrate a regression introduced by:

    commit b5ec771e60c1a0863e51eb491c85c674097e9e13
    Date:   Wed Nov 8 14:22:32 2017 +0000
    Subject: Introduce lookup_name_info and generalize Ada's FULL/WILD name matching

The purpose of the testcase is to verify that a user can insert
a breakpoint on a C function while debugging Ada, even if the name
of the function includes uppercase letters, requiring us to use
Ada's "<...>" notation to tell the GDB that the symbol name should
be looked up verbatim.

As of the commit above, GDB is no longer finding the function:

    (gdb) break <MixedCaseFunc>
    Function "<MixedCaseFunc>" not defined.
    Make breakpoint pending on future shared library load? (y or [n])

Before the patch, the breakpoint was inserted without problem.

gdb/testsuite/ChangeLog:

        PR gdb/22670
        * gdb.ada/bp_c_mixed_case: New testcase.

Tested on x86_64-linux; generates a KPASS before the regression
was introduced, and now generates a KFAIL.
---
 gdb/testsuite/ChangeLog                            |  5 +++
 gdb/testsuite/gdb.ada/bp_c_mixed_case.exp          | 52 ++++++++++++++++++++++
 gdb/testsuite/gdb.ada/bp_c_mixed_case/bar.c        | 21 +++++++++
 .../gdb.ada/bp_c_mixed_case/foo_h731_021.adb       | 21 +++++++++
 4 files changed, 99 insertions(+)
 create mode 100644 gdb/testsuite/gdb.ada/bp_c_mixed_case.exp
 create mode 100644 gdb/testsuite/gdb.ada/bp_c_mixed_case/bar.c
 create mode 100644 gdb/testsuite/gdb.ada/bp_c_mixed_case/foo_h731_021.adb

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index a81f9da..c3b38c4 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,6 +1,11 @@
 2018-01-04  Joel Brobecker  <brobecker@adacore.com>
 
 	PR gdb/22670
+	* gdb.ada/bp_c_mixed_case: New testcase.
+
+2018-01-04  Joel Brobecker  <brobecker@adacore.com>
+
+	PR gdb/22670
 	* gdb.ada/complete.exp: Add "complete break ada" test.
 
 2018-01-04  Joel Brobecker  <brobecker@adacore.com>
diff --git a/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp b/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp
new file mode 100644
index 0000000..54c61e3
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp
@@ -0,0 +1,52 @@
+# Copyright 2018 Free Software Foundation, Inc.
+#
+# 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/>.
+
+load_lib "ada.exp"
+
+standard_ada_testfile foo_h731_021
+
+set cfile "bar"
+set csrcfile ${srcdir}/${subdir}/${testdir}/${cfile}.c
+set cobject [standard_output_file ${cfile}.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]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+if ![runto "foo_h731_021"] then {
+  perror "Couldn't run ${testfile}"
+  return
+}
+
+# Verify that the current language is Ada.
+gdb_test "show lang" \
+         "\"auto; currently 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.
+setup_kfail gdb/22670 "*-*-*"
+gdb_test "break <MixedCaseFunc>" \
+         "Breakpoint $decimal at $hex: file .*bar.c, line $decimal\\."
+
+# Resume the program's execution, verifying that it lands at the expected
+# location.
+setup_kfail gdb/22670 "*-*-*"
+gdb_test "continue" \
+         "Breakpoint $decimal, MixedCaseFunc \\(\\) at .*bar\\.c:$decimal.*"
diff --git a/gdb/testsuite/gdb.ada/bp_c_mixed_case/bar.c b/gdb/testsuite/gdb.ada/bp_c_mixed_case/bar.c
new file mode 100644
index 0000000..4bcbfa8
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_c_mixed_case/bar.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
+MixedCaseFunc (void)
+{
+}
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
new file mode 100644
index 0000000..88e0c31
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/bp_c_mixed_case/foo_h731_021.adb
@@ -0,0 +1,21 @@
+--  Copyright 2018 Free Software Foundation, Inc.
+--
+--  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/>.
+
+procedure Foo_H731_021 is
+   Procedure C_Func;
+   pragma Import (C, C_Func, "MixedCaseFunc");
+begin
+   C_Func;
+end Foo_H731_021;
-- 
2.1.4

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)

[Pedro Alves]

On 01/04/2018 08:35 AM, Joel Brobecker wrote:
> This patch adds a new testcase to demonstrate a regression introduced by:
> 
>     commit b5ec771e60c1a0863e51eb491c85c674097e9e13
>     Date:   Wed Nov 8 14:22:32 2017 +0000
>     Subject: Introduce lookup_name_info and generalize Ada's FULL/WILD name matching
> 
> The purpose of the testcase is to verify that a user can insert
> a breakpoint on a C function while debugging Ada, even if the name
> of the function includes uppercase letters, requiring us to use
> Ada's "<...>" notation to tell the GDB that the symbol name should
> be looked up verbatim.
> 
> As of the commit above, GDB is no longer finding the function:
> 
>     (gdb) break <MixedCaseFunc>
>     Function "<MixedCaseFunc>" not defined.
>     Make breakpoint pending on future shared library load? (y or [n])
> 
> Before the patch, the breakpoint was inserted without problem.
> 

Below's a fix for this one.

I've force-pushed this one along with the following on to the
users/palves/literal-matching branch, rebased on master to pick up the
other fixes that went in meanwhile.

From 439f8c51ff8f6cd9fb3bbc330a40492a15992add Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 5 Jan 2018 00:17:19 +0000
Subject: [PATCH 1/2] Fix gdb.ada/bp_c_mixed_case.exp (PR gdb/22670)

The problem here is that we were using the user-provided lookup name
literally for linkage name comparisons.  I.e., "<MixedCase>" with the
"<>"s included.  That obviously can't work since the "<>" are not
really part of the linkage name.  The original idea was that we'd use
the symbol's language to select the right symbol name matching
algorithm, but that doesn't work for Ada because it's not really
possible to unambiguously tell from the linkage name alone whether
we're dealing with Ada symbols, so Ada minsyms end up with no language
set, or sometimes C++ set.  So fix this by treating Ada mode specially
when determining the linkage name to match against.

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

	PR gdb/22670
	* minsyms.c (linkage_name_str): New function.
	(iterate_over_minimal_symbols): Use it.

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

	PR gdb/22670
	* gdb.ada/bp_c_mixed_case.exp: Remove setup_kfail calls.
---
 gdb/minsyms.c                             | 21 ++++++++++++++++++++-
 gdb/testsuite/gdb.ada/bp_c_mixed_case.exp |  4 +---
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 26c91ec8819..fded0d65e93 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -447,6 +447,25 @@ find_minimal_symbol_address (const char *name, CORE_ADDR *addr,
   return sym.minsym == NULL;
 }
 
+/* Get the lookup name form best suitable for linkage name
+   matching.  */
+
+static const char *
+linkage_name_str (const lookup_name_info &lookup_name)
+{
+  /* Unlike most languages (including C++), Ada uses the
+     encoded/linkage name as the search name recorded in symbols.  So
+     if debugging in Ada mode, prefer the Ada-encoded name.  This also
+     makes Ada's verbatim match syntax ("<...>") work, because
+     "lookup_name.name()" includes the "<>"s, while
+     "lookup_name.ada().lookup_name()" is the encoded name with "<>"s
+     stripped.  */
+  if (current_language->la_language == language_ada)
+    return lookup_name.ada ().lookup_name ().c_str ();
+
+  return lookup_name.name ().c_str ();
+}
+
 /* See minsyms.h.  */
 
 void
@@ -459,7 +478,7 @@ iterate_over_minimal_symbols (struct objfile *objf,
 
   /* The first pass is over the ordinary hash table.  */
     {
-      const char *name = lookup_name.name ().c_str ();
+      const char *name = linkage_name_str (lookup_name);
       unsigned int hash = msymbol_hash (name) % MINIMAL_SYMBOL_HASH_SIZE;
       auto *mangled_cmp
 	= (case_sensitivity == case_sensitive_on
diff --git a/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp b/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp
index 54c61e3a8e8..7787646c67f 100644
--- a/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp
+++ b/gdb/testsuite/gdb.ada/bp_c_mixed_case.exp
@@ -40,13 +40,11 @@ gdb_test "show lang" \
 # 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.
-setup_kfail gdb/22670 "*-*-*"
+# and that it inserts the breakpoint at the expected location.  See gdb/22670.
 gdb_test "break <MixedCaseFunc>" \
          "Breakpoint $decimal at $hex: file .*bar.c, line $decimal\\."
 
 # Resume the program's execution, verifying that it lands at the expected
 # location.
-setup_kfail gdb/22670 "*-*-*"
 gdb_test "continue" \
          "Breakpoint $decimal, MixedCaseFunc \\(\\) at .*bar\\.c:$decimal.*"
-- 
2.14.3

Re: 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)

[Joel Brobecker]

Hi Pedro,

On Fri, Jan 05, 2018 at 04:34:39PM +0000, Pedro Alves wrote:
> On 01/04/2018 08:35 AM, Joel Brobecker wrote:
> > This patch adds a new testcase to demonstrate a regression introduced by:
> > 
> >     commit b5ec771e60c1a0863e51eb491c85c674097e9e13
> >     Date:   Wed Nov 8 14:22:32 2017 +0000
> >     Subject: Introduce lookup_name_info and generalize Ada's FULL/WILD name matching
> > 
> > The purpose of the testcase is to verify that a user can insert
> > a breakpoint on a C function while debugging Ada, even if the name
> > of the function includes uppercase letters, requiring us to use
> > Ada's "<...>" notation to tell the GDB that the symbol name should
> > be looked up verbatim.
> > 
> > As of the commit above, GDB is no longer finding the function:
> > 
> >     (gdb) break <MixedCaseFunc>
> >     Function "<MixedCaseFunc>" not defined.
> >     Make breakpoint pending on future shared library load? (y or [n])
> > 
> > Before the patch, the breakpoint was inserted without problem.
> > 
> 
> Below's a fix for this one.

Thanks!

I confirm the test now passes for me as well :). I have a question
though:

> >From 439f8c51ff8f6cd9fb3bbc330a40492a15992add Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 5 Jan 2018 00:17:19 +0000
> Subject: [PATCH 1/2] Fix gdb.ada/bp_c_mixed_case.exp (PR gdb/22670)
> 
> The problem here is that we were using the user-provided lookup name
> literally for linkage name comparisons.  I.e., "<MixedCase>" with the
> "<>"s included.  That obviously can't work since the "<>" are not
> really part of the linkage name.  The original idea was that we'd use
> the symbol's language to select the right symbol name matching
> algorithm, but that doesn't work for Ada because it's not really
> possible to unambiguously tell from the linkage name alone whether
> we're dealing with Ada symbols, so Ada minsyms end up with no language
> set, or sometimes C++ set.  So fix this by treating Ada mode specially
> when determining the linkage name to match against.

I am wondering why minimal symbols are involved in this case,
considering that the C file was build with debugging information.
Shouldn't we be getting the function's address from the partial/full
symtabs instead?

-- 
Joel

Re: 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)

[Pedro Alves]

On 01/08/2018 03:57 AM, Joel Brobecker wrote:
> On Fri, Jan 05, 2018 at 04:34:39PM +0000, Pedro Alves wrote:

>> Below's a fix for this one.
> 
> Thanks!
> 
> I confirm the test now passes for me as well :). 

Great!

> I have a question though:
> 
>> >From 439f8c51ff8f6cd9fb3bbc330a40492a15992add Mon Sep 17 00:00:00 2001
>> From: Pedro Alves <palves@redhat.com>
>> Date: Fri, 5 Jan 2018 00:17:19 +0000
>> Subject: [PATCH 1/2] Fix gdb.ada/bp_c_mixed_case.exp (PR gdb/22670)
>>
>> The problem here is that we were using the user-provided lookup name
>> literally for linkage name comparisons.  I.e., "<MixedCase>" with the
>> "<>"s included.  That obviously can't work since the "<>" are not
>> really part of the linkage name.  The original idea was that we'd use
>> the symbol's language to select the right symbol name matching
>> algorithm, but that doesn't work for Ada because it's not really
>> possible to unambiguously tell from the linkage name alone whether
>> we're dealing with Ada symbols, so Ada minsyms end up with no language
>> set, or sometimes C++ set.  So fix this by treating Ada mode specially
>> when determining the linkage name to match against.
> 
> I am wondering why minimal symbols are involved in this case,
> considering that the C file was build with debugging information.
> Shouldn't we be getting the function's address from the partial/full
> symtabs instead?

AFAIK, GDB always worked this way for linespecs, even before my C++
wildmatching patches -- we collect symbols from both debug info and
minsyms, and coalesce them by address to avoid duplicates
(linespec.c:add_matching_symbols_to_info).  

The completion paths then try to do the same thing (though implemented
differently) [1].

I think it makes sense because:

- even if you have debug information in the binary, the debug information
  won't cover all function symbols.  Some may be internal linker-/compiler-
  generated symbols.  Or..

- ..there may be multiple symbols with the same name in different
  compilation units that all end up in the same binary/objfile.  Some may
  have debug info while others not.  E.g. (C, but applies to any language,
  or mixed languages):

static void function () {}                // in file1.c, compiled with -g
static int function (int p) { return p; } // in file2.c, compiled WITHOUT -g

  I could easily see the 'with'/'without -g' functions ending up both in
  the same objfile via static linking, for example.  We want "b <function>" to
  set a breakpoint on all the functions.

[1] - the C++ wildmatching series eliminated the divergence a bit, but
there's still a lot more duplication that there should ideally be.
One of the points of the gdb.linespec/cpcompletion.exp and
gdb.linespec/cpls-*.exp testcases is making sure that completion
and actually setting a breakpoint finds the same locations.

Let me know if this answers your question.

Thanks,
Pedro Alves

Re: 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)

[Joel Brobecker]

> > I am wondering why minimal symbols are involved in this case,
> > considering that the C file was build with debugging information.
> > Shouldn't we be getting the function's address from the partial/full
> > symtabs instead?
> 
> AFAIK, GDB always worked this way for linespecs, even before my C++
> wildmatching patches -- we collect symbols from both debug info and
> minsyms, and coalesce them by address to avoid duplicates
> (linespec.c:add_matching_symbols_to_info).  

That's true.

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.

Your patch, IIUC, handles the lookup at the minimal symbol level,
which is indeed a good thing. But shouldn't we also be finding
that same symbol through the partial/full symtab search? I have
a feeling that your minimal symbol patch might be hiding a bug
in the search for the symbol, at least from the linespec module.

I did a bit of debugging this morning, first with the following
snapshot, which is shortly before the wild-matching patch series:

    commit b346cb961f729e2955391513a5b05eaf02b308ea
    Author: GDB Administrator <gdbadmin@sourceware.org>
    Date:   Wed Nov 8 00:00:20 2017 +0000

The function iterate_over_all_matching_symtabs finds the function
in the bar.c's partial symtab because the matching function is...

           [&] (const char *symbol_name)
           {
             return symbol_name_cmp (symbol_name, name) == 0;
           },

... where name, in this case is "MixedCaseFunc" -- The "<>" has been
stripped. They got stripped by linespec.c::find_linespec_symbols
when it took that name and converted it to a lookup name via:

  if (state->language->la_language == language_ada)
    {
      /* In Ada, the symbol lookups are performed using the encoded
         name rather than the demangled name.  */
      ada_lookup_storage = ada_name_for_lookup (name);
      lookup_name = ada_lookup_storage.c_str ();
    }
  else
    {
      lookup_name = demangle_for_lookup (name,
                                         state->language->la_language,
                                         demangle_storage);
    }

In the newer version, find_linespec_symbols gets passed the lookup_name
directly, and that lookup_name is now "<MixedCaseFunc>". Those extra
"<...>" are what eventually gets in the way when we compare this
lookup_name against the partial's symbols name (in
default_symbol_name_matcher, which does an strncmp_iw_with_mode
comparison, IIUC).

The call to find_linespec_symbols comes from linespace_parse_basic,
which has:

  /* Try looking it up as a function/method.  */
  find_linespec_symbols (PARSER_STATE (parser),
                         PARSER_RESULT (parser)->file_symtabs, name,
                         PARSER_EXPLICIT (parser)->func_name_match_type,
                         &symbols, &minimal_symbols);

I really hate to be stopping the investigation at this point, as
I feel I am onto something, but I am running out of time for today.

The part where I am not sure yet is whether we should be transforming
"name" into a "lookup_name" before calling find_linespec_symbols, or
whether we should be handling the angle brackets during the symbol
comparison... Or something else entirely! This is still all fairly
new to me...

Note that I was thinkg we would need to be stripping the executable
for us to demonstrate an error, but in fact, this is what happens
if I use "print" instead of "break":

    (gdb) p <MixedCaseFunc>
    $1 = {<text variable, no debug info>} 0x4024dc <MixedCaseFunc>

With the snapshot prior to the patch series, GDB knows that
MixedCaseFunc is a function without parameters, and the expression
above means calling it. As I was debugging without having started
the inferior, I got the following (expected) error:

    (gdb) print  <MixedCaseFunc>
    You can't do that without a process to debug.

in the bp_c_mixed_case.exp, we should see GDB telling us that
we stopped on our MixedCaseFunc breakpoint while evaluating
a function call...

Does this make some kind of sense to you? I can get back to this
for more digging again tomorrow.

Thanks!
-- 
Joel

Re: 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)

[Pedro Alves]

On 01/09/2018 09:46 AM, Joel Brobecker wrote:
>>> I am wondering why minimal symbols are involved in this case,
>>> considering that the C file was build with debugging information.
>>> Shouldn't we be getting the function's address from the partial/full
>>> symtabs instead?
>>
>> AFAIK, GDB always worked this way for linespecs, even before my C++
>> wildmatching patches -- we collect symbols from both debug info and
>> minsyms, and coalesce them by address to avoid duplicates
>> (linespec.c:add_matching_symbols_to_info).  
> 
> That's true.
> 
> 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.

> 
> Your patch, IIUC, handles the lookup at the minimal symbol level,
> which is indeed a good thing. But shouldn't we also be finding
> that same symbol through the partial/full symtab search? I have
> a feeling that your minimal symbol patch might be hiding a bug
> in the search for the symbol, at least from the linespec module.
> 
> I did a bit of debugging this morning, first with the following
> snapshot, which is shortly before the wild-matching patch series:
> 
>     commit b346cb961f729e2955391513a5b05eaf02b308ea
>     Author: GDB Administrator <gdbadmin@sourceware.org>
>     Date:   Wed Nov 8 00:00:20 2017 +0000
> 
> The function iterate_over_all_matching_symtabs finds the function
> in the bar.c's partial symtab because the matching function is...
> 
>            [&] (const char *symbol_name)
>            {
>              return symbol_name_cmp (symbol_name, name) == 0;
>            },
> 
> ... where name, in this case is "MixedCaseFunc" -- The "<>" has been
> stripped. They got stripped by linespec.c::find_linespec_symbols
> when it took that name and converted it to a lookup name via:
> 
>   if (state->language->la_language == language_ada)
>     {
>       /* In Ada, the symbol lookups are performed using the encoded
>          name rather than the demangled name.  */
>       ada_lookup_storage = ada_name_for_lookup (name);
>       lookup_name = ada_lookup_storage.c_str ();
>     }
>   else
>     {
>       lookup_name = demangle_for_lookup (name,
>                                          state->language->la_language,
>                                          demangle_storage);
>     }
> 
> In the newer version, find_linespec_symbols gets passed the lookup_name
> directly, and that lookup_name is now "<MixedCaseFunc>". Those extra
> "<...>" are what eventually gets in the way when we compare this
> lookup_name against the partial's symbols name (in
> default_symbol_name_matcher, which does an strncmp_iw_with_mode
> comparison, IIUC).
> 
> The call to find_linespec_symbols comes from linespace_parse_basic,
> which has:
> 
>   /* Try looking it up as a function/method.  */
>   find_linespec_symbols (PARSER_STATE (parser),
>                          PARSER_RESULT (parser)->file_symtabs, name,
>                          PARSER_EXPLICIT (parser)->func_name_match_type,
>                          &symbols, &minimal_symbols);
> 
> I really hate to be stopping the investigation at this point, as
> I feel I am onto something, but I am running out of time for today.
> 
> The part where I am not sure yet is whether we should be transforming
> "name" into a "lookup_name" before calling find_linespec_symbols, or
> whether we should be handling the angle brackets during the symbol
> comparison... Or something else entirely! This is still all fairly
> new to me...
> 
> Note that I was thinkg we would need to be stripping the executable
> for us to demonstrate an error, but in fact, this is what happens
> if I use "print" instead of "break":
> 
>     (gdb) p <MixedCaseFunc>
>     $1 = {<text variable, no debug info>} 0x4024dc <MixedCaseFunc>
> 
> With the snapshot prior to the patch series, GDB knows that
> MixedCaseFunc is a function without parameters, and the expression
> above means calling it. As I was debugging without having started
> the inferior, I got the following (expected) error:
> 
>     (gdb) print  <MixedCaseFunc>
>     You can't do that without a process to debug.
> 
> in the bp_c_mixed_case.exp, we should see GDB telling us that
> we stopped on our MixedCaseFunc breakpoint while evaluating
> a function call...
> 
> 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.

> I can get back to this
> for more digging again tomorrow.
Thanks,
Pedro Alves

Re: 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)

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 1953 bytes --]

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

[-- Attachment #2: 0001-Ada-make-verbatim-matcher-override-other-language-ma.patch --]
[-- Type: text/x-patch, Size: 10769 bytes --]

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

Re: 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)

[Pedro Alves]

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

Re: 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)

[Joel Brobecker]

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!
-- 
Joel

Re: 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)

[Pedro Alves]

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

Re: 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)

[Joel Brobecker]

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

-- 
Joel

[PATCH 2/3] Add "complete break ada" test to gdb.ada/complete.exp

[Joel Brobecker]

This patch adds a new test to demonstrate a regression introduced by:

    commit b5ec771e60c1a0863e51eb491c85c674097e9e13
    Date:   Wed Nov 8 14:22:32 2017 +0000
    Subject: Introduce lookup_name_info and generalize Ada's FULL/WILD name matching

The original purpose of the new test is to exercise the "complete"
command with an expression for which a large number of matches are
returned and to verify that each match returned is a plausible match.
In this particular case, the commit above causes GDB to generate
additional matches which should in fact not appear in the list
(internally generated symbols, or symbols that should be enclosed
between "<...>"). These extraneous entries are easy to spot, because
they have uppercase characters, such as:

    break ada__stringsS
    break ada__strings__R11s
    [etc]

For now, the new test is KFAIL'ed, to avoid generating a new FAIL
while we work on fixing that regression.

gdb/testsuite/ChangeLog:

        PR gdb/22670
        * gdb.ada/complete.exp: Add "complete break ada" test.

Tested on x86_64-linux with GDB built before and after the patch
that caused the regression (b5ec771e60c1a0863e51eb491c85c674097e9e13).
The test passes before the regression, and generates a KFAIL after.
---
 gdb/testsuite/ChangeLog            |  5 +++++
 gdb/testsuite/gdb.ada/complete.exp | 21 +++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b1be7e3..a81f9da 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,6 +1,11 @@
 2018-01-04  Joel Brobecker  <brobecker@adacore.com>
 
 	PR gdb/22670
+	* gdb.ada/complete.exp: Add "complete break ada" test.
+
+2018-01-04  Joel Brobecker  <brobecker@adacore.com>
+
+	PR gdb/22670
 	* gdb.ada/info_addr_mixed_case: New testcase.
 
 2018-01-03  Xavier Roirand  <roirand@adacore.com>
diff --git a/gdb/testsuite/gdb.ada/complete.exp b/gdb/testsuite/gdb.ada/complete.exp
index 9b64d15..c1f22c2 100644
--- a/gdb/testsuite/gdb.ada/complete.exp
+++ b/gdb/testsuite/gdb.ada/complete.exp
@@ -204,3 +204,24 @@ test_gdb_complete "ambiguous_f" \
                   "p ambiguous_func"
 test_gdb_complete "ambiguous_func" \
                   "p ambiguous_func"
+
+# Perform a test intented to verify the behavior where the number
+# of possible completions is very large.  The goal is not to verify
+# precisely the list returned by the complete command (this depends
+# on too many parameters -- targets, compiler version, runtime, etc).
+# However, we want to sanity-check each one of them, knowing that
+# each result should start with "break ada" and that the proposed
+# completion should look like a valid symbol name (in particular,
+# no uppercase letters...).
+
+gdb_test_no_output "set max-completions unlimited"
+
+set test "complete break ada"
+gdb_test_multiple "$test" $test {
+    -re "^$test$eol\(break ada(\[a-z0-9._\])*$eol\)+$gdb_prompt $" {
+        pass $test
+    }
+    -re "\[A-Z\].*$gdb_prompt $" {
+        kfail gdb/22670 $test
+    }
+}
-- 
2.1.4

[PATCH] Fix gdb.ada/complete.exp's "complete break ada" test (PR, gdb/22670) (Re: [PATCH 2/3] Add "complete break ada" test to gdb.ada/complete.exp)

[Pedro Alves]

On 01/04/2018 08:35 AM, Joel Brobecker wrote:
> This patch adds a new test to demonstrate a regression introduced by:
> 
>     commit b5ec771e60c1a0863e51eb491c85c674097e9e13
>     Date:   Wed Nov 8 14:22:32 2017 +0000
>     Subject: Introduce lookup_name_info and generalize Ada's FULL/WILD name matching
> 
> The original purpose of the new test is to exercise the "complete"
> command with an expression for which a large number of matches are
> returned and to verify that each match returned is a plausible match.
> In this particular case, the commit above causes GDB to generate
> additional matches which should in fact not appear in the list
> (internally generated symbols, or symbols that should be enclosed
> between "<...>"). These extraneous entries are easy to spot, because
> they have uppercase characters, such as:
> 
>     break ada__stringsS
>     break ada__strings__R11s
>     [etc]
> 
> For now, the new test is KFAIL'ed, to avoid generating a new FAIL
> while we work on fixing that regression.

And here's the fix for this one.  Also pushed to the
users/palves/literal-matching branch.

I think I addressed all the Ada regressions you reported.  Let me
know if I missed some.

From 2805fc780e497535f3966dea932023cad9b92b61 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 5 Jan 2018 12:21:25 +0000
Subject: [PATCH 2/2] Fix gdb.ada/complete.exp's "complete break ada" test (PR
 gdb/22670)

This patch fixes the regression covered by the test added by:

    commit 344420da6beac1e0b2f7964e7101f8dcdb509b0d
    Date: Thu Jan 4 03:30:37 2018 -0500
    Subject: Add "complete break ada" test to gdb.ada/complete.exp

The regression had been introduced by:

    commit b5ec771e60c1a0863e51eb491c85c674097e9e13
    Date:   Wed Nov 8 14:22:32 2017 +0000
    Subject: Introduce lookup_name_info and generalize Ada's FULL/WILD name matching

The gist of it is that linespec completion in Ada mode is generating
additional matches that should not appear in the match list
(internally generated symbols, or symbols that should be enclosed
between "<...>").  These extraneous entries have uppercase characters, such as:

    break ada__stringsS
    break ada__strings__R11s
    [etc]

These matches come from minimal symbols.  The problem is that Ada
minsyms end up with no language set (language_auto), and thus we end
up using the generic symbol name matcher for those instead of Ada's.
We already had a special case for in compare_symbol_name to handle
this, but it was limited to expressions, while the case at hand is
completing a linespec.  Fix this by applying the special case to
linespec completion as well.  I.e., remove the EXPRESSION check from
compare_symbol_name.  That alone turns out to not be sufficient still
-- GDB would still show a couple entries that shouldn't be there:

~~
    break ada__exceptions__exception_data__append_info_exception_name__2Xn
    break ada__exceptions__exception_data__exception_name_length__2Xn
~~

The reason is that these minimal symbols end up with their language
set to language_cplus / C++, because those encoded names manage to
demangle successfully as C++ symbols (using an old C++ mangling
scheme):

  $ echo ada__exceptions__exception_data__append_info_exception_name__2Xn | c++filt
  Xn::ada__exceptions__exception_data__append_info_exception_name(void)

It's unfortunate that Ada's encoding scheme doesn't start with some
unique prefix like "_Z" in the C++ Itanium ABI mangling scheme.  For
now, paper over that by treating C++ minsyms as Ada minsyms.

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

        PR gdb/22670
	* ada-lang.c (ada_collect_symbol_completion_matches): If the
	minsym's language is language_auto or language_cplus, pass down
	language_ada instead.
	* symtab.c (compare_symbol_name): Don't frob symbol language here.

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

        PR gdb/22670
	* gdb.ada/complete.exp ("complete break ada"): Replace kfail with
	a fail.
---
 gdb/ada-lang.c                     | 19 ++++++++++++++++++-
 gdb/symtab.c                       | 16 +---------------
 gdb/testsuite/gdb.ada/complete.exp |  4 ++--
 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 622cfd0a81e..ab1083830ed 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -6499,8 +6499,25 @@ ada_collect_symbol_completion_matches (completion_tracker &tracker,
     if (completion_skip_symbol (mode, msymbol))
       continue;
 
+    language symbol_language = MSYMBOL_LANGUAGE (msymbol);
+
+    /* Ada minimal symbols won't have their language set to Ada.  If
+       we let completion_list_add_name compare using the
+       default/C-like matcher, then when completing e.g., symbols in a
+       package named "pck", we'd match internal Ada symbols like
+       "pckS", which are invalid in an Ada expression, unless you wrap
+       them in '<' '>' to request a verbatim match.
+
+       Unfortunately, some Ada encoded names successfully demangle as
+       C++ symbols (using an old mangling scheme), such as "name__2Xn"
+       -> "Xn::name(void)" and thus some Ada minimal symbols end up
+       with the wrong language set.  Paper over that issue here.  */
+    if (symbol_language == language_auto
+	|| symbol_language == language_cplus)
+      symbol_language = language_ada;
+
     completion_list_add_name (tracker,
-			      MSYMBOL_LANGUAGE (msymbol),
+			      symbol_language,
 			      MSYMBOL_LINKAGE_NAME (msymbol),
 			      lookup_name, text, word);
   }
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 146dc2e4213..2fe249682f2 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4704,21 +4704,7 @@ compare_symbol_name (const char *symbol_name, language symbol_language,
 		     const lookup_name_info &lookup_name,
 		     completion_match_result &match_res)
 {
-  const language_defn *lang;
-
-  /* If we're completing for an expression and the symbol doesn't have
-     an explicit language set, fallback to the current language.  Ada
-     minimal symbols won't have their language set to Ada, for
-     example, and if we compared using the default/C-like matcher,
-     then when completing e.g., symbols in a package named "pck", we'd
-     match internal Ada symbols like "pckS", which are invalid in an
-     Ada expression, unless you wrap them in '<' '>' to request a
-     verbatim match.  */
-  if (symbol_language == language_auto
-      && lookup_name.match_type () == symbol_name_match_type::EXPRESSION)
-    lang = current_language;
-  else
-    lang = language_def (symbol_language);
+  const language_defn *lang = language_def (symbol_language);
 
   symbol_name_matcher_ftype *name_match
     = language_get_symbol_name_matcher (lang, lookup_name);
diff --git a/gdb/testsuite/gdb.ada/complete.exp b/gdb/testsuite/gdb.ada/complete.exp
index c1f22c2a3e4..cb9e4ae7ffc 100644
--- a/gdb/testsuite/gdb.ada/complete.exp
+++ b/gdb/testsuite/gdb.ada/complete.exp
@@ -212,7 +212,7 @@ test_gdb_complete "ambiguous_func" \
 # However, we want to sanity-check each one of them, knowing that
 # each result should start with "break ada" and that the proposed
 # completion should look like a valid symbol name (in particular,
-# no uppercase letters...).
+# no uppercase letters...).  See gdb/22670.
 
 gdb_test_no_output "set max-completions unlimited"
 
@@ -222,6 +222,6 @@ gdb_test_multiple "$test" $test {
         pass $test
     }
     -re "\[A-Z\].*$gdb_prompt $" {
-        kfail gdb/22670 $test
+	fail "$test (gdb/22670)"
     }
 }
-- 
2.14.3

Re: [PATCH] Fix gdb.ada/complete.exp's "complete break ada" test (PR, gdb/22670) (Re: [PATCH 2/3] Add "complete break ada" test to gdb.ada/complete.exp)

[Joel Brobecker]

Hi Pedro,

> >From 2805fc780e497535f3966dea932023cad9b92b61 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 5 Jan 2018 12:21:25 +0000
> Subject: [PATCH 2/2] Fix gdb.ada/complete.exp's "complete break ada" test (PR
>  gdb/22670)
[...]
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
>         PR gdb/22670
> 	* ada-lang.c (ada_collect_symbol_completion_matches): If the
> 	minsym's language is language_auto or language_cplus, pass down
> 	language_ada instead.
> 	* symtab.c (compare_symbol_name): Don't frob symbol language here.
> 
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
>         PR gdb/22670
> 	* gdb.ada/complete.exp ("complete break ada"): Replace kfail with
> 	a fail.

Thanks Pedro!

This patch looks good to me. In a way, I'm glad the Ada exception
is moving back to an ada-* file...

FTR, I tested this patch on x86_64-linux against AdaCore's testsuite
as well.

-- 
Joel