[RFA] Use FILENAME_CMP to compare filenames in compare_search_syms.

[RFA] Use FILENAME_CMP to compare filenames in compare_search_syms.

[Joel Brobecker]

Hello,

While working on an unrelated issue, I noticed that two symbols were
sorted differently on Windows, compared to the other Unix platforms.
I tracked it down to compare_search_syms which compares filenames
using a plain strcmp instead of FILENAME_CMP.

Not sure how to create a testcase...

gdb/ChangeLog:

        * symtab.c (compare_search_syms): Use FILENAME_CMP instead of
        strcmp to compare two symtab filenames.

Tested on x86-windows and x86_64-linux, no regression.
OK to apply?

Thanks,
-- 
Joel

---
 gdb/symtab.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index dbff042..93d29d6 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3336,7 +3336,7 @@ compare_search_syms (const void *sa, const void *sb)
   struct symbol_search *sym_b = *(struct symbol_search **) sb;
   int c;
 
-  c = strcmp (sym_a->symtab->filename, sym_b->symtab->filename);
+  c = FILENAME_CMP (sym_a->symtab->filename, sym_b->symtab->filename);
   if (c != 0)
     return c;
 
-- 
1.8.1.2

Re: [RFA] Use FILENAME_CMP to compare filenames in compare_search_syms.

[Pedro Alves]

On 10/01/2013 10:07 AM, Joel Brobecker wrote:
> Hello,
> 
> While working on an unrelated issue, I noticed that two symbols were
> sorted differently on Windows, compared to the other Unix platforms.
> I tracked it down to compare_search_syms which compares filenames
> using a plain strcmp instead of FILENAME_CMP.
> 
> Not sure how to create a testcase...
> 
> gdb/ChangeLog:
> 
>         * symtab.c (compare_search_syms): Use FILENAME_CMP instead of
>         strcmp to compare two symtab filenames.
> 
> Tested on x86-windows and x86_64-linux, no regression.
> OK to apply?

I'd say that use of FILENAME_CMP instead of strcmp is an obvious change.

However, this is not complete.  This function is called by:

/* Sort the NFOUND symbols in list FOUND and remove duplicates.
   The duplicates are freed, and the new list is returned in
   *NEW_HEAD, *NEW_TAIL.  */

static void
sort_search_symbols_remove_dups (struct symbol_search *found, int nfound,
				 struct symbol_search **new_head,
				 struct symbol_search **new_tail)
{
...
  qsort (symbols, nfound, sizeof (struct symbol_search *),
	 compare_search_syms);

So this is sorting symbols, to then walk over the sorted list
linearly and remove dups.  That happens right afterwards:

  /* Collapse out the dups.  */
  for (i = 1, j = 1; i < nfound; ++i)
    {
      if (! search_symbols_equal (symbols[j - 1], symbols[i]))
	symbols[j++] = symbols[i];
      else
	xfree (symbols[i]);
    }

So the compare_search_syms and search_symbols_equal predicates
_must_ agree.  And, lo, search_symbols_equal also has that strcmp
that would need adjustment as well:

 static int
 search_symbols_equal (const struct symbol_search *a,
 		      const struct symbol_search *b)
 {
   return (strcmp (a->symtab->filename, b->symtab->filename) == 0
 	  && a->block == b->block
 	  && strcmp (SYMBOL_PRINT_NAME (a->symbol),
 		     SYMBOL_PRINT_NAME (b->symbol)) == 0);
 }


But really, so that this sort of out-of-sync bugs doesn't
happen, it'd be better if search_symbols_equal were reimplemented
in terms of compare_search_syms or even be eliminated.

-- 
Pedro Alves

Re: [RFA] Use FILENAME_CMP to compare filenames in compare_search_syms.

[Joel Brobecker]

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

> I'd say that use of FILENAME_CMP instead of strcmp is an obvious change.
> 
> However, this is not complete.  This function is called by:
[...]
> But really, so that this sort of out-of-sync bugs doesn't
> happen, it'd be better if search_symbols_equal were reimplemented
> in terms of compare_search_syms or even be eliminated.

Indeed! I'm turning this fix into a 2-patch series:

  1. Delete search_symbols_equal

     This patch does not fix anything, other than removing the duplication.

     I pondered a bit over that change, thinking maybe we'd want
     to keep it, just because the arguments have the right type
     instead of "void *". In the end, I felt it was sufficiently
     localized that deleting was OK. Putting it back is easy,
     though, so whichever we want it shall be.

  2. The patch I initially proposed.

     This patch implements the strcmp -> FILENAME_CMP fix.

gdb/ChangeLog:

        * symtab.c (search_symbols_equal): Delete.
        (sort_search_symbols_remove_dups): Replace call to
        search_symbols_equal by call to compare_search_syms,
        adjusting as necessary.

gdb/ChangeLog:

        * symtab.c (compare_search_syms): Use FILENAME_CMP instead of
        strcmp to compare two symtab filenames.

Tested on x86_64-linux, no regression. OK to apply?

Thanks,
-- 
Joel

[-- Attachment #2: 0001-Delete-search_symbols_equal-use-compare_search_syms-.patch --]
[-- Type: text/x-diff, Size: 1845 bytes --]

From 56b13bd393e3e1a205888af374a236aae668140a Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Tue, 1 Oct 2013 14:15:46 +0200
Subject: [PATCH 1/2] Delete search_symbols_equal (use compare_search_syms
 instead).

This avoids duplicating the logic comparing two symbol_search
objects (in search_symbols_equal and compare_search_syms).

gdb/ChangeLog:

        * symtab.c (search_symbols_equal): Delete.
        (sort_search_symbols_remove_dups): Replace call to
        search_symbols_equal by call to compare_search_syms,
        adjusting as necessary.
---
 gdb/symtab.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index dbff042..cf03b43 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3347,19 +3347,6 @@ compare_search_syms (const void *sa, const void *sb)
 		 SYMBOL_PRINT_NAME (sym_b->symbol));
 }
 
-/* Helper function for sort_search_symbols_remove_dups.
-   Return TRUE if symbols A, B are equal.  */
-
-static int
-search_symbols_equal (const struct symbol_search *a,
-		      const struct symbol_search *b)
-{
-  return (strcmp (a->symtab->filename, b->symtab->filename) == 0
-	  && a->block == b->block
-	  && strcmp (SYMBOL_PRINT_NAME (a->symbol),
-		     SYMBOL_PRINT_NAME (b->symbol)) == 0);
-}
-
 /* Sort the NFOUND symbols in list FOUND and remove duplicates.
    The duplicates are freed, and the new list is returned in
    *NEW_HEAD, *NEW_TAIL.  */
@@ -3393,7 +3380,7 @@ sort_search_symbols_remove_dups (struct symbol_search *found, int nfound,
   /* Collapse out the dups.  */
   for (i = 1, j = 1; i < nfound; ++i)
     {
-      if (! search_symbols_equal (symbols[j - 1], symbols[i]))
+      if (compare_search_syms (&symbols[j - 1], &symbols[i]) != 0)
 	symbols[j++] = symbols[i];
       else
 	xfree (symbols[i]);
-- 
1.8.1.2


[-- Attachment #3: 0002-Use-FILENAME_CMP-to-compare-filenames-in-compare_sea.patch --]
[-- Type: text/x-diff, Size: 889 bytes --]

From a191522c5b61288616ef91780f4e89edbf4d33fb Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Thu, 12 Sep 2013 14:03:47 -0400
Subject: [PATCH 2/2] Use FILENAME_CMP to compare filenames in
 compare_search_syms.

gdb/ChangeLog:

        * symtab.c (compare_search_syms): Use FILENAME_CMP instead of
        strcmp to compare two symtab filenames.
---
 gdb/symtab.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index cf03b43..d3622e5 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3336,7 +3336,7 @@ compare_search_syms (const void *sa, const void *sb)
   struct symbol_search *sym_b = *(struct symbol_search **) sb;
   int c;
 
-  c = strcmp (sym_a->symtab->filename, sym_b->symtab->filename);
+  c = FILENAME_CMP (sym_a->symtab->filename, sym_b->symtab->filename);
   if (c != 0)
     return c;
 
-- 
1.8.1.2

Re: [RFA] Use FILENAME_CMP to compare filenames in compare_search_syms.

[Pedro Alves]

On 10/01/2013 01:30 PM, Joel Brobecker wrote:
>> I'd say that use of FILENAME_CMP instead of strcmp is an obvious change.
>>
>> However, this is not complete.  This function is called by:
> [...]
>> But really, so that this sort of out-of-sync bugs doesn't
>> happen, it'd be better if search_symbols_equal were reimplemented
>> in terms of compare_search_syms or even be eliminated.
> 
> Indeed! I'm turning this fix into a 2-patch series:
> 
>   1. Delete search_symbols_equal
> 
>      This patch does not fix anything, other than removing the duplication.
> 
>      I pondered a bit over that change, thinking maybe we'd want
>      to keep it, just because the arguments have the right type
>      instead of "void *". In the end, I felt it was sufficiently
>      localized that deleting was OK. Putting it back is easy,
>      though, so whichever we want it shall be.

This is fine with me.

>         * symtab.c (compare_search_syms): Use FILENAME_CMP instead of
>         strcmp to compare two symtab filenames.

This one looks obvious to me.

Thanks,
-- 
Pedro Alves

Re: [RFA] Use FILENAME_CMP to compare filenames in compare_search_syms.

[Joel Brobecker]

> > Indeed! I'm turning this fix into a 2-patch series:
> > 
> >   1. Delete search_symbols_equal
> > 
> >      This patch does not fix anything, other than removing the duplication.
> > 
> >      I pondered a bit over that change, thinking maybe we'd want
> >      to keep it, just because the arguments have the right type
> >      instead of "void *". In the end, I felt it was sufficiently
> >      localized that deleting was OK. Putting it back is easy,
> >      though, so whichever we want it shall be.
> 
> This is fine with me.
> 
> >         * symtab.c (compare_search_syms): Use FILENAME_CMP instead of
> >         strcmp to compare two symtab filenames.
> 
> This one looks obvious to me.

Thanks, Pedro. Both patches are now in.

-- 
Joel