Bug 36610 - objc_msg_sendv is broken for targets which pass argument via registers
Summary: objc_msg_sendv is broken for targets which pass argument via registers
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libobjc (show other bugs)
Version: 4.3.1
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
: 38689 39817 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-06-24 01:48 UTC by Robert W. Fuller
Modified: 2018-04-06 00:06 UTC (History)
10 users (show)

See Also:
Host:
Target: arguments in registers
Build:
Known to work:
Known to fail:
Last reconfirmed: 2009-03-31 19:12:03


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Robert W. Fuller 2008-06-24 01:48:00 UTC
Note that this is a 64-bit problem.  i386 works fine.  Code for reproducing is included, as well as all system information.


correct output of program on 32-bit:

forwarding for selector display from 134536384 to 134536136
worked, self == receiver
foo is 1234567890, and should be 1234567890


incorrect output of program on 64-bit:

forwarding for selector display from 6330720 to 6330224
broken, self != receiver
foo is 6330224, and should be 1234567890


test program:

// Objective-C x86_64 bug:  self is wrong on forward;
// broken on gcc-4.1.2, 4.3.0, and 4.3.1

#include <stdio.h>
#include <stdlib.h>
#include <objc/Object.h>
#include <objc/objc-api.h>

id forwarder, receiver;

@interface Forwarder:Object
{
    id receiver;
}

-initWithReceiver:theReceiver;

@end

@interface Receiver:Object
{
    int foo;
}

-display;
-initWithFoo:(int)theFoo;

@end

@implementation Receiver

-initWithFoo:(int)theFoo
{
    foo = theFoo;
    return self;
}

-display
{
    if (self == receiver) {
        printf("worked, self == receiver\n");
    } else {
        printf("broken, self != receiver\n");
    }
    printf("foo is %d, and should be %d\n", foo, 1234567890);
    return self;
}

@end

@implementation Forwarder

-initWithReceiver:theReceiver
{
    [super init];
    receiver = theReceiver;
    return self;
}

-(retval_t) forward:(SEL)theSel:(arglist_t)theArgFrame
{
    if (receiver) {

        printf("forwarding for selector %s from %ld to %ld\n",
            sel_get_name(theSel), self, receiver);

        //return objc_msg_sendv(itsNextLink, theSel, theArgFrame);
        return [receiver performv:theSel:theArgFrame];

    } else {
        return [self doesNotRecognize:theSel];
    }
}

@end

int main()
{
    receiver = [[Receiver alloc] initWithFoo:1234567890];
    forwarder = [[Forwarder alloc] initWithReceiver:receiver];

    [forwarder display];

    exit(EXIT_SUCCESS);
    return 0;
}


I compiled gcc from the 4.3.1 tarball on ftp.gnu.org in order to avoid any distro patches.  Here's the output of gcc -v:

Using built-in specs.
Target: x86_64-pc-linux-gnu
Configured with: ../gcc-4.3.1/configure --prefix=/usr/local --host=x86_64-pc-linux-gnu --build=x86_64-pc-linux-gnu --disable-altivec --enable-nls -
-without-included-gettext --with-system-zlib --disable-checking --disable-werror --enable-secureplt --disable-multilib --enable-libmudflap --disabl
e-libssp --disable-libgcj --enable-languages=c,objc --enable-shared --enable-threads=posix --enable-__cxa_atexit --enable-clocale=gnu             
Thread model: posix
gcc version 4.3.1 (GCC)
Comment 1 Andrew Pinski 2008-06-24 01:52:14 UTC
This is most likely a problem with __builtin_apply :).
Comment 2 Andrew Pinski 2008-12-29 02:18:07 UTC
Subject: Bug 36610

Author: pinskia
Date: Mon Dec 29 02:16:45 2008
New Revision: 142945

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=142945
Log:
2008-12-28  Andrew Pinski  <pinskia@gmail.com>

        PR libobjc/36610
        * objc/execute/forward-1.m: New test.


Added:
    trunk/gcc/testsuite/objc/execute/forward-1.m
Modified:
    trunk/gcc/testsuite/ChangeLog

Comment 3 Andrew Pinski 2009-01-01 16:21:32 UTC
*** Bug 38689 has been marked as a duplicate of this bug. ***
Comment 4 Andrew Pinski 2009-01-01 16:23:10 UTC
Confirmed, this is really a __builtin_apply bug.  Libobjc either needs to move to libffi or __builtin_apply needs to be fixed up.
Comment 5 H.J. Lu 2009-01-01 16:26:00 UTC
(In reply to comment #4)
> Confirmed, this is really a __builtin_apply bug.  Libobjc either needs to move
> to libffi or __builtin_apply needs to be fixed up.
> 

Is there a __builtin_apply testcase in C?
Comment 6 Michael Matz 2009-02-12 11:32:00 UTC
I don't think it's __builtin_apply which is wrong.  It's rather how it is
used in libobjc.  In particular register parameters are handled wrongly.
libobjc objc_msg_sendv() simply tries to overwrite the first two argument
slots returned by __builtin_apply_args (called in a different routine) with
those it really wants in there.  It uses method_get_{first,next}_argument
for that which tries to use the argument pointer in that memory block.

But memory pointed to by that arg pointer only contains the args passed on 
stack.  Those passed in registers lie somewhere else (in the block returned
by builtin_apply_args, but behind the arg pointer), in register order (not
in argument order!).

So, overwriting the argument slots doesn't actually overwrite the data which
later is used in __builtin_apply --> boom.

Accordingly changing the summary.
Comment 7 Kaveh Ghazi 2009-03-27 16:51:47 UTC
Andrew - You added the testcase for this PR back in December to see which lp64 targets failed:
http://gcc.gnu.org/ml/gcc-patches/2008-12/msg01199.html

It's still failing several months later.
x86_64: http://gcc.gnu.org/ml/gcc-patches/2008-12/msg01199.html
ppc64: http://gcc.gnu.org/ml/gcc-testresults/2009-03/msg02732.html
ia64: http://gcc.gnu.org/ml/gcc-testresults/2009-03/msg02716.html
s390: http://gcc.gnu.org/ml/gcc-testresults/2009-03/msg02491.html
sh4: http://gcc.gnu.org/ml/gcc-testresults/2009-03/msg02437.html

etc.

I think there's plenty of data now to see where it fails.  You should XFAIL it until a fix is installed to avoid noise in the testsuite results.

Thanks.
Comment 8 Kaveh Ghazi 2009-03-27 16:55:16 UTC
(In reply to comment #7)
> It's still failing several months later.
> x86_64: http://gcc.gnu.org/ml/gcc-patches/2008-12/msg01199.html

The x86_64 link should be: http://gcc.gnu.org/ml/gcc-testresults/2009-03/msg02744.html
Comment 9 dave 2009-03-27 17:38:03 UTC
Subject: Re:  objc_msg_sendv is broken for targets which pass argument via registers

> It's still failing several months later.
> x86_64: http://gcc.gnu.org/ml/gcc-patches/2008-12/msg01199.html
> ppc64: http://gcc.gnu.org/ml/gcc-testresults/2009-03/msg02732.html
> ia64: http://gcc.gnu.org/ml/gcc-testresults/2009-03/msg02716.html
> s390: http://gcc.gnu.org/ml/gcc-testresults/2009-03/msg02491.html
> sh4: http://gcc.gnu.org/ml/gcc-testresults/2009-03/msg02437.html

Also, hppa.

> I think there's plenty of data now to see where it fails.  You should XFAIL it
> until a fix is installed to avoid noise in the testsuite results.

I tried but .x files don't seem to work in this directory.

Dave
Comment 10 Kaveh Ghazi 2009-03-31 19:12:03 UTC
(In reply to comment #9)
> > I think there's plenty of data now to see where it fails.  You should XFAIL it
> > until a fix is installed to avoid noise in the testsuite results.
> I tried but .x files don't seem to work in this directory.
> Dave

I can't get dg- style to work either.  What I'd like to do is something like this:

/* { dg-xfail-run-if "PR36610" { lp64 } "*" "" } */

Janis, can you please help?  Thanks.
Comment 11 Andrew Pinski 2009-03-31 19:34:20 UTC
(In reply to comment #10)
> I can't get dg- style to work either.  What I'd like to do is something like
> this:
> 
> /* { dg-xfail-run-if "PR36610" { lp64 } "*" "" } */
> 
> Janis, can you please help?  Thanks.

Except that is not a fully correct xfail as it is more than just lp64 targets, it is about targets which pass via registers.
Comment 12 Janis Johnson 2009-03-31 20:28:14 UTC
The objc/execute directory can process .x files like the one added here, which I've tested on powerpc64-linux with -m32/-m64:

Index: gcc/testsuite/objc/execute/forward-1.x
===================================================================
--- gcc/testsuite/objc/execute/forward-1.x      (revision 0)
+++ gcc/testsuite/objc/execute/forward-1.x      (revision 0)
@@ -0,0 +1,8 @@
+load_lib target-supports.exp
+
+if { ([istarget x86_64-*-linux*] && [is_effective_target_lp64])
+     || [istarget powerpc*-*-linux*] } {
+    set torture_execute_xfail "*-*-*"
+}
+
+return 0

As Andrew said, the list of affected targets is longer than this.  I don't think it's worth adding an effective-target keyword, people can just add new targets to this list.
Comment 13 Janis Johnson 2009-04-09 16:58:49 UTC
Subject: Bug 36610

Author: janis
Date: Thu Apr  9 16:58:34 2009
New Revision: 145849

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=145849
Log:
	PR libobjc/36610
	* objc/execute/forward-1.x: New.

Added:
    trunk/gcc/testsuite/objc/execute/forward-1.x
Modified:
    trunk/gcc/testsuite/ChangeLog

Comment 14 Janis Johnson 2009-04-09 17:01:19 UTC
Subject: Bug 36610

Author: janis
Date: Thu Apr  9 17:00:57 2009
New Revision: 145850

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=145850
Log:
	PR libobjc/36610
	* objc/execute/forward-1.x: New.

Added:
    branches/gcc-4_4-branch/gcc/testsuite/objc/execute/forward-1.x
Modified:
    branches/gcc-4_4-branch/gcc/testsuite/ChangeLog

Comment 15 Andrew Pinski 2009-04-19 15:52:19 UTC
*** Bug 39817 has been marked as a duplicate of this bug. ***
Comment 16 Rainer Orth 2010-02-24 11:56:31 UTC
Subject: Bug 36610

Author: ro
Date: Wed Feb 24 11:56:10 2010
New Revision: 157035

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=157035
Log:
	PR libobjc/36610
	* objc/execute/forward-1.x: XFAIL on alpha*-dec-osf*, 64-bit
	i?86-*-solaris2*, mips-sgi-irix*, sparc*-sun-solaris2* with
	-fgnu-runtime.
	Sort entries.

Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/objc/execute/forward-1.x

Comment 17 Rainer Orth 2010-06-02 17:17:23 UTC
Subject: Bug 36610

Author: ro
Date: Wed Jun  2 17:16:55 2010
New Revision: 160172

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=160172
Log:
	Backport from mainline:
	2010-02-24  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	PR libobjc/36610
	* objc/execute/forward-1.x: XFAIL on alpha*-dec-osf*, 64-bit
	i?86-*-solaris2*, mips-sgi-irix*, sparc*-sun-solaris2* with
	-fgnu-runtime.
	Sort entries.

Modified:
    branches/gcc-4_4-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_4-branch/gcc/testsuite/objc/execute/forward-1.x

Comment 18 Iain Sandoe 2010-11-07 14:50:00 UTC
The test has been removed for GNU runtime for the present, since it seems that fixing this is probably outside the scope of 4.6.

The forward-1.m test has been moved to objc.dg/torture (so that is also exercises with lto) and confined to m32 NeXT runtime.
Comment 19 Andrew Pinski 2010-11-07 19:01:53 UTC
(In reply to comment #18)
> The forward-1.m test has been moved to objc.dg/torture (so that is also
> exercises with lto) and confined to m32 NeXT runtime.

I think this is the wrong approach.  Please just xfail it for the GNU runtime.  This testcase works on some targets (x86 32bits).

-- Pinski
Comment 20 Iain Sandoe 2010-11-07 19:55:00 UTC
Author: iains
Date: Sun Nov  7 19:54:51 2010
New Revision: 166421

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=166421
Log:

gcc/testsuite:

	PR libobjc/36610
	* objc.dg/torture/forward-1.m: Re-enable for gnu-runtime, XFAIL the run for
	all but m32 x86.


Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/objc.dg/torture/forward-1.m
Comment 21 ro@CeBiTec.Uni-Bielefeld.DE 2011-06-14 13:53:56 UTC
objc.dg/torture/forward-1.m now seems to XPASS (almost?) everywhere with
-fgnu-runtime:

        alpha-dec-osf5.1b
        i386-pc-solaris2.1[01] -m64
        mips-sgi-irix6.5
        sparc-sun-solaris2*

Could it be that this PR has now been fixed by the libobjc API rework?
If so, the xfail should be removed.

	Rainer
Comment 22 js-gcc 2011-06-14 14:02:05 UTC
Nope, it's still using __builtin_apply.
Comment 23 Nicola Pero 2011-06-14 18:26:40 UTC
Yes, in a strict sense this bug could be closed ... because objc_msg_sendv() no 
longer exists in the GNU Objective-C runtime.  So if the bug is about it not 
working on some platforms, it can certainly be closed. ;-)

Forwarding in libobjc still uses __builtin_apply(), so the problem, in a wider
sense, still exists.  But that forwarding is not used by any user of libobjc as 
far as I know (they all have their own libffi-based implementation of forwarding
that are used through the forwarding hooks that libobjc has) and I'm planning on 
entirely removing it for 4.7.0 - it's unused (the forwarding hooks override any
implementation in libobjc), and it often doesn't work.

I also have some other plans for a new forwarding infrastructure, that may or may
not happen for 4.7.0.

Summarizing, I would close the bug, or leave it open but just to remind me/us to:

 * remove the existing forwarding code from libobjc;

 * remove the forward-1.m testcase;

 * add new testcases for the libobjc forwarding hooks.  As these are the official
ways to use forwarding (and the only ones available), we should have testcases.

Let me know if you have any comments.

Thanks
Comment 24 ro@CeBiTec.Uni-Bielefeld.DE 2011-06-17 13:23:39 UTC
> --- Comment #23 from Nicola Pero <nicola at gcc dot gnu.org> 2011-06-14 18:26:40 UTC ---
[...]
> Summarizing, I would close the bug, or leave it open but just to remind me/us
> to:
>
>  * remove the existing forwarding code from libobjc;
>
>  * remove the forward-1.m testcase;

Since the test now seems to pass everywhere, we could just remove the
xfail instead.

>  * add new testcases for the libobjc forwarding hooks.  As these are the
> official
> ways to use forwarding (and the only ones available), we should have testcases.

Agreed, but that's orthogonal to the issue at hand.

Whatever is done, we should soon remove the annoying XFAILs of
objc.dg/torture/forward-1.m.

Thanks.
        Rainer
Comment 25 Dominique d'Humieres 2011-06-17 13:29:33 UTC
I have the following patch in my working tree without regression

--- /opt/gcc/_clean/gcc/testsuite/objc.dg/torture/forward-1.m	2011-06-07 16:48:47.000000000 +0200
+++ /opt/gcc/work/gcc/testsuite/objc.dg/torture/forward-1.m	2011-06-07 17:10:00.000000000 +0200
@@ -1,7 +1,5 @@
 /* { dg-do run } */
 /* See if -forward:: is able to work. */
-/* { dg-xfail-run-if "PR36610" { ! { { i?86-*-* x86_64-*-* } && ilp32 } } { "-fgnu-runtime" } { "" } } */
-/* { dg-skip-if "Needs OBJC2 Implementation" { *-*-darwin* && { lp64 } } { "-fnext-runtime" } { "" } } */
 
 #include <stdio.h>
 #include <stdlib.h>
Comment 26 Rainer Orth 2011-06-30 10:02:48 UTC
Author: ro
Date: Thu Jun 30 10:02:45 2011
New Revision: 175689

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=175689
Log:
2011-06-28  Dominique d'Humieres  <dominiq@lps.ens.fr>
	    Iain Sandoe  <iains@gcc.gnu.org>

	PR libobjc/36610
	* objc.dg/torture/forward-1.m: Remove dg-xfail-run-if.
	Only skip on 64-bit *-*-darwin8* && !objc2.

Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/objc.dg/torture/forward-1.m
Comment 27 Eric Gallager 2018-04-05 01:49:17 UTC
(In reply to Rainer Orth from comment #26)
> Author: ro
> Date: Thu Jun 30 10:02:45 2011
> New Revision: 175689
> 
> URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=175689
> Log:
> 2011-06-28  Dominique d'Humieres  <dominiq@lps.ens.fr>
> 	    Iain Sandoe  <iains@gcc.gnu.org>
> 
> 	PR libobjc/36610
> 	* objc.dg/torture/forward-1.m: Remove dg-xfail-run-if.
> 	Only skip on 64-bit *-*-darwin8* && !objc2.
> 
> Modified:
>     trunk/gcc/testsuite/ChangeLog
>     trunk/gcc/testsuite/objc.dg/torture/forward-1.m

Did this fix it?
Comment 28 ro@CeBiTec.Uni-Bielefeld.DE 2018-04-05 12:56:28 UTC
> --- Comment #27 from Eric Gallager <egallager at gcc dot gnu.org> ---
[...]
> Did this fix it?

It seems so, both according to my own testing and gcc-testresults
postings.

	Rainer
Comment 29 Eric Gallager 2018-04-06 00:06:36 UTC
(In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #28)
> > --- Comment #27 from Eric Gallager <egallager at gcc dot gnu.org> ---
> [...]
> > Did this fix it?
> 
> It seems so, both according to my own testing and gcc-testresults
> postings.
> 
> 	Rainer

Cool, closing accordingly then.