[RFC] Fix wrong use of SYMBOL_VALUE in find_pc_sect_line.

[Thiago Jung Bauermann <bauerman@br.ibm.com>]

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

Hi,

I was working with an older version of GDB and found out a difference in
behaviour between a 64-bit GDB debugging a 32-bit inferior and a 32-bit
GDB doing the same thing (on a PowerPC machine).

Some investigation revealed that the culprit in that case is
find_pc_sect_line (comments removed for brevity):

	mfunsym = lookup_minimal_symbol_text (SYMBOL_LINKAGE_NAME (msymbol),
					      NULL);
	if (mfunsym == NULL)
	  /* warning ("In stub for %s; unable to find real function/line info",
SYMBOL_LINKAGE_NAME (msymbol)) */ ;
	/* fall through */
	else if (SYMBOL_VALUE (mfunsym) == SYMBOL_VALUE (msymbol))
	  /* warning ("In stub for %s; unable to find real function/line info",
SYMBOL_LINKAGE_NAME (msymbol)) */ ;
	/* fall through */
	else
	  return find_pc_line (SYMBOL_VALUE (mfunsym), 0);

which uses SYMBOL_VALUE instead of SYMBOL_VALUE_ADDRESS when comparing
what is expected to be function addresses. This is not a problem in
64-bit GDB because both msymbol->ginfo.value.ivalue and
msymbol->ginfo.value.address are 64-bit wide, so using either of them in
a comparison is fine. In 32-bit GDB, however,
msymbol->ginfo.value.ivalue is 32-bit wide while
msymbol->ginfo.value.address is 64-bit wide. This yelds the following
problem:

(gdb) p mfunsym->ginfo.value
$47 = {ivalue = 0, block = 0x0, bytes = 0x0, address = 268301724, chain
= 0x0}
(gdb) p msymbol->ginfo.value
$48 = {ivalue = 0, block = 0x0, bytes = 0x0, address = 268441920, chain
= 0x0}

Even though the address element is different, the comparison will say
they are equal because ivalue is equal in both. In this case GDB will
not go to the else block and call find_pc_line with mfunsym's value, but
erroneously continue in the find_pc_sect_line function instead.

The attached patch fixes the problem. In the older GDB version I was
working with, the patch fixed gdb.base/gdb1555.exp and
gdb.base/so-impl-ld.exp, which test single-stepping into a shared
library function. A patch from Ulrich Weigand
(http://sourceware.org/ml/gdb-patches/2007-10/msg00879.html) also fixed
these testcases, though, so in current GDB no testcase is fixed by the
patch. Still, I think the current code is conceptually wrong and should
be fixed. What do you think?

Another way to work around this would be to change ginfo.ivalue from
long to LONGEST, since other places could be using SYMBOL_VALUE
erroneously and there are many uses (105) of the macro to check. But I
don't have a specific testcase where this is causing a problem, so such
big change may not have enough justification.
-- 
[]'s
Thiago Jung Bauermann
Software Engineer
IBM Linux Technology Center

[-- Attachment #2: find_pc_sect_line-32bits-fix.diff --]
[-- Type: text/x-patch, Size: 1143 bytes --]

2008-01-03  Thiago Jung Bauermann  <bauerman@br.ibm.com>

	* symtab.c (find_pc_sect_line): Use SYMBOL_VALUE_ADDRESS instead
	of SYMBOL_VALUE when working with function symbols.

Index: src-git/gdb/symtab.c
===================================================================
--- src-git.orig/gdb/symtab.c	2008-01-03 15:13:59.000000000 -0200
+++ src-git/gdb/symtab.c	2008-01-03 15:14:42.000000000 -0200
@@ -2109,13 +2109,13 @@ find_pc_sect_line (CORE_ADDR pc, struct 
 	   * So I commented out the warning. RT */
 	  /* warning ("In stub for %s; unable to find real function/line info", SYMBOL_LINKAGE_NAME (msymbol)) */ ;
 	/* fall through */
-	else if (SYMBOL_VALUE (mfunsym) == SYMBOL_VALUE (msymbol))
+	else if (SYMBOL_VALUE_ADDRESS (mfunsym) == SYMBOL_VALUE_ADDRESS (msymbol))
 	  /* Avoid infinite recursion */
 	  /* See above comment about why warning is commented out */
 	  /* warning ("In stub for %s; unable to find real function/line info", SYMBOL_LINKAGE_NAME (msymbol)) */ ;
 	/* fall through */
 	else
-	  return find_pc_line (SYMBOL_VALUE (mfunsym), 0);
+	  return find_pc_line (SYMBOL_VALUE_ADDRESS (mfunsym), 0);
       }