Re: [PATCH] Fix some 64-bit Objective-C bugs

Re: [PATCH] Fix some 64-bit Objective-C bugs

[Adam Fedor]

On Thursday, December 4, 2003, at 01:10 PM, Ulrich Weigand wrote:

>
>
> ChangeLog:
>
>         * eval.c (evaluate_subexp_standard, case OP_OBJC_MSGCALL): Use
>         CORE_ADDR as type for selectors.  Correct types for GNU run 
> time
>         message lookup function to use double indirection.
>         * objc-lang.c (lookup_child_selector): Use CORE_ADDR as return 
> type.
>         * objc-lang.h (lookup_child_selector): Adapt prototype.
>

Looks good to me. It also happens to fix a problem I was just getting 
around to looking at. I'll apply it in a few days if there are no other 
comments. Thanks for taking the time to look at this.

> testsuite/ChangeLog:
>
>         * gdb.objc/basicclass.exp: Adapt to fixed return type of 
> printHi.
>
>

Not sure if I have authority to approve this, unless it's under the 
obvious rule...

Re: [PATCH] Fix some 64-bit Objective-C bugs

[Daniel Jacobowitz]

On Fri, Dec 05, 2003 at 10:45:52AM -0700, Adam Fedor wrote:
> 
> On Thursday, December 4, 2003, at 01:10 PM, Ulrich Weigand wrote:
> 
> >
> >
> >ChangeLog:
> >
> >        * eval.c (evaluate_subexp_standard, case OP_OBJC_MSGCALL): Use
> >        CORE_ADDR as type for selectors.  Correct types for GNU run 
> >time
> >        message lookup function to use double indirection.
> >        * objc-lang.c (lookup_child_selector): Use CORE_ADDR as return 
> >type.
> >        * objc-lang.h (lookup_child_selector): Adapt prototype.
> >
> 
> Looks good to me. It also happens to fix a problem I was just getting 
> around to looking at. I'll apply it in a few days if there are no other 
> comments. Thanks for taking the time to look at this.

Hi Adam,

Just reminding you that you never checked in this patch.

> >testsuite/ChangeLog:
> >
> >        * gdb.objc/basicclass.exp: Adapt to fixed return type of 
> >printHi.
> >
> >
> 
> Not sure if I have authority to approve this, unless it's under the 
> obvious rule...

It's obvious.  Also, I think you should be listed as the Objective-C
testsuite maintainer in addition to the Objective-C language
maintainer - does anyone disagree?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [PATCH] Fix some 64-bit Objective-C bugs

[Adam Fedor]

On Monday, January 19, 2004, at 09:32 AM, Daniel Jacobowitz wrote:

> On Fri, Dec 05, 2003 at 10:45:52AM -0700, Adam Fedor wrote:
>>
>> On Thursday, December 4, 2003, at 01:10 PM, Ulrich Weigand wrote:
>>
>>>
>>>
>>> ChangeLog:
>>>
>>>         * eval.c (evaluate_subexp_standard, case OP_OBJC_MSGCALL): 
>>> Use
>>>         CORE_ADDR as type for selectors.  Correct types for GNU run
>>> time
>>>         message lookup function to use double indirection.
>>>         * objc-lang.c (lookup_child_selector): Use CORE_ADDR as 
>>> return
>>> type.
>>>         * objc-lang.h (lookup_child_selector): Adapt prototype.
>>>
>>
>> Looks good to me. It also happens to fix a problem I was just getting
>> around to looking at. I'll apply it in a few days if there are no 
>> other
>> comments. Thanks for taking the time to look at this.
>
> Hi Adam,
>
> Just reminding you that you never checked in this patch.
>
>

Andrew told me to wait on this as Ulrich needed to send in an 
assignment.  I don't think that has been done yet (at least it hasn't 
been processed).

Re: [PATCH] Fix some 64-bit Objective-C bugs

[Andrew Cagney]

> Andrew told me to wait on this as Ulrich needed to send in an assignment.  I don't think that has been done yet (at least it hasn't been processed).

Yes, better safe than sorry.  Ulrich, can IBM do the paperwork for all 
the stuff that's approved (this would leave just frame cleanup).

Andrew

Re: [PATCH] Fix some 64-bit Objective-C bugs

[Daniel Jacobowitz]

On Mon, Jan 19, 2004 at 01:19:04PM -0500, Andrew Cagney wrote:
> 
> >Andrew told me to wait on this as Ulrich needed to send in an assignment.  
> >I don't think that has been done yet (at least it hasn't been processed).
> 
> Yes, better safe than sorry.  Ulrich, can IBM do the paperwork for all 
> the stuff that's approved (this would leave just frame cleanup).

OK, thanks for the update!  Filing this back in pending.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [PATCH] Fix some 64-bit Objective-C bugs

[Ulrich Weigand]

Andrew Cagney wrote:

>Things are a lot more flexible than GCC.  In the past we've ended up
>with s390 specific patches being committed to the branch.

That's good to know ...

>Also I've
>seen your assignment lawyers in action - they move much faster than you
>might think.

I've started the process now; we'll see.

>Main thing is to try and get the changes you depend on sorted out before
>the branch.

The patches that are pending for assignment don't in fact depend on
common code changes at all.  The dependencies are only related to
DWARF-2 frames, which are currently still being worked on.


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand
  Linux for S/390 Design & Development
  IBM Deutschland Entwicklung GmbH, Schoenaicher Str. 220, 71032 Boeblingen
  Phone: +49-7031/16-3727   ---   Email: Ulrich.Weigand@de.ibm.com

Re: [PATCH] Fix some 64-bit Objective-C bugs

[Andrew Cagney]

> Andrew Cagney wrote:
> 
> 
>>Yes, better safe than sorry.  Ulrich, can IBM do the paperwork for all
>>the stuff that's approved (this would leave just frame cleanup).
> 
> 
> OK, I've respun the s390 backend patches to adapt them to the latest
> changes and also to fix everything reported by ARI (-Wari only, not -Wall).
> I'll post the latest version shortly.

Thanks.

> I'll then immediately start the legal process for
>   - the main backend patches (part 1 .. 4)
>   - the bi-arch patch
>   - the ObjC patch
> 
> (The Java patch was incorrect, and the DWARF-2 patch still awaits some
> common code changes.)
> 
> I hope we can get this through in time for gdb 6.1 ...
> 
> What's the policy for the branch?  In case the paperwork is still not
> ready at the time the branch is created, can we still get the patches
> in afterwards (after all, the contents are already approved ...)?

Things are a lot more flexible than GCC.  In the past we've ended up 
with s390 specific patches being committed to the branch.  Also I've 
seen your assignment lawyers in action - they move much faster than you 
might think.

Main thing is to try and get the changes you depend on sorted out before 
the branch.

Andrew

Re: [PATCH] Fix some 64-bit Objective-C bugs

[Ulrich Weigand]

Andrew Cagney wrote:

>Yes, better safe than sorry.  Ulrich, can IBM do the paperwork for all
>the stuff that's approved (this would leave just frame cleanup).

OK, I've respun the s390 backend patches to adapt them to the latest
changes and also to fix everything reported by ARI (-Wari only, not -Wall).
I'll post the latest version shortly.

I'll then immediately start the legal process for
  - the main backend patches (part 1 .. 4)
  - the bi-arch patch
  - the ObjC patch

(The Java patch was incorrect, and the DWARF-2 patch still awaits some
common code changes.)

I hope we can get this through in time for gdb 6.1 ...

What's the policy for the branch?  In case the paperwork is still not
ready at the time the branch is created, can we still get the patches
in afterwards (after all, the contents are already approved ...)?


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand
  Linux for S/390 Design & Development
  IBM Deutschland Entwicklung GmbH, Schoenaicher Str. 220, 71032 Boeblingen
  Phone: +49-7031/16-3727   ---   Email: Ulrich.Weigand@de.ibm.com

[PATCH] Fix some 64-bit Objective-C bugs

[Ulrich Weigand]

Hello,

when analysing test suite failures I've noticed a couple of objc
FAILs on s390x that are not backend-related, but in fact caused
but 64-bit unclean code in gdb common objc code.  They are 
typically caused by converting a target pointer to 'int'.

In particular objc selectors are target pointers, and should
thus be held in variables of type CORE_ADDR, not 'int', on the
host.  This patch fixes a couple of occurrances of this problem.

Also, the type information for method pointers was incorrect
when using the GNU runtime, because this involves an additional
indirection step.  Due to this, 'call_function_by_hand' would
assume a return type of 'int' in some cases instead of a correct
pointer return type -- this would again incorrectly truncate
the return value.

The patch fixes this problem as well; it also adapts one test
case which actually tested for the incorrect 'int' return value
instead of the correct pointer type.

Tested on s390-ibm-linux and s390x-ibm-linux with no new regressions.
Fixes 4 test case failures on s390x.

Bye,
Ulrich


ChangeLog:

	* eval.c (evaluate_subexp_standard, case OP_OBJC_MSGCALL): Use 
	CORE_ADDR as type for selectors.  Correct types for GNU run time
	message lookup function to use double indirection.
	* objc-lang.c (lookup_child_selector): Use CORE_ADDR as return type.
	* objc-lang.h (lookup_child_selector): Adapt prototype.

testsuite/ChangeLog:

	* gdb.objc/basicclass.exp: Adapt to fixed return type of printHi.


diff -c -p -r gdb-head/gdb/eval.c gdb-head-new/gdb/eval.c
*** gdb-head/gdb/eval.c	Wed Nov 19 22:37:09 2003
--- gdb-head-new/gdb/eval.c	Thu Nov 20 00:07:21 2003
*************** evaluate_subexp_standard (struct type *e
*** 697,706 ****
      case OP_OBJC_MSGCALL:
        {				/* Objective C message (method) call.  */
  
! 	static unsigned long responds_selector = 0;
! 	static unsigned long method_selector = 0;
  
! 	unsigned long selector = 0;
  
  	int using_gcc = 0;
  	int struct_return = 0;
--- 697,706 ----
      case OP_OBJC_MSGCALL:
        {				/* Objective C message (method) call.  */
  
! 	static CORE_ADDR responds_selector = 0;
! 	static CORE_ADDR method_selector = 0;
  
! 	CORE_ADDR selector = 0;
  
  	int using_gcc = 0;
  	int struct_return = 0;
*************** evaluate_subexp_standard (struct type *e
*** 750,757 ****
--- 750,768 ----
  	   only).  */
  	if (gnu_runtime)
  	  {
+ 	    struct type *type;
+ 	    type = lookup_pointer_type (builtin_type_void);
+ 	    type = lookup_function_type (type);
+ 	    type = lookup_pointer_type (type);
+ 	    type = lookup_function_type (type);
+ 	    type = lookup_pointer_type (type);
+ 
  	    msg_send = find_function_in_inferior ("objc_msg_lookup");
  	    msg_send_stret = find_function_in_inferior ("objc_msg_lookup");
+ 
+ 	    msg_send = value_from_pointer (type, value_as_address (msg_send));
+ 	    msg_send_stret = value_from_pointer (type, 
+ 					value_as_address (msg_send_stret));
  	  }
  	else
  	  {
*************** evaluate_subexp_standard (struct type *e
*** 936,949 ****
  
  	if (gnu_runtime && (method != NULL))
  	  {
- 	    ret = call_function_by_hand (argvec[0], nargs + 2, argvec + 1);
  	    /* Function objc_msg_lookup returns a pointer.  */
! 	    argvec[0] = ret;
! 	    ret = call_function_by_hand (argvec[0], nargs + 2, argvec + 1);
  	  }
- 	else
- 	  ret = call_function_by_hand (argvec[0], nargs + 2, argvec + 1);
  
  	return ret;
        }
        break;
--- 947,959 ----
  
  	if (gnu_runtime && (method != NULL))
  	  {
  	    /* Function objc_msg_lookup returns a pointer.  */
! 	    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);
  	  }
  
+ 	ret = call_function_by_hand (argvec[0], nargs + 2, argvec + 1);
  	return ret;
        }
        break;
diff -c -p -r gdb-head/gdb/objc-lang.c gdb-head-new/gdb/objc-lang.c
*** gdb-head/gdb/objc-lang.c	Wed Nov 19 22:37:09 2003
--- gdb-head-new/gdb/objc-lang.c	Thu Nov 20 00:07:21 2003
*************** lookup_objc_class (char *classname)
*** 133,139 ****
  							   1, &classval));
  }
  
! int
  lookup_child_selector (char *selname)
  {
    struct value * function, *selstring;
--- 133,139 ----
  							   1, &classval));
  }
  
! CORE_ADDR
  lookup_child_selector (char *selname)
  {
    struct value * function, *selstring;
*************** end_msglist(void)
*** 753,759 ****
    int val = msglist_len;
    struct selname *sel = selname_chain;
    char *p = msglist_sel;
!   int selid;
  
    selname_chain = sel->next;
    msglist_len = sel->msglist_len;
--- 753,759 ----
    int val = msglist_len;
    struct selname *sel = selname_chain;
    char *p = msglist_sel;
!   CORE_ADDR selid;
  
    selname_chain = sel->next;
    msglist_len = sel->msglist_len;
diff -c -p -r gdb-head/gdb/objc-lang.h gdb-head-new/gdb/objc-lang.h
*** gdb-head/gdb/objc-lang.h	Wed Nov 19 22:37:09 2003
--- gdb-head-new/gdb/objc-lang.h	Thu Nov 20 00:07:21 2003
*************** extern int c_value_print (struct value *
*** 39,45 ****
  			  int, enum val_prettyprint);
  
  extern CORE_ADDR lookup_objc_class     (char *classname);
! extern int       lookup_child_selector (char *methodname);
  
  extern char *objc_demangle (const char *mangled, int options);
  
--- 39,45 ----
  			  int, enum val_prettyprint);
  
  extern CORE_ADDR lookup_objc_class     (char *classname);
! extern CORE_ADDR lookup_child_selector (char *methodname);
  
  extern char *objc_demangle (const char *mangled, int options);
  
diff -c -p -r gdb-head/gdb/testsuite/gdb.objc/basicclass.exp gdb-head-new/gdb/testsuite/gdb.objc/basicclass.exp
*** gdb-head/gdb/testsuite/gdb.objc/basicclass.exp	Wed Nov 19 22:37:09 2003
--- gdb-head-new/gdb/testsuite/gdb.objc/basicclass.exp	Thu Nov 20 00:07:21 2003
*************** gdb_test continue \
*** 178,184 ****
  # Test calling Objective-C methods
  #
  gdb_test "print \[self printHi\]" \
!     "Hi.*\\$\[0-9\] = -?\[0-9\]+" \
      "Call an Objective-C method with no arguments"
  
  gdb_test "print \[self printNumber: 42\]" \
--- 178,184 ----
  # Test calling Objective-C methods
  #
  gdb_test "print \[self printHi\]" \
!     "Hi.*\\$\[0-9\] = \\(.*objc_object \\*\\) 0x\[0-9a-f\]+" \
      "Call an Objective-C method with no arguments"
  
  gdb_test "print \[self printNumber: 42\]" \
-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de