diff mbox

[RFC] regresssion(internal-error) printing subprogram argument

Message ID 07a154ef-b6f5-ad86-1410-a73620de4b5b@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves Dec. 15, 2017, 6:57 p.m. UTC 
  On 12/15/2017 11:02 AM, Pedro Alves wrote:
> Hi Joel,
> 
> I woke up realizing that I completely forgot that psymbols have no
> overload/function parameter info in C++, so a straight strcmp probably
> doesn't work properly for C++.  Indeed, I remembered now to do
> "maint check-psymtabs" when debugging gdb, and I get back a few
> problems:
> 
> (top-gdb) maint check-psymtabs 
> Global symbol `__gnu_cxx::operator!=<symtab_and_line*, std::vector<symtab_and_line> >' only found in src/gdb/cli/cli-cmds.c psymtab
> Global symbol `__gnu_cxx::operator-<const symtab_and_line*, std::vector<symtab_and_line> >' only found in src/gdb/cli/cli-cmds.c psymtab
> [...]
> 
> Maybe what we need is to be a little less aggressive then and
> add a new symbol_name_match_type::SEARCH_SYMBOL instead that takes as
> input a non-user-input search symbol like symbol_name_match_type::LITERAL
> was, and then we skip any decoding/demangling steps (like LITERAL) and make:
>   - Ada treat that as a verbatim match,
>   - other languages treat it as symbol_name_match_type::FULL.
> 
> I can't look at this right now, though I'll try to play with it a bit
> more later today.  I'm a bit worried on timing since I'm going to be
> away next week (today's my last official day before xmas holiday).  :-/
> 

Errr, it turns out that I had done the above on the wrong branch...
I get the above output even if I use the system gdb (7.10) to debug
the new gdb, and then do "maint check-psymtabs".  So those are not
a regression.

However, (as suspected) with the LITERAL patch things indeed got
worse for C++:

Global symbol `error' only found in src/gdb/common/errors.c psymtab
Global symbol `internal_error' only found in src/gdb/common/errors.c psymtab
Global symbol `internal_warning' only found in src/gdb/common/errors.c psymtab
Global symbol `warning' only found in src/gdb/common/errors.c psymtab
Static symbol `info_command' only found in src/gdb/cli/cli-cmds.c psymtab
Static symbol `show_command' only found in src/gdb/cli/cli-cmds.c psymtab
Static symbol `help_command' only found in src/gdb/cli/cli-cmds.c psymtab
Static symbol `complete_command' only found in src/gdb/cli/cli-cmds.c psymtab
Static symbol `show_version' only found in src/gdb/cli/cli-cmds.c psymtab
Static symbol `show_configuration' only found in src/gdb/cli/cli-cmds.c psymtab
Static symbol `pwd_command' only found in src/gdb/cli/cli-cmds.c psymtab
Static symbol `show_script_ext_mode' only found in src/gdb/cli/cli-cmds.c psymtab
Static symbol `source_script_from_stream' only found in src/gdb/cli/cli-cmds.c psymtab
[...many more...]

With the patch below applied on top, which is a minimal version of
the design change I suggested (LITERAL -> SEARCH_SYMBOL), I now get
again the same "maint check-psymtabs" output that I get with system gdb.
So seems like this is on the good track.

Your internal-error test still passes as well as the new
"maint check-psymtabs" for gdb.ada/ testcase.

I've pushed the current state to users/palves/literal-matching.

I probably won't be able to look at this more today.

From 6b4025034b0b0e8b2511e4c355365c9ff69be822 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 15 Dec 2017 17:46:56 +0000
Subject: [PATCH] Move literal matching to Ada

---
 gdb/ada-lang.c   |  8 ++++++++
 gdb/cp-support.c |  1 +
 gdb/language.c   |  9 +++------
 gdb/symtab.c     | 12 ++++++++----
 4 files changed, 20 insertions(+), 10 deletions(-)

Comments

Joel Brobecker Jan. 3, 2018, 4:33 a.m. UTC | #1 
Hi Pedro,

> > Maybe what we need is to be a little less aggressive then and
> > add a new symbol_name_match_type::SEARCH_SYMBOL instead that takes as
> > input a non-user-input search symbol like symbol_name_match_type::LITERAL
> > was, and then we skip any decoding/demangling steps (like LITERAL) and make:
> >   - Ada treat that as a verbatim match,
> >   - other languages treat it as symbol_name_match_type::FULL.
[...]
> With the patch below applied on top, which is a minimal version of
> the design change I suggested (LITERAL -> SEARCH_SYMBOL), I now get
> again the same "maint check-psymtabs" output that I get with system gdb.
> So seems like this is on the good track.
> 
> Your internal-error test still passes as well as the new
> "maint check-psymtabs" for gdb.ada/ testcase.

Sounds like we are indeed on the right track!

I ran the changes through both the official testsuite and AdaCore's
testsuite, and I can confirm no regression :). Is there something
else I can do to help?
Joel Brobecker Jan. 4, 2018, 9:48 a.m. UTC | #2 
Hi Pedro,

FYI, I just pushed 3 commits that add more testing in gdb.ada
demonstrating issues introduced by the wild-matching patch series
I hadn't noticed before. See:

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

As hinted by the ChangeLog entries, I have also created PR gdb/22670
so as to be able to KFAIL the expected failures. And now that we have
this PR, we can use it to track the fixes as well ;-).

I also pushed the patch which adds "maint check psymtabs" testing;
also with a setup_kfail.

Here is my plan: I will continue going through the results I get
when running AdaCore's GDB testsuite (almost there). If there are
other tests related to this patch series, I will try to contribute
those as well. Once I'm doing analyzing the current results, I will
start investigating those failures one by one, and let you know
what I find. For now, all I know is that litteral matching is not
enough to fix those, so I am not sure what is happening just yet.
I will let you know!
Pedro Alves Jan. 4, 2018, 12:39 p.m. UTC | #3 
Hi Joel,
On 01/04/2018 09:48 AM, Joel Brobecker wrote:
> Hi Pedro,
> 
> FYI, I just pushed 3 commits that add more testing in gdb.ada
> demonstrating issues introduced by the wild-matching patch series
> I hadn't noticed before. See:
> 
> https://www.sourceware.org/ml/gdb-patches/2018-01/msg00052.html
> 
> As hinted by the ChangeLog entries, I have also created PR gdb/22670
> so as to be able to KFAIL the expected failures. And now that we have
> this PR, we can use it to track the fixes as well ;-).
> 
> I also pushed the patch which adds "maint check psymtabs" testing;
> also with a setup_kfail.
> 
> Here is my plan: I will continue going through the results I get
> when running AdaCore's GDB testsuite (almost there). If there are
> other tests related to this patch series, I will try to contribute
> those as well. Once I'm doing analyzing the current results, I will
> start investigating those failures one by one, and let you know
> what I find. For now, all I know is that litteral matching is not
> enough to fix those, so I am not sure what is happening just yet.
> I will let you know!

Thanks!  I'm catching up to this and will take a look at the
new tests.

Pedro Alves
diff mbox

Patch

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 9e637eb..5f4e255 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -14065,12 +14065,20 @@  ada_symbol_name_matches (const char *symbol_search_name,
 				     comp_match_res);
 }
 
+bool
+literal_symbol_name_matcher (const char *symbol_search_name,
+			     const lookup_name_info &lookup_name,
+			     completion_match_result *comp_match_res);
+
 /* Implement the "la_get_symbol_name_matcher" language_defn method for
    Ada.  */
 
 static symbol_name_matcher_ftype *
 ada_get_symbol_name_matcher (const lookup_name_info &lookup_name)
 {
+  if (lookup_name.match_type () == symbol_name_match_type::LITERAL)
+    return literal_symbol_name_matcher;
+
   if (lookup_name.completion_mode ())
     return ada_symbol_name_matches;
   else
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 34a8439..7c14423 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -1793,6 +1793,7 @@  cp_get_symbol_name_matcher (const lookup_name_info &lookup_name)
     {
     case symbol_name_match_type::FULL:
     case symbol_name_match_type::EXPRESSION:
+    case symbol_name_match_type::LITERAL:
       return cp_fq_symbol_name_matches;
     case symbol_name_match_type::WILD:
       return cp_symbol_name_matches;
diff --git a/gdb/language.c b/gdb/language.c
index 5bcdfb9..44afdef 100644
--- a/gdb/language.c
+++ b/gdb/language.c
@@ -727,7 +727,7 @@  default_symbol_name_matcher (const char *symbol_search_name,
 /* A name matcher that matches the symbol name exactly, with
    strcmp.  */
 
-static bool
+bool
 literal_symbol_name_matcher (const char *symbol_search_name,
 			     const lookup_name_info &lookup_name,
 			     completion_match_result *comp_match_res)
@@ -753,13 +753,10 @@  symbol_name_matcher_ftype *
 language_get_symbol_name_matcher (const language_defn *lang,
 				  const lookup_name_info &lookup_name)
 {
-  if (lookup_name.match_type () == symbol_name_match_type::LITERAL)
-    return literal_symbol_name_matcher;
-
   if (lang->la_get_symbol_name_matcher != nullptr)
     return lang->la_get_symbol_name_matcher (lookup_name);
-
-  return default_symbol_name_matcher;
+  else
+    return default_symbol_name_matcher;
 }
 
 /* Define the language that is no language.  */
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 78018a1..6957757 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -1775,14 +1775,18 @@  demangle_for_lookup_info::demangle_for_lookup_info
 
       if (without_params != NULL)
 	{
-	  m_demangled_name = demangle_for_lookup (without_params.get (),
-						  lang, storage);
+	  if (lookup_name.match_type () != symbol_name_match_type::LITERAL)
+	    m_demangled_name = demangle_for_lookup (without_params.get (),
+						    lang, storage);
 	  return;
 	}
     }
 
-  m_demangled_name = demangle_for_lookup (lookup_name.name ().c_str (),
-					  lang, storage);
+  if (lookup_name.match_type () == symbol_name_match_type::LITERAL)
+    m_demangled_name = lookup_name.name ();
+  else
+    m_demangled_name = demangle_for_lookup (lookup_name.name ().c_str (),
+					    lang, storage);
 }
 
 /* See symtab.h.  */