[rfc] Fix Obj-C method calls on 64-bit PowerPC

[rfc] Fix Obj-C method calls on 64-bit PowerPC

[Ulrich Weigand]

Hello,

on ppc64-linux, most Obj-C test cases now fail with a timeout due to
segmentation faults, causing very long testsuite run times.

The fundamental problem is that calling an Obj-C method does not
properly support platforms where a function pointer actually points
to a function descriptor instead of the target address.  This showed
up in a number of places:

- linespec.c:decode_objc did not convert minimal symbol addresses to
  function addresses (in addition, it did not fully initialize the SAL
  it returned in that case, resulting in an uninitialized "section"
  pointer that sometimes caused GDB to crash).

- objc-lang.c:find_methods required minimal symbols to reside in the
  text section (which is not valid if the symbol points to a descriptor
  in the data section) and likewise did not convert the minsym address
  to the function address.

- eval.c:evaluate_subexp_standard did some quite interesting hand-crafted
  "type casts" between *function pointer* and *function* types, which
  fail if function pointers actually refer to descriptors.  This code
  needs to use real function pointers as long as possible (to avoid
  losing descriptor information that is needed later on).

- and finally, ppc-sysv-tdep.c:ppc64_sysv_abi_push_dummy_call is sometimes
  unable to re-construct a function descriptor for a function called via
  function pointer (if the target is in a library without debug info).
  But this is silly, as the function pointer actually contains all the
  descriptor information needed, we just have to look at it ...

With the patch below, the Obj-C test cases now work as well as on
32-bit ppc-linux (and other Linux platforms).  There is still the
one failure in objcdecode.exp (like on other platforms).

Tested on s390(x)-linux and ppc(64)-linux with no regressions.

Does this look OK?

Bye,
Ulrich


ChangeLog:

	* eval.c (evaluate_subexp_standard) [OP_OBJC_MSGCALL]: Support
	platforms that use function descriptors.  Prefer to use function
	pointer types instead of function types.
	* linespec.c (decode_objc): Support function descriptors.  Fully
	initialize SAL result.
	* objc-lang.c (find_methods): Support function descriptors.
	Do not require function symbol to point to text section.

	* ppc-sysv-tdep.c (ppc64_sysv_abi_push_dummy_call): When calling
	via a function pointer, use the descriptor it point to.


Index: gdb/eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.120
diff -c -p -r1.120 eval.c
*** gdb/eval.c	22 Sep 2009 17:39:53 -0000	1.120
--- gdb/eval.c	25 Sep 2009 17:54:25 -0000
*************** evaluate_subexp_standard (struct type *e
*** 1161,1168 ****
  	if (addr)
  	  {
  	    struct symbol *sym = NULL;
- 	    /* Is it a high_level symbol?  */
  
  	    sym = find_pc_function (addr);
  	    if (sym != NULL) 
  	      method = value_of_variable (sym, 0);
--- 1161,1173 ----
  	if (addr)
  	  {
  	    struct symbol *sym = NULL;
  
+ 	    /* The address might point to a function descriptor;
+ 	       resolve it to the actual code address instead.  */
+ 	    addr = gdbarch_convert_from_func_ptr_addr (exp->gdbarch, addr,
+ 						       &current_target);
+ 
+ 	    /* Is it a high_level symbol?  */
  	    sym = find_pc_function (addr);
  	    if (sym != NULL) 
  	      method = value_of_variable (sym, 0);
*************** evaluate_subexp_standard (struct type *e
*** 1216,1226 ****
  	  {
  	    if (TYPE_CODE (value_type (method)) != TYPE_CODE_FUNC)
  	      error (_("method address has symbol information with non-function type; skipping"));
  	    if (struct_return)
! 	      set_value_address (method, value_as_address (msg_send_stret));
  	    else
! 	      set_value_address (method, value_as_address (msg_send));
! 	    called_method = method;
  	  }
  	else
  	  {
--- 1221,1240 ----
  	  {
  	    if (TYPE_CODE (value_type (method)) != TYPE_CODE_FUNC)
  	      error (_("method address has symbol information with non-function type; skipping"));
+ 
+ 	    /* Create a function pointer of the appropriate type, and replace
+ 	       its value with the value of msg_send or msg_send_stret.  We must
+ 	       use a pointer here, as msg_send and msg_send_stret are of pointer
+ 	       type, and the representation may be different on systems that use
+ 	       function descriptors.  */
  	    if (struct_return)
! 	      called_method
! 		= value_from_pointer (lookup_pointer_type (value_type (method)),
! 				      value_as_address (msg_send_stret));
  	    else
! 	      called_method
! 		= value_from_pointer (lookup_pointer_type (value_type (method)),
! 				      value_as_address (msg_send));
  	  }
  	else
  	  {
*************** evaluate_subexp_standard (struct type *e
*** 1275,1281 ****
  	  {
  	    /* Function objc_msg_lookup returns a pointer.  */
  	    deprecated_set_value_type (argvec[0],
! 				       lookup_function_type (lookup_pointer_type (value_type (argvec[0]))));
  	    argvec[0] = call_function_by_hand (argvec[0], nargs + 2, argvec + 1);
  	  }
  
--- 1289,1295 ----
  	  {
  	    /* Function objc_msg_lookup returns a pointer.  */
  	    deprecated_set_value_type (argvec[0],
! 				       lookup_pointer_type (lookup_function_type (value_type (argvec[0]))));
  	    argvec[0] = call_function_by_hand (argvec[0], nargs + 2, argvec + 1);
  	  }
  
Index: gdb/linespec.c
===================================================================
RCS file: /cvs/src/src/gdb/linespec.c,v
retrieving revision 1.90
diff -c -p -r1.90 linespec.c
*** gdb/linespec.c	21 Sep 2009 19:46:43 -0000	1.90
--- gdb/linespec.c	25 Sep 2009 17:54:26 -0000
*************** decode_objc (char **argptr, int funfirst
*** 1172,1182 ****
  	}
        else
  	{
! 	  /* The only match was a non-debuggable symbol.  */
! 	  values.sals[0].symtab = NULL;
! 	  values.sals[0].line = 0;
! 	  values.sals[0].end = 0;
! 	  values.sals[0].pc = SYMBOL_VALUE_ADDRESS (sym_arr[0]);
  	}
        return values;
      }
--- 1172,1190 ----
  	}
        else
  	{
! 	  /* The only match was a non-debuggable symbol, which might point
! 	     to a function descriptor; resolve it to the actual code address
! 	     instead.  */
! 	  struct minimal_symbol *msymbol = (struct minimal_symbol *)sym_arr[0];
! 	  struct objfile *objfile = msymbol_objfile (msymbol);
! 	  struct gdbarch *gdbarch = get_objfile_arch (objfile);
! 	  CORE_ADDR pc = SYMBOL_VALUE_ADDRESS (msymbol);
! 
! 	  pc = gdbarch_convert_from_func_ptr_addr (gdbarch, pc,
! 						   &current_target);
! 
! 	  init_sal (&values.sals[0]);
! 	  values.sals[0].pc = pc;
  	}
        return values;
      }
Index: gdb/objc-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/objc-lang.c,v
retrieving revision 1.83
diff -c -p -r1.83 objc-lang.c
*** gdb/objc-lang.c	2 Jul 2009 17:25:57 -0000	1.83
--- gdb/objc-lang.c	25 Sep 2009 17:54:27 -0000
*************** find_methods (struct symtab *symtab, cha
*** 1173,1188 ****
  
        ALL_OBJFILE_MSYMBOLS (objfile, msymbol)
  	{
  	  QUIT;
  
! 	  if ((MSYMBOL_TYPE (msymbol) != mst_text)
! 	      && (MSYMBOL_TYPE (msymbol) != mst_file_text))
! 	    /* Not a function or method.  */
! 	    continue;
  
  	  if (symtab)
! 	    if ((SYMBOL_VALUE_ADDRESS (msymbol) <  BLOCK_START (block)) ||
! 		(SYMBOL_VALUE_ADDRESS (msymbol) >= BLOCK_END (block)))
  	      /* Not in the specified symtab.  */
  	      continue;
  
--- 1173,1190 ----
  
        ALL_OBJFILE_MSYMBOLS (objfile, msymbol)
  	{
+ 	  struct gdbarch *gdbarch = get_objfile_arch (objfile);
+ 	  CORE_ADDR pc = SYMBOL_VALUE_ADDRESS (msymbol);
+ 
  	  QUIT;
  
! 	  /* The minimal symbol might point to a function descriptor;
! 	     resolve it to the actual code address instead.  */
! 	  pc = gdbarch_convert_from_func_ptr_addr (gdbarch, pc,
! 						   &current_target);
  
  	  if (symtab)
! 	    if (pc < BLOCK_START (block) || pc >= BLOCK_END (block))
  	      /* Not in the specified symtab.  */
  	      continue;
  
*************** find_methods (struct symtab *symtab, cha
*** 1221,1227 ****
  	      ((nselector == NULL) || (strcmp (selector, nselector) != 0)))
  	    continue;
  
! 	  sym = find_pc_function (SYMBOL_VALUE_ADDRESS (msymbol));
  	  if (sym != NULL)
  	    {
  	      const char *newsymname = SYMBOL_NATURAL_NAME (sym);
--- 1223,1229 ----
  	      ((nselector == NULL) || (strcmp (selector, nselector) != 0)))
  	    continue;
  
! 	  sym = find_pc_function (pc);
  	  if (sym != NULL)
  	    {
  	      const char *newsymname = SYMBOL_NATURAL_NAME (sym);
Index: gdb/ppc-sysv-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/ppc-sysv-tdep.c,v
retrieving revision 1.55
diff -c -p -r1.55 ppc-sysv-tdep.c
*** gdb/ppc-sysv-tdep.c	2 Jul 2009 17:25:58 -0000	1.55
--- gdb/ppc-sysv-tdep.c	25 Sep 2009 17:54:27 -0000
*************** ppc64_sysv_abi_push_dummy_call (struct g
*** 1326,1335 ****
    regcache_cooked_write_signed (regcache, tdep->ppc_lr_regnum, bp_addr);
  
    /* Use the func_addr to find the descriptor, and use that to find
!      the TOC.  */
    {
!     CORE_ADDR desc_addr;
!     if (convert_code_addr_to_desc_addr (func_addr, &desc_addr))
        {
  	/* The TOC is the second double word in the descriptor.  */
  	CORE_ADDR toc =
--- 1326,1339 ----
    regcache_cooked_write_signed (regcache, tdep->ppc_lr_regnum, bp_addr);
  
    /* Use the func_addr to find the descriptor, and use that to find
!      the TOC.  If we're calling via a function pointer, the pointer
!      itself identifies the descriptor.  */
    {
!     struct type *ftype = check_typedef (value_type (function));
!     CORE_ADDR desc_addr = value_as_address (function);
! 
!     if (TYPE_CODE (ftype) == TYPE_CODE_PTR
! 	|| convert_code_addr_to_desc_addr (func_addr, &desc_addr))
        {
  	/* The TOC is the second double word in the descriptor.  */
  	CORE_ADDR toc =
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Fix Obj-C method calls on 64-bit PowerPC

[Joel Brobecker]

> 	* eval.c (evaluate_subexp_standard) [OP_OBJC_MSGCALL]: Support
> 	platforms that use function descriptors.  Prefer to use function
> 	pointer types instead of function types.
> 	* linespec.c (decode_objc): Support function descriptors.  Fully
> 	initialize SAL result.
> 	* objc-lang.c (find_methods): Support function descriptors.
> 	Do not require function symbol to point to text section.
> 
> 	* ppc-sysv-tdep.c (ppc64_sysv_abi_push_dummy_call): When calling
> 	via a function pointer, use the descriptor it point to.
                                                      ^^^^^ points ;-)

Seems fine to me. Given that this fixes SEGVs, we should consider it
for inclusion in the 7.0 branch. You might want to ask ratmice@gmail.com
to give it a test before installing on 7.0, but I don't think it would
buy us much given that you already tested on s390. At least the risk
is limited to ObjC.

-- 
Joel

Re: [rfc] Fix Obj-C method calls on 64-bit PowerPC

[Matt Rice]

On Mon, Sep 28, 2009 at 9:32 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>>       * eval.c (evaluate_subexp_standard) [OP_OBJC_MSGCALL]: Support
>>       platforms that use function descriptors.  Prefer to use function
>>       pointer types instead of function types.
>>       * linespec.c (decode_objc): Support function descriptors.  Fully
>>       initialize SAL result.
>>       * objc-lang.c (find_methods): Support function descriptors.
>>       Do not require function symbol to point to text section.
>>
>>       * ppc-sysv-tdep.c (ppc64_sysv_abi_push_dummy_call): When calling
>>       via a function pointer, use the descriptor it point to.
>                                                      ^^^^^ points ;-)
>
> Seems fine to me. Given that this fixes SEGVs, we should consider it
> for inclusion in the 7.0 branch. You might want to ask ratmice@gmail.com
> to give it a test before installing on 7.0, but I don't think it would
> buy us much given that you already tested on s390. At least the risk
> is limited to ObjC.

I ran this and the tests against cvs head on x86-64
Additionally I did some tests for things by hand that aren't yet
covered by the testsuite

all seemed fine, thanks for looking into this.

Re: [rfc] Fix Obj-C method calls on 64-bit PowerPC

[Ulrich Weigand]

Matt Rice wrote:
> On Mon, Sep 28, 2009 at 9:32 AM, Joel Brobecker <brobecker@adacore.com> wro=
> te:
> >> =A0 =A0 =A0 * eval.c (evaluate_subexp_standard) [OP_OBJC_MSGCALL]: Suppo=
> rt
> >> =A0 =A0 =A0 platforms that use function descriptors. =A0Prefer to use fu=
> nction
> >> =A0 =A0 =A0 pointer types instead of function types.
> >> =A0 =A0 =A0 * linespec.c (decode_objc): Support function descriptors. =
> =A0Fully
> >> =A0 =A0 =A0 initialize SAL result.
> >> =A0 =A0 =A0 * objc-lang.c (find_methods): Support function descriptors.
> >> =A0 =A0 =A0 Do not require function symbol to point to text section.
> >>
> >> =A0 =A0 =A0 * ppc-sysv-tdep.c (ppc64_sysv_abi_push_dummy_call): When cal=
> ling
> >> =A0 =A0 =A0 via a function pointer, use the descriptor it point to.
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0^^^^^ points ;-)
> >
> > Seems fine to me. Given that this fixes SEGVs, we should consider it
> > for inclusion in the 7.0 branch. You might want to ask ratmice@gmail.com
> > to give it a test before installing on 7.0, but I don't think it would
> > buy us much given that you already tested on s390. At least the risk
> > is limited to ObjC.
> 
> I ran this and the tests against cvs head on x86-64
> Additionally I did some tests for things by hand that aren't yet
> covered by the testsuite
> 
> all seemed fine, thanks for looking into this.

Thanks for testing!  I've now checked this in to head and branch.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

[ia64] Regression: Re: [rfc] Fix Obj-C method calls on 64-bit PowerPC

[Jan Kratochvil]

Hi Ulrich,

ia64 is now broken, also on the 7.0 branch due to the check-in:
 http://sourceware.org/ml/gdb-cvs/2009-09/msg00210.html

$ echo 'main(){}' | gcc -o 1 -x c -; ./gdb -q -nx -ex 'b main' -ex q ./1
Cannot access memory at address 0x6000000000000c10

#0  ia64_convert_from_func_ptr_addr (gdbarch=0x6000000000167b20, addr=6917529027641085184, targ=0x600000000006fa40) at ia64-tdep.c:3511
#1  0x40000000003ca9d0 in gdbarch_convert_from_func_ptr_addr (gdbarch=0x6000000000167b20, addr=6917529027641085184, targ=0x600000000006fa40) at gdbarch.c:2559
#2  0x4000000000541a50 in find_methods (symtab=0x0, type=0 '\0', class=0x0, category=0x0, selector=0x60000fffffa79dc0 "main", syms=0x0, nsym=0x60000fffffa79e70, ndebug=0x60000fffffa79e6c) at objc-lang.c:1183
#3  0x4000000000543210 in find_imps (symtab=0x0, block=0x0, method=0x60000fffffa7ab89 "main", syms=0x0, nsym=0x60000fffffa79f90, ndebug=0x60000fffffa79f8c) at objc-lang.c:1344
#4  0x4000000000340d20 in decode_objc (argptr=0x60000fffffa7a210, funfirstline=1, file_symtab=0x0, canonical=0x60000fffffa7a2c0, saved_arg=0x60000fffffa7ab89 "main") at linespec.c:1131
#5  0x400000000033ea00 in decode_line_1 (argptr=0x60000fffffa7a210, funfirstline=1, default_symtab=0x0, default_line=0, canonical=0x60000fffffa7a2c0, not_found_ptr=0x60000fffffa7a300) at linespec.c:744
#6  0x4000000000262580 in parse_breakpoint_sals (address=0x60000fffffa7a210, sals=0x60000fffffa7a2a0, addr_string=0x60000fffffa7a2c0, not_found_ptr=0x60000fffffa7a300) at breakpoint.c:6092
#7  0x4000000000262af0 in do_captured_parse_breakpoint (ui=0x600000000016ff40, data=0x60000fffffa7a280) at breakpoint.c:6128
#8  0x40000000003949c0 in catch_exception (uiout=0x600000000016ff40, func=@0x4000000000a03d10: 0x4000000000262a10 <do_captured_parse_breakpoint>, func_args=0x60000fffffa7a280, mask=6) at exceptions.c:462
#9  0x4000000000263600 in break_command_really (gdbarch=0x6000000000167b20, arg=0x60000fffffa7ab89 "main", cond_string=0x0, thread=0, parse_condition_and_thread=1, tempflag=0, hardwareflag=0, traceflag=0, ignore_count=0, pending_break_support=AUTO_BOOLEAN_AUTO, ops=0x0, from_tty=1, enabled=1) at breakpoint.c:6246
#10 0x40000000002645e0 in break_command_1 (arg=0x60000fffffa7ab89 "main", flag=0, from_tty=1) at breakpoint.c:6411
#11 0x4000000000264f60 in break_command (arg=0x60000fffffa7ab89 "main", from_tty=1) at breakpoint.c:6525

Attaching ia64 patch.

Checked that in the ia64 case there is:

#4  0x40000000005419c0 in find_methods (symtab=0x0, type=0 '\0', class=0x0, category=0x0, selector=0x60000fffff829dc0 "main", syms=0x0, nsym=0x60000fffff829e70, ndebug=0x60000fffff829e6c) at objc-lang.c:1183
1183		  pc = gdbarch_convert_from_func_ptr_addr (gdbarch, pc,
(gdb) p *msymbol
$6 = {ginfo = {name = 0x6000000000191360 "__data_start", value = {ivalue = 6917529027641085184, block = 0x6000000000000d00, bytes = 0x6000000000000d00 <Address 0x6000000000000d00 out of bounds>, address = 6917529027641085184, chain = 0x6000000000000d00}, language_specific = {cplus_specific = {demangled_name = 0x0}}, language = language_auto, section = 20, obj_section = 0x6000000000190ec0}, size = 0, filename = 0x60000000001911c0 "../../gcc/config/ia64/crtend.asm", type = mst_data, target_flag_1 = 0, target_flag_2 = 0, hash_next = 0x0, demangled_hash_next = 0x0}
-> value = 0x6000000000000d00
which fails to be read as <0x6000000000000d04..0x6000000000000d08) is just missing:
Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [21] .data             PROGBITS        6000000000000d00 000d00 000004 00  WA  0   0  4
  [22] .ctors            PROGBITS        6000000000000d08 000d08 000010 00  WA  0   0  8

mst_data looks suspicious but on ppc64 case the function descriptor has really
mst_data there:

descriptor for `main':
  [20] .opd              PROGBITS        00000000100108c8 0008c8 0000a8 00  WA  0   0  8
  71: 0000000010010928    44 FUNC    GLOBAL DEFAULT   20 main
$40 = {ginfo = {name = 0x10bb4b50 "main", value = {ivalue = 268503336, block = 0x10010928, bytes = 0x10010928 "", address = 268503336, chain = 0x10010928}, language_specific = {cplus_specific = {demangled_name = 0x0}}, language = language_auto, section = 19, obj_section = 0x10bb46e8}, size = 44, filename = 0x10bb4990 "crtsavres.S", type = mst_data, target_flag_1 = 0, target_flag_2 = 0, hash_next = 0x0, demangled_hash_next = 0x0}
code for `main':
  [10] .text             PROGBITS        0000000010000320 000320 000368 00  AX  0   0 16
00000000100004c4 <.main>:
$8 = {ginfo = {name = 0x10bb4c10 ".main", value = {ivalue = 268436676, block = 0x100004c4, bytes = 0x100004c4 "\220", address = 268436676, chain = 0x100004c4}, language_specific = {cplus_specific = {demangled_name = 0x0}}, language = language_auto, section = 9, obj_section = 0x10bb45f8}, size = 44, filename = 0x10bb4b80 "", type = mst_text, target_flag_1 = 0, target_flag_2 = 0, hash_next = 0x0, demangled_hash_next = 0x0}

On ia64-rhel5.4-linux-gnu with this patch there are still these regressions:
+FAIL: gdb.base/corefile.exp: print func2::coremaker_local
+FAIL: gdb.base/corefile.exp: backtrace in corefile.exp
+FAIL: gdb.base/corefile.exp: up in corefile.exp
+FAIL: gdb.base/corefile.exp: up in corefile.exp (reinit)
+FAIL: gdb.base/gcore.exp: where in corefile (pattern 1)
+FAIL: gdb.base/gcore.exp: corefile restored general registers
+FAIL: gdb.base/gcore.exp: corefile restored all registers
+FAIL: gdb.base/gcore.exp: capture_command_output failed on print array_func::local_array.
+FAIL: gdb.base/gcore.exp: corefile restored stack array
+FAIL: gdb.base/gcore.exp: corefile restored backtrace
+FAIL: gdb.gdb/selftest.exp: unknown source line after step over ttyarg initialization
+FAIL: gdb.gdb/selftest.exp: step into xmalloc call
+FAIL: gdb.threads/gcore-thread.exp: corefile contains at least two threads
+FAIL: gdb.threads/gcore-thread.exp: a corefile thread is executing thread2
+FAIL: gdb.threads/gcore-thread.exp: thread2 is current thread in corefile

So rather submitting first before possible next fixes.


Regards,
Jan


gdb/
2009-09-29  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* ia64-tdep.c (ia64_convert_from_func_ptr_addr): New variable buf.
	Check first the descriptor memory is readable.

--- a/gdb/ia64-tdep.c
+++ b/gdb/ia64-tdep.c
@@ -3510,6 +3510,7 @@ ia64_convert_from_func_ptr_addr (struct gdbarch *gdbarch, CORE_ADDR addr,
 {
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   struct obj_section *s;
+  gdb_byte buf[8];
 
   s = find_pc_section (addr);
 
@@ -3520,10 +3521,12 @@ ia64_convert_from_func_ptr_addr (struct gdbarch *gdbarch, CORE_ADDR addr,
   /* Normally, functions live inside a section that is executable.
      So, if ADDR points to a non-executable section, then treat it
      as a function descriptor and return the target address iff
-     the target address itself points to a section that is executable.  */
-  if (s && (s->the_bfd_section->flags & SEC_CODE) == 0)
+     the target address itself points to a section that is executable.
+     Check first the memory of the whole length of 8 bytes is readable.  */
+  if (s && (s->the_bfd_section->flags & SEC_CODE) == 0
+      && target_read_memory (addr, buf, 8) == 0)
     {
-      CORE_ADDR pc = read_memory_unsigned_integer (addr, 8, byte_order);
+      CORE_ADDR pc = extract_unsigned_integer (buf, 8, byte_order);
       struct obj_section *pc_section = find_pc_section (pc);
 
       if (pc_section && (pc_section->the_bfd_section->flags & SEC_CODE))

Re: [ia64] Regression: Re: [rfc] Fix Obj-C method calls on 64-bit PowerPC

[Ulrich Weigand]

Jan Kratochvil wrote:

> ia64 is now broken, also on the 7.0 branch due to the check-in:
>  http://sourceware.org/ml/gdb-cvs/2009-09/msg00210.html

Huh.  Sorry for the breakage ...

> Checked that in the ia64 case there is:
> 
> #4  0x40000000005419c0 in find_methods (symtab=0x0, type=0 '\0', class=0x0, category=0x0, selector=0x60000fffff829dc0 "main", syms=0x0, nsym=0x60000fffff829e70, ndebug=0x60000fffff829e6c) at objc-lang.c:1183
> 1183		  pc = gdbarch_convert_from_func_ptr_addr (gdbarch, pc,
> (gdb) p *msymbol
> $6 = {ginfo = {name = 0x6000000000191360 "__data_start", value = {ivalue = 6917529027641085184, block = 0x6000000000000d00, bytes = 0x6000000000000d00 <Address 0x6000000000000d00 out of bounds>, address = 6917529027641085184, chain = 0x6000000000000d00}, language_specific = {cplus_specific = {demangled_name = 0x0}}, language = language_auto, section = 20, obj_section = 0x6000000000190ec0}, size = 0, filename = 0x60000000001911c0 "../../gcc/config/ia64/crtend.asm", type = mst_data, target_flag_1 = 0, target_flag_2 = 0, hash_next = 0x0, demangled_hash_next = 0x0}
> -> value = 0x6000000000000d00
> which fails to be read as <0x6000000000000d04..0x6000000000000d08) is just missing:
> Section Headers:
>   [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
>   [21] .data             PROGBITS        6000000000000d00 000d00 000004 00  WA  0   0  4
>   [22] .ctors            PROGBITS        6000000000000d08 000d08 000010 00  WA  0   0  8

I agree that ia64_convert_from_func_ptr_addr should not error out in such cases,
so your patch looks good to me (would have looked good in any case, even without
the Obj-C changes) ...   Are you going to check it in?

> mst_data looks suspicious but on ppc64 case the function descriptor has really
> mst_data there:

OK, that's probably the core change that triggers the problem.  But as you say,
on ppc64 we do actually get mst_data minimal symbols for functions (that point
to the descriptor) ...

> On ia64-rhel5.4-linux-gnu with this patch there are still these regressions:
> +FAIL: gdb.base/corefile.exp: print func2::coremaker_local
> +FAIL: gdb.base/corefile.exp: backtrace in corefile.exp
> +FAIL: gdb.base/corefile.exp: up in corefile.exp
> +FAIL: gdb.base/corefile.exp: up in corefile.exp (reinit)
> +FAIL: gdb.base/gcore.exp: where in corefile (pattern 1)
> +FAIL: gdb.base/gcore.exp: corefile restored general registers
> +FAIL: gdb.base/gcore.exp: corefile restored all registers
> +FAIL: gdb.base/gcore.exp: capture_command_output failed on print array_func::local_array.
> +FAIL: gdb.base/gcore.exp: corefile restored stack array
> +FAIL: gdb.base/gcore.exp: corefile restored backtrace
> +FAIL: gdb.gdb/selftest.exp: unknown source line after step over ttyarg initialization
> +FAIL: gdb.gdb/selftest.exp: step into xmalloc call
> +FAIL: gdb.threads/gcore-thread.exp: corefile contains at least two threads
> +FAIL: gdb.threads/gcore-thread.exp: a corefile thread is executing thread2
> +FAIL: gdb.threads/gcore-thread.exp: thread2 is current thread in corefile

Are these regressions also introduced by my Obj-C patch?  That seems
really odd to me ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [ia64] Regression: Re: [rfc] Fix Obj-C method calls on 64-bit PowerPC

[Joel Brobecker]

> ia64 is now broken, also on the 7.0 branch due to the check-in:
>  http://sourceware.org/ml/gdb-cvs/2009-09/msg00210.html

Darn function descriptors on ia64, and darn objc support that
overmatches. One of the assumptions I was making when considering
this patch for 7.0 was the fact that it would only affect ObjC.
But I should have known better after all the discussions I had
with Matt Rice. My suggestion is to revert it from 7.0, and maybe
consider it for 7.0.1 if we manage to fix all unexpected side-effects
by then.

> 2009-09-29  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* ia64-tdep.c (ia64_convert_from_func_ptr_addr): New variable buf.
> 	Check first the descriptor memory is readable.

This patch is OK.

-- 
Joel

Re: [ia64] Regression: Re: [rfc] Fix Obj-C method calls on 64-bit PowerPC

[Jan Kratochvil]

On Tue, 29 Sep 2009 18:07:24 +0200, Ulrich Weigand wrote:
> I agree that ia64_convert_from_func_ptr_addr should not error out in such cases,
> so your patch looks good to me (would have looked good in any case, even without
> the Obj-C changes) ...   Are you going to check it in?

Considering this as your check-in approval so yes.

I also think this patch is appropriate no matter if it will be required for
the descriptor solution.


> > On ia64-rhel5.4-linux-gnu with this patch there are still these regressions:
> > +FAIL: gdb.base/corefile.exp: print func2::coremaker_local
> > +FAIL: gdb.base/corefile.exp: backtrace in corefile.exp
> > +FAIL: gdb.base/corefile.exp: up in corefile.exp
> > +FAIL: gdb.base/corefile.exp: up in corefile.exp (reinit)
> > +FAIL: gdb.base/gcore.exp: where in corefile (pattern 1)
> > +FAIL: gdb.base/gcore.exp: corefile restored general registers
> > +FAIL: gdb.base/gcore.exp: corefile restored all registers
> > +FAIL: gdb.base/gcore.exp: capture_command_output failed on print array_func::local_array.
> > +FAIL: gdb.base/gcore.exp: corefile restored stack array
> > +FAIL: gdb.base/gcore.exp: corefile restored backtrace
> > +FAIL: gdb.gdb/selftest.exp: unknown source line after step over ttyarg initialization
> > +FAIL: gdb.gdb/selftest.exp: step into xmalloc call
> > +FAIL: gdb.threads/gcore-thread.exp: corefile contains at least two threads
> > +FAIL: gdb.threads/gcore-thread.exp: a corefile thread is executing thread2
> > +FAIL: gdb.threads/gcore-thread.exp: thread2 is current thread in corefile
> 
> Are these regressions also introduced by my Obj-C patch?  That seems
> really odd to me ...

Tested HEAD with our change unpatched -> HEAD with my ia64 change.

Verified gdb.base/corefile.exp is reproducible this way.

Therefore going to check these specific cases more.


Thanks,
Jan

Re: [ia64] Regression: Re: [rfc] Fix Obj-C method calls on 64-bit PowerPC

[Jan Kratochvil]

On Tue, 29 Sep 2009 18:16:08 +0200, Joel Brobecker wrote:
> My suggestion is to revert it from 7.0, and maybe consider it for 7.0.1 if
> we manage to fix all unexpected side-effects by then.
> 
> > 2009-09-29  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* ia64-tdep.c (ia64_convert_from_func_ptr_addr): New variable buf.
> > 	Check first the descriptor memory is readable.
> 
> This patch is OK.

Checked-in for HEAD:
	http://sourceware.org/ml/gdb-cvs/2009-09/msg00223.html

Have not touched gdb_7_0-branch.


Thanks,
Jan

Re: [ia64] Regression: Re: [rfc] Fix Obj-C method calls on 64-bit PowerPC

[Joel Brobecker]

> Checked-in for HEAD:
> 	http://sourceware.org/ml/gdb-cvs/2009-09/msg00223.html
> Have not touched gdb_7_0-branch.

Go ahead with the branch. The patch is good, regardless of whether
Ulrich's patch exposes the issue or not.

-- 
Joel

Re: [ia64] Regression: Re: [rfc] Fix Obj-C method calls on 64-bit PowerPC

[Ulrich Weigand]

Joel Brobecker wrote:
> > ia64 is now broken, also on the 7.0 branch due to the check-in:
> >  http://sourceware.org/ml/gdb-cvs/2009-09/msg00210.html
> 
> Darn function descriptors on ia64, and darn objc support that
> overmatches. One of the assumptions I was making when considering
> this patch for 7.0 was the fact that it would only affect ObjC.
> But I should have known better after all the discussions I had
> with Matt Rice. My suggestion is to revert it from 7.0, and maybe
> consider it for 7.0.1 if we manage to fix all unexpected side-effects
> by then.

Argh.  I (perhaps naively) assumed that the "decode_objc" routine
would only trigger for ObjC source files ...  Of course this turns
out not to be true, and it is in fact invoked for *any* symbol
lookup first, just to find out that the symbol is not ObjC.

In this case, I agree that this change is much too risky for 7.0
at this stage (even assuming Jan would manage to find and fix all
ia64 regressions).  In any case, ObjC has always been broken on
ppc64 so my patch isn't even a regression fix as such (reverting
it will get us back to testsuite runs taking 20 mins longer than
they should, but I guess I can live with that).

I'm going to revert my ObjC patch from the branch as soon as
testing completes.  However, I'll leave it in mainline for now,
to expose it to further testing and see what comes up.

If there is a follow-on release on the 7.0 branch (but I do not
consider the ppc64 ObjC issue by itself sufficient cause to
trigger such a release -- only if we decide to do it anyway),
and the patch has been on mainline for a while with the
resulting issues fixed, we can revisit the decision to put
it on the branch.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

[rfc] Move PC in-range check in objc-lang.c:find_methods (Re: [ia64] Regression)

[Ulrich Weigand]

Jan Kratochvil wrote:

> mst_data looks suspicious but on ppc64 case the function descriptor has really
> mst_data there:

Looking at the location in objc-lang.c:find_methods where the mst_text
check used to be a bit closer, I noticed something odd anyway:

      ALL_OBJFILE_MSYMBOLS (objfile, msymbol)
        {
          struct gdbarch *gdbarch = get_objfile_arch (objfile);
          CORE_ADDR pc = SYMBOL_VALUE_ADDRESS (msymbol);

          QUIT;

          /* The minimal symbol might point to a function descriptor;
             resolve it to the actual code address instead.  */
          pc = gdbarch_convert_from_func_ptr_addr (gdbarch, pc,
                                                   &current_target);

          if (symtab)
            if (pc < BLOCK_START (block) || pc >= BLOCK_END (block))
              /* Not in the specified symtab.  */
              continue;

          symname = SYMBOL_NATURAL_NAME (msymbol);
          if (symname == NULL)
            continue;

          if ((symname[0] != '-' && symname[0] != '+') || (symname[1] != '['))
            /* Not a method name.  */
            continue;

          while ((strlen (symname) + 1) >= tmplen)
            {
              tmplen = (tmplen == 0) ? 1024 : tmplen * 2;
              tmp = xrealloc (tmp, tmplen);
            }
          strcpy (tmp, symname);

          if (parse_method (tmp, &ntype, &nclass, &ncategory, &nselector) == NULL)
            continue;

          objfile_csym++;

For every msymbol in the objfile, the very *first* check is whether
its address is in range of the given symbol table.  Only then come the
checks against the symbol name up to and including the parse_method call.
If those checks pass, the objfile_csym count is increased.

However, that count is supposed to be the number of *all* ObjC symbols
in the objfile, not just those that match the given symtab.  Note the
sanity check at the end:

        /* Count of ObjC methods in this objfile should be constant.  */
        gdb_assert (*objc_csym == objfile_csym);

It would appear that if find_methods is first called with a non-NULL
symtab, objfile_csym is set to the number of ObjC symbols within that
symtab.  Assuming this is non-zero, and find_methods is later called
with a different (or NULL) symtab value, the loop will now possibly
compute a *different* count of ObjC symbols, triggering the assertion.

Therefore, it seems that the range check against the symtab ought
to be performed only *after* the symbol name checks are done and
objfile_csym is incremented ...

As a side effect, this would also reduce the number of calls to
gdbarch_convert_from_func_ptr_addr for symbols that clearly are not
ObjC symbols, reducing the potential of running into problems in
targets that cannot deterministically tell whether an address
points to a function descriptor or not.

This is implemented by the following patch.  Does this look
reasonable?

Tested on ppc(64)-linux with no regressions.

Bye,
Ulrich

ChangeLog:

	* objc-lang.c (find_methods): Move function descriptor conversion
	and PC in-range check until after basic checks for ObjC symbols.


Index: gdb/objc-lang.c
===================================================================
RCS file: /cvs/src/src/gdb/objc-lang.c,v
retrieving revision 1.84
diff -c -p -r1.84 objc-lang.c
*** gdb/objc-lang.c	29 Sep 2009 00:48:31 -0000	1.84
--- gdb/objc-lang.c	29 Sep 2009 17:11:19 -0000
*************** find_methods (struct symtab *symtab, cha
*** 1178,1193 ****
  
  	  QUIT;
  
- 	  /* The minimal symbol might point to a function descriptor;
- 	     resolve it to the actual code address instead.  */
- 	  pc = gdbarch_convert_from_func_ptr_addr (gdbarch, pc,
- 						   &current_target);
- 
- 	  if (symtab)
- 	    if (pc < BLOCK_START (block) || pc >= BLOCK_END (block))
- 	      /* Not in the specified symtab.  */
- 	      continue;
- 
  	  symname = SYMBOL_NATURAL_NAME (msymbol);
  	  if (symname == NULL)
  	    continue;
--- 1178,1183 ----
*************** find_methods (struct symtab *symtab, cha
*** 1208,1213 ****
--- 1198,1213 ----
        
  	  objfile_csym++;
  
+ 	  /* The minimal symbol might point to a function descriptor;
+ 	     resolve it to the actual code address instead.  */
+ 	  pc = gdbarch_convert_from_func_ptr_addr (gdbarch, pc,
+ 						   &current_target);
+ 
+ 	  if (symtab)
+ 	    if (pc < BLOCK_START (block) || pc >= BLOCK_END (block))
+ 	      /* Not in the specified symtab.  */
+ 	      continue;
+ 
  	  if ((type != '\0') && (ntype != type))
  	    continue;
  
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [rfc] Move PC in-range check in objc-lang.c:find_methods (Re: [ia64] Regression)

[Joel Brobecker]

> ChangeLog:
> 
> 	* objc-lang.c (find_methods): Move function descriptor conversion
> 	and PC in-range check until after basic checks for ObjC symbols.

Seems right to me. Note that the counter is really used as a boolean
since: as soon as the count is at least 1, we do a full search of
our objfile...

-- 
Joel

[commit] Avoid Obj-C test timeouts due to symbols not found

[Ulrich Weigand]

I wrote:

> In this case, I agree that this change is much too risky for 7.0
> at this stage (even assuming Jan would manage to find and fix all
> ia64 regressions).  In any case, ObjC has always been broken on
> ppc64 so my patch isn't even a regression fix as such (reverting
> it will get us back to testsuite runs taking 20 mins longer than
> they should, but I guess I can live with that).
> 
> I'm going to revert my ObjC patch from the branch as soon as
> testing completes.  However, I'll leave it in mainline for now,
> to expose it to further testing and see what comes up.

I've reverted the patch on the branch now.

To alleviate the test case timeout problem, I've committed the
following test-suite only patch that converts the timeouts
into simple test case failures.  This makes the testsuite
usable again ...

Tested on ppc(64)-linux.  Committed to mainline and branch.

Bye,
Ulrich

ChangeLog:

	* gdb.objc/basicclass.exp: Disable pending breakpoint query.
	* gdb.objc/nondebug.exp: Likewise.

Index: gdb/testsuite/gdb.objc/basicclass.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.objc/basicclass.exp,v
retrieving revision 1.10
diff -c -p -r1.10 basicclass.exp
*** gdb/testsuite/gdb.objc/basicclass.exp	3 Jan 2009 05:58:06 -0000	1.10
--- gdb/testsuite/gdb.objc/basicclass.exp	29 Sep 2009 18:24:15 -0000
*************** do_objc_tests
*** 102,107 ****
--- 102,112 ----
  #
  # Breakpoint tests
  #
+ 
+ # Disable pending breakpoint query to avoid timeouts
+ # if Obj-C symbols cannot be found
+ gdb_test "set breakpoint pending off" "" "set breakpoint pending"
+ 
  gdb_test "break doIt" \
      "Breakpoint.*at.* file .*$srcfile, line.29.*" \
      "breakpoint method"
Index: gdb/testsuite/gdb.objc/nondebug.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.objc/nondebug.exp,v
retrieving revision 1.6
diff -c -p -r1.6 nondebug.exp
*** gdb/testsuite/gdb.objc/nondebug.exp	3 Jan 2009 05:58:06 -0000	1.6
--- gdb/testsuite/gdb.objc/nondebug.exp	29 Sep 2009 18:24:15 -0000
*************** proc do_objc_tests {} {
*** 56,61 ****
--- 56,65 ----
  
  do_objc_tests
  
+ # Disable pending breakpoint query to avoid timeouts
+ # if Obj-C symbols cannot be found
+ gdb_test "set breakpoint pending off" "" "set breakpoint pending"
+ 
  #
  # Break on multiply defined non-debuggable symbol (PR objc/1236)
  #


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [ia64] Regression: Re: [rfc] Fix Obj-C method calls on 64-bit PowerPC

[Jan Kratochvil]

On Tue, 29 Sep 2009 18:40:19 +0200, Joel Brobecker wrote:
> > Checked-in for HEAD:
> > 	http://sourceware.org/ml/gdb-cvs/2009-09/msg00223.html
> > Have not touched gdb_7_0-branch.
> 
> Go ahead with the branch. The patch is good, regardless of whether
> Ulrich's patch exposes the issue or not.

Checked-in on branch:
	http://sourceware.org/ml/gdb-cvs/2009-09/msg00227.html


Thanks,
Jan

Re: [ia64] Regression: Re: [rfc] Fix Obj-C method calls on 64-bit PowerPC

[Jan Kratochvil]

On Tue, 29 Sep 2009 17:43:28 +0200, Jan Kratochvil wrote:
> On ia64-rhel5.4-linux-gnu with this patch there are still these regressions:
> +FAIL: gdb.base/corefile.exp: print func2::coremaker_local
> +FAIL: gdb.base/corefile.exp: backtrace in corefile.exp
> +FAIL: gdb.base/corefile.exp: up in corefile.exp
> +FAIL: gdb.base/corefile.exp: up in corefile.exp (reinit)
> +FAIL: gdb.base/gcore.exp: where in corefile (pattern 1)
> +FAIL: gdb.base/gcore.exp: corefile restored general registers
> +FAIL: gdb.base/gcore.exp: corefile restored all registers
> +FAIL: gdb.base/gcore.exp: capture_command_output failed on print array_func::local_array.
> +FAIL: gdb.base/gcore.exp: corefile restored stack array
> +FAIL: gdb.base/gcore.exp: corefile restored backtrace
> +FAIL: gdb.gdb/selftest.exp: unknown source line after step over ttyarg initialization
> +FAIL: gdb.gdb/selftest.exp: step into xmalloc call
> +FAIL: gdb.threads/gcore-thread.exp: corefile contains at least two threads
> +FAIL: gdb.threads/gcore-thread.exp: a corefile thread is executing thread2
> +FAIL: gdb.threads/gcore-thread.exp: thread2 is current thread in corefile

FYI these regressions are no longer reproducible, it may have been a random
result.


Thanks,
Jan