Ubuntu Fesity on PowerPC, gcc 4.1.2, WebKit from svn trunk as of 04/07/2007 19:29 EEST :) Output:../../../../JavaScriptCore/wtf/TCSpinLock.h: In function ‘void* TCMalloc_SystemAlloc(size_t, size_t)’: ../../../../JavaScriptCore/wtf/TCSpinLock.h:98: error: ‘asm’ operand requires impossible reload make[1]: *** [TCSystemAlloc.o] Error 1 make[1]: Leaving directory `/home/xan/svn/WebKit/WebKitBuild/Release/JavaScriptCore/kjs'
Same here with gcc-4.2.2 ppc-pld-linux-g++ -c -pipe -D_REENTRANT -D_REENTRANT -I/usr/include -O2 -fno-strict-aliasing -fwrapv -fsigned-char -fno-strict-aliasing -gdwarf-2 -g2 -Wall -W -DBUILDING_GTK__ -I/usr/share/qt4/mkspecs/linux-g++ -I../../../../JavaScriptCore/kjs -I../../../../JavaScriptCore -I../../../../JavaScriptCore/kjs -I../../../../JavaScriptCore/bindings -I../../../../JavaScriptCore/bindings/c -I../../../../JavaScriptCore/wtf -Itmp -I../../../../JavaScriptCore -I../../../../JavaScriptCore/kjs -I../../../../JavaScriptCore/bindings -I../../../../JavaScriptCore/bindings/c -I../../../../JavaScriptCore/wtf -I../../../../JavaScriptCore/pcre -Itmp -I../../../../JavaScriptCore/kjs -I. -o TCSystemAlloc.o ../../../../JavaScriptCore/wtf/TCSystemAlloc.cpp ../../../../JavaScriptCore/wtf/TCSpinLock.h: In function 'void* TCMalloc_SystemAlloc(size_t, size_t)': ../../../../JavaScriptCore/wtf/TCSpinLock.h:98: error: 'asm' operand requires impossible reload make[1]: *** [TCSystemAlloc.o] Error 1 make[1]: Leaving directory `/home/users/builder/rpm/BUILD/WebKit-r26865/WebKitBuild/Release/JavaScriptCore/kjs' make: *** [sub-JavaScriptCore-kjs-testkjs-pro-make_default-ordered] Error 2
*** Bug 14974 has been marked as a duplicate of this bug. ***
Details about how to work around this issue (using -O0 to compile this one file or switching to generic pthreads) is detailed here: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=438415 Bug 14974 Comment #2
Created attachment 17187 [details] Possible patch This patch solves the build problem on a simplified testcase, and produces the same assembly output in -O0 and -Os. I haven't checked if it builds with the real code, and can't verify it doesn't break at runtime, so if someone could check that out...
(In reply to comment #4) > This patch solves the build problem on a simplified testcase, and produces the > same assembly output in -O0 and -Os. I haven't checked if it builds with the > real code, and can't verify it doesn't break at runtime, so if someone could > check that out... Applying this patch and recompiling a local debug build of r27683 seems to work on my PowerBook G4. JavaScriptCore tests and layout tests all pass. I don't know enough PPC assembly to give this an r+, though.
Works ok in Linux/PPC too (but no clue about its correctness either).
(In reply to comment #5) > Applying this patch and recompiling a local debug build of r27683 seems to work > on my PowerBook G4. JavaScriptCore tests and layout tests all pass. I don't > know enough PPC assembly to give this an r+, though. The gcc inline assembly constraints are documented here: http://www.ibiblio.org/gferg/ldp/GCC-Inline-Assembly-HOWTO.html#ss6.1 Here is the difference between "=m" and "=o" (the "=" is a separate modifier): "m" : A memory operand is allowed, with any kind of address that the machine supports in general. "o" : A memory operand is allowed, but only if the address is offsettable. ie, adding a small offset to the address gives a valid address.
FWIW, the PPC inline code has been added, according to the changelog, by mjs in revision 10701.
(In reply to comment #7) > Here is the difference between "=m" and "=o" (the "=" is a separate modifier): > > "m" : A memory operand is allowed, with any kind of address that the machine > supports in general. > > "o" : A memory operand is allowed, but only if the address is offsettable. ie, > adding a small offset to the address gives a valid address. Maciej, would changing "=o" to "=m" cause a significant performance issue? I'm sure there's a locality-of-reference issue here (where "=o" should perform better because it's going to be "closer"), but I'm not sure how much the affect would be. Is this something SunSpider would pick up?
Comment on attachment 17187 [details] Possible patch There's two possible kinds of problems that can be caused by getting the assembly operand constraints wrong: 1) Inferior codegen, due to overconstraining the operands, and forcing gcc to do needless moves. 2) Incorrect codegen, de to using an operand that is disallowed. I think #2 will make the assembly stage fail, so if PPC compiles with this patch it should be safe. For #1, we should be ok as well, because m is a looser constraint than o, so you'd never have a needless copy to satisfy m when o is satisfied. r=me
Landed in r27708.
Rolled out in r27709 as it broke the Mac PowerPC build: {standard input}:47313:Parameter syntax error (parameter 2) {standard input}:54379:Parameter syntax error (parameter 2)
(In reply to comment #12) > Rolled out in r27709 as it broke the Mac PowerPC build: > {standard input}:47313:Parameter syntax error (parameter 2) > {standard input}:54379:Parameter syntax error (parameter 2) I only compiled a debug build with this change. I presume it failed on a release build? Does the buildbot system have an up-to-date version of Xcode installed?
(In reply to comment #7) > http://www.ibiblio.org/gferg/ldp/GCC-Inline-Assembly-HOWTO.html#ss6.1 Apple documentation: <http://developer.apple.com/documentation/DeveloperTools/gcc-4.0.1/gcc/Simple-Constraints.html#Simple-Constraints>
(In reply to comment #13) > (In reply to comment #12) > > Rolled out in r27709 as it broke the Mac PowerPC build: > > {standard input}:47313:Parameter syntax error (parameter 2) > > {standard input}:54379:Parameter syntax error (parameter 2) > > I only compiled a debug build with this change. I presume it failed on a > release build? Does the buildbot system have an up-to-date version of Xcode > installed? I also get an error building a release build with this change, while the debug build worked fine. I have Xcode 2.4.1 installed. Compiler bug? $ gcc --version powerpc-apple-darwin8-gcc-4.0.1 (GCC) 4.0.1 (Apple Computer, Inc. build 5367) Copyright (C) 2005 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Dave, if you have a PowerPC machine it may be worth creating a reduced test case and filing a bug report against the Apple compiler (assuming it works correctly with the same optimisations on for the standard GCC compiler?). We may need to #ifdef this to work around the issues.
FWIW, I have a simplified testcase for the original code here: http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=5;filename=TCSystemAlloc.cpp;att=1;bug=438415 So you should only need to replace =o with =m.
(In reply to comment #16) > Dave, if you have a PowerPC machine it may be worth creating a reduced test > case and filing a bug report against the Apple compiler (assuming it works > correctly with the same optimisations on for the standard GCC compiler?). We > may need to #ifdef this to work around the issues. Will do. (In reply to comment #17) > FWIW, I have a simplified testcase for the original code here: > http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=5;filename=TCSystemAlloc.cpp;att=1;bug=438415 > > So you should only need to replace =o with =m. Thanks, Mike! The OS X gcc compiler fails to compile AllInOneFile.cpp for Release builds, so I may have to start from there for that reduction. It's troubling that =o is broken for non-Apple gcc while =m is broken for Apple's gcc.
Comment on attachment 17187 [details] Possible patch Clearing review flag.
(In reply to comment #18) > (In reply to comment #16) > > Dave, if you have a PowerPC machine it may be worth creating a reduced test > > case and filing a bug report against the Apple compiler (assuming it works > > correctly with the same optimisations on for the standard GCC compiler?). We > > may need to #ifdef this to work around the issues. > > Will do. Filed <rdar://problem/5596043> for the gcc issue. This does NOT cover this WebKit bug, so please don't add the "InRadar" keyword for this radar.
*** Bug 16293 has been marked as a duplicate of this bug. ***
(In reply to comment #20) > Filed <rdar://problem/5596043> for the gcc issue. This does NOT cover this > WebKit bug, so please don't add the "InRadar" keyword for this radar. I think we should use #ifdefs to use "=o" on OS-X-compiled WebKit and "=m" for Linux-compiled WebKit (both using gcc). Perhaps something like: #if PLATFORM(DARWIN) : "=o" (private_lockword_) #else : "=m" (private_lockword_) #endif
(In reply to comment #22) > Perhaps something like: > > #if PLATFORM(DARWIN) > : "=o" (private_lockword_) > #else > : "=m" (private_lockword_) > #endif > This seems to work fine and sounds like a sensible compromise until the bug in the Apple compiler is fixed. Should I attach a proper patch with it?
(In reply to comment #23) > This seems to work fine and sounds like a sensible compromise until the bug in > the Apple compiler is fixed. Should I attach a proper patch with it? Yes, please do. Thanks!
Created attachment 17975 [details] 0001-Use-less-strict-memory-operand-constraint-on-inline.patch Subject: [PATCH] Use less strict memory operand constraint on inline ASM for TCSpinLock.h. --- JavaScriptCore/ChangeLog | 15 +++++++++++++++ JavaScriptCore/wtf/TCSpinLock.h | 6 +++++- 2 files changed, 20 insertions(+), 1 deletions(-)
Comment on attachment 17975 [details] 0001-Use-less-strict-memory-operand-constraint-on-inline.patch r=me, based on above comments
$ svn commit JavaScriptCore Sending JavaScriptCore/ChangeLog Sending JavaScriptCore/wtf/TCSpinLock.h Transmitting file data .. Committed revision 28847.