diff mbox

[COMMITTED] Revert fix to symtab/17602, bigger fix needed

Message ID m3fvcu5ued.fsf@sspiff.org
State Committed
Headers show

Commit Message

Doug Evans Dec. 5, 2014, 9:16 a.m. UTC
Hi.

This patch caused a regression in gdb.ada/operator_bp.exp.

https://sourceware.org/ml/gdb-patches/2014-11/msg00642.html

There's still a bug here, but the deeper I dig the bigger the problem I find.
Wrong comments, confusing code, and on and on ...
[More than once I've gotten to the point of wondering "How does this
work at all?" :-(]

So I'm going to revert this patch for now and work on a better fix.

2014-12-05  Doug Evans  <xdje42@gmail.com>

	Revert:
	PR symtab/17602
	* linespec.c (iterate_name_matcher): Fix arguments to symbol_name_cmp.

Comments

Doug Evans Dec. 5, 2014, 5:20 p.m. UTC | #1
On Fri, Dec 5, 2014 at 1:16 AM, Doug Evans <xdje42@gmail.com> wrote:
> Hi.
>
> This patch caused a regression in gdb.ada/operator_bp.exp.
>
> https://sourceware.org/ml/gdb-patches/2014-11/msg00642.html
>
> There's still a bug here, but the deeper I dig the bigger the problem I find.
> Wrong comments, confusing code, and on and on ...
> [More than once I've gotten to the point of wondering "How does this
> work at all?" :-(]
>
> So I'm going to revert this patch for now and work on a better fix.
>
> 2014-12-05  Doug Evans  <xdje42@gmail.com>
>
>         Revert:
>         PR symtab/17602
>         * linespec.c (iterate_name_matcher): Fix arguments to symbol_name_cmp.
>
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index af958dc..82384ca 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -982,12 +982,7 @@ iterate_name_matcher (const char *name, void *d)
>  {
>    const struct symbol_matcher_data *data = d;
>
> -  /* The order of arguments we pass to symbol_name_cmp is important as
> -     strcmp_iw, a typical value for symbol_name_cmp, only performs special
> -     processing of '(' to remove overload info on the first argument and not
> -     the second.  The first argument is what the user provided, the second
> -     argument is what came from partial syms / .gdb_index.  */
> -  if (data->symbol_name_cmp (data->lookup_name, name) == 0)
> +  if (data->symbol_name_cmp (name, data->lookup_name) == 0)
>      return 1; /* Expand this symbol's symbol table.  */
>    return 0; /* Skip this symbol.  */
>  }

fyi,
I've added text to the PR
to show how to see the bug.

https://sourceware.org/bugzilla/show_bug.cgi?id=17602#c4

If anyone has thoughts on how to fix this (beyond a simple "quick fix")
I'm all ears.  I've got a dozen symtab-related sandboxes in flight,
and I need to set this one aside for now.

ISTM a proper fix will involve a significant cleanup including
either getting rid of SYMBOL_SEARCH_NAME, and/or
establishing a lot more clarification on how symbol
comparisons work throughout gdb.

For starters, we have both symbol_compare_ftype
and symbol_name_cmp_ftype.
Let's get rid of one of them.
[But this needs to be more than just a simple
mechanical removal of symbol_name_cmp_type,
the one I think we should remove.
It also needs to involve establishing clarity
on what the arguments to symbol_compare_ftype
REALLY are, and are not.
And then auditing all of gdb to verify the code
adheres to those rules.]
Doug Evans Dec. 5, 2014, 5:38 p.m. UTC | #2
On Fri, Dec 5, 2014 at 9:20 AM, Doug Evans <xdje42@gmail.com> wrote:
> For starters, we have both symbol_compare_ftype
> and symbol_name_cmp_ftype.
> Let's get rid of one of them.

[sorry for the followup]

FAOD, Or, if we do need two,
then let's at least add a lot more clarity
to distinguish the different use-cases.
diff mbox

Patch

diff --git a/gdb/linespec.c b/gdb/linespec.c
index af958dc..82384ca 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -982,12 +982,7 @@  iterate_name_matcher (const char *name, void *d)
 {
   const struct symbol_matcher_data *data = d;
 
-  /* The order of arguments we pass to symbol_name_cmp is important as
-     strcmp_iw, a typical value for symbol_name_cmp, only performs special
-     processing of '(' to remove overload info on the first argument and not
-     the second.  The first argument is what the user provided, the second
-     argument is what came from partial syms / .gdb_index.  */
-  if (data->symbol_name_cmp (data->lookup_name, name) == 0)
+  if (data->symbol_name_cmp (name, data->lookup_name) == 0)
     return 1; /* Expand this symbol's symbol table.  */
   return 0; /* Skip this symbol.  */
 }