| Summary: | objc_msg_sendv is broken for targets which pass argument via registers | ||
|---|---|---|---|
| Product: | gcc | Reporter: | Robert W. Fuller <hydrologiccycle> |
| Component: | libobjc | Assignee: | Not yet assigned to anyone <unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | danglin, gcc-bugs, ghazi, hjl.tools, iains, janis, js-gcc, nicola, pinskia, ro |
| Priority: | P3 | Keywords: | wrong-code |
| Version: | 4.3.1 | ||
| Target Milestone: | --- | ||
| Host: | Target: | arguments in registers | |
| Build: | Known to work: | ||
| Known to fail: | Last reconfirmed: | 2009-03-31 19:12:03 | |
This is most likely a problem with __builtin_apply :). 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 *** Bug 38689 has been marked as a duplicate of this bug. *** Confirmed, this is really a __builtin_apply bug. Libobjc either needs to move to libffi or __builtin_apply needs to be fixed up. (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? 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.
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. (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 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 (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. (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. 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.
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 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 *** Bug 39817 has been marked as a duplicate of this bug. *** 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 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 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. (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 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 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
Nope, it's still using __builtin_apply. 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 #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 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>
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 (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 #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 (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. |
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)