[commit/Ada] psymbol search failure due to comparison function discrepancy

[commit/Ada] psymbol search failure due to comparison function discrepancy

[Joel Brobecker]

Hello,

Upon trying to print the value of a variant record, a user noticed
the following problem:

        (gdb) print rt
        warning: Unknown upper bound, using 1.
        warning: Unknown upper bound, using 1.
        $1 = (a => ((a1 => (4), a2 => (4)), (a1 => (8), a2 => (8))))
The expected output is:

        (gdb) print rt
        $1 = (a => ((a1 => (4, 4), a2 => (8, 8)), (a1 => (4, 4),
              a2 => (8, 8))))

(the number of elements in the "a1", "a2" arrays should be 2,
not 1)

The problems comes from the fact that components "a1" and "a2" are
defined as arrays whose upper bound is dynamic.  To determine the value
of that upper bound, GDB relies on the GNAT encoding and searches
for the parallel ___U variable.  Unfortunately, the search fails
while doing a binary search inside the partial symtab of the unit
where the array and its bound (and therefore the parallel ___U variable)
are defined.

It fails because partial symbols are sorted using strcmp_iw_ordered,
while Ada symbol lookups are performed using a different comparison
function (ada-lang.c:compare_names). The two functions are supposed
to be compatible, but a change performed in April 2011 modified
strcmp_iw_ordered, introducing case-sensitivity issues. As a result,
the two functions would now disagree when passed the following
two arguments:

  string1="common__inner_arr___SIZE_A_UNIT"
  string2="common__inner_arr__T4s___U"

The difference starts at "_SIZE_A_UNIT" vs "T4s___U". So, it's mostly
a matter of comparing '_' with 'T'.

On the one hand, strcmp_iw_ordered would return -1, while compare_names
returned 11. The change that made all the difference is that
strcmp_iw_ordered now performs a case-insensitive comparison,
and only resorts to case-sentitive comparison if the first comparison
finds an equality. This changes everything, because while 'T' (84)
and 't' (116) are on opposite sides of '_' (95).

This patch aims at restoring the compatibility between the two
functions, by adding case-sensitivity handling in the Ada comparison
function.

gdb/ChangeLog:

        * ada-lang.c (compare_names_with_case): Renamed from
        compare_names, adding a new parameter "casing" and its handling.
        New function documentation.
        (compare_names): New function, implemented using
        compare_names_with_case.

Tested on x86_64-linux. No testcase unfortunately, as the problem
is sensitive to symbol names and ordering, and the only reproducer
we have has non-contributable code in it.

I will commit momentarily.

Cheeers,
-- 
Joel

---
 gdb/ada-lang.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 7 deletions(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 1e18129..9ff3ab9 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -4982,23 +4982,37 @@ aux_add_nonlocal_symbols (struct block *block, struct symbol *sym, void *data0)
   return 0;
 }
 
-/* Compare STRING1 to STRING2, with results as for strcmp.
-   Compatible with strcmp_iw in that strcmp_iw (STRING1, STRING2) <= 0
-   implies compare_names (STRING1, STRING2) (they may differ as to
-   what symbols compare equal).  */
+/* Implements compare_names, but only applying the comparision using
+   the given CASING.  */
 
 static int
-compare_names (const char *string1, const char *string2)
+compare_names_with_case (const char *string1, const char *string2,
+			 enum case_sensitivity casing)
 {
   while (*string1 != '\0' && *string2 != '\0')
     {
+      char c1, c2;
+
       if (isspace (*string1) || isspace (*string2))
 	return strcmp_iw_ordered (string1, string2);
-      if (*string1 != *string2)
+
+      if (casing == case_sensitive_off)
+	{
+	  c1 = tolower (*string1);
+	  c2 = tolower (*string2);
+	}
+      else
+	{
+	  c1 = *string1;
+	  c2 = *string2;
+	}
+      if (c1 != c2)
 	break;
+
       string1 += 1;
       string2 += 1;
     }
+
   switch (*string1)
     {
     case '(':
@@ -5016,10 +5030,43 @@ compare_names (const char *string1, const char *string2)
       if (*string2 == '(')
 	return strcmp_iw_ordered (string1, string2);
       else
-	return *string1 - *string2;
+	{
+	  if (casing == case_sensitive_off)
+	    return tolower (*string1) - tolower (*string2);
+	  else
+	    return *string1 - *string2;
+	}
     }
 }
 
+/* Compare STRING1 to STRING2, with results as for strcmp.
+   Compatible with strcmp_iw_ordered in that...
+
+       strcmp_iw_ordered (STRING1, STRING2) <= 0
+
+   ... implies...
+
+       compare_names (STRING1, STRING2) <= 0
+
+   (they may differ as to what symbols compare equal).  */
+
+static int
+compare_names (const char *string1, const char *string2)
+{
+  int result;
+
+  /* Similar to what strcmp_iw_ordered does, we need to perform
+     a case-insensitive comparison first, and only resort to
+     a second, case-sensitive, comparison if the first one was
+     not sufficient to differentiate the two strings.  */
+
+  result = compare_names_with_case (string1, string2, case_sensitive_off);
+  if (result == 0)
+    result = compare_names_with_case (string1, string2, case_sensitive_on);
+
+  return result;
+}
+
 /* Add to OBSTACKP all non-local symbols whose name and domain match
    NAME and DOMAIN respectively.  The search is performed on GLOBAL_BLOCK
    symbols if GLOBAL is non-zero, or on STATIC_BLOCK symbols otherwise.  */
-- 
1.8.1.2

Re: [commit/Ada] psymbol search failure due to comparison function discrepancy

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> This patch aims at restoring the compatibility between the two
Joel> functions, by adding case-sensitivity handling in the Ada comparison
Joel> function.

Joel> Tested on x86_64-linux. No testcase unfortunately, as the problem
Joel> is sensitive to symbol names and ordering, and the only reproducer
Joel> we have has non-contributable code in it.

Is it possible to construct a synthetic test case using the DWARF assembler?

I ask because I think that, while the patch is fine, having two
functions which must remain in sync semantically, where one such
function is only used by Ada and has no test case for some branches, is
definitely going to cause future regressions.

Tom

Re: [commit/Ada] psymbol search failure due to comparison function discrepancy

[Joel Brobecker]

> Is it possible to construct a synthetic test case using the DWARF assembler?
> 
> I ask because I think that, while the patch is fine, having two
> functions which must remain in sync semantically, where one such
> function is only used by Ada and has no test case for some branches, is
> definitely going to cause future regressions.

I agree. Let me see what I can do...

In the long term, I'd really like the psymtab to have the language,
so we can simply use the same routine for both sorting and searching.
ISTR other cases where I really wished I had access to the language
from the psymtab.

-- 
Joel

Re: [commit/Ada] psymbol search failure due to comparison function discrepancy

[Joel Brobecker]

> In the long term, I'd really like the psymtab to have the language,
> so we can simply use the same routine for both sorting and searching.
> ISTR other cases where I really wished I had access to the language
> from the psymtab.

Oops - I also meant to say that this would allow us to look at the option
of avoiding the case-sentivity dual pass, which could help in terms of
performance.

-- 
Joel

Re: [commit/Ada] psymbol search failure due to comparison function discrepancy

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> In the long term, I'd really like the psymtab to have the language,
Joel> so we can simply use the same routine for both sorting and searching.
Joel> ISTR other cases where I really wished I had access to the language
Joel> from the psymtab.

Joel> Oops - I also meant to say that this would allow us to look at the option
Joel> of avoiding the case-sentivity dual pass, which could help in terms of
Joel> performance.

Offhand it seems reasonable to me.

Tom