RESOLVED FIXED 14521
JavaScriptCore fails to build on Linux/PPC gcc 4.1.2
https://bugs.webkit.org/show_bug.cgi?id=14521
Summary JavaScriptCore fails to build on Linux/PPC gcc 4.1.2
Xan Lopez
Reported 2007-07-04 09:28:21 PDT
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'
Attachments
Possible patch (411 bytes, patch)
2007-11-11 09:11 PST, Mike Hommey
no flags
0001-Use-less-strict-memory-operand-constraint-on-inline.patch (1.59 KB, patch)
2007-12-18 09:45 PST, Xan Lopez
ggaren: review+
Patryk Zawadzki
Comment 1 2007-10-23 07:53:49 PDT
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
David Kilzer (:ddkilzer)
Comment 2 2007-10-23 08:36:30 PDT
*** Bug 14974 has been marked as a duplicate of this bug. ***
David Kilzer (:ddkilzer)
Comment 3 2007-10-23 08:38:15 PDT
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
Mike Hommey
Comment 4 2007-11-11 09:11:10 PST
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...
David Kilzer (:ddkilzer)
Comment 5 2007-11-11 12:43:14 PST
(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.
Xan Lopez
Comment 6 2007-11-11 12:46:57 PST
Works ok in Linux/PPC too (but no clue about its correctness either).
David Kilzer (:ddkilzer)
Comment 7 2007-11-11 12:57:45 PST
(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.
Mike Hommey
Comment 8 2007-11-11 13:32:32 PST
FWIW, the PPC inline code has been added, according to the changelog, by mjs in revision 10701.
David Kilzer (:ddkilzer)
Comment 9 2007-11-11 14:09:13 PST
(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?
Maciej Stachowiak
Comment 10 2007-11-11 19:04:55 PST
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
Mark Rowe (bdash)
Comment 11 2007-11-11 22:03:31 PST
Landed in r27708.
Mark Rowe (bdash)
Comment 12 2007-11-11 22:20:56 PST
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)
David Kilzer (:ddkilzer)
Comment 13 2007-11-11 22:27:42 PST
(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?
David Kilzer (:ddkilzer)
Comment 15 2007-11-11 22:37:35 PST
(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.
Mark Rowe (bdash)
Comment 16 2007-11-12 02:31:10 PST
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.
Mike Hommey
Comment 17 2007-11-12 02:44:11 PST
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.
David Kilzer (:ddkilzer)
Comment 18 2007-11-12 05:55:07 PST
(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.
Darin Adler
Comment 19 2007-11-12 12:10:11 PST
Comment on attachment 17187 [details] Possible patch Clearing review flag.
David Kilzer (:ddkilzer)
Comment 20 2007-11-12 17:50:50 PST
(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.
Mark Rowe (bdash)
Comment 21 2007-12-04 11:33:37 PST
*** Bug 16293 has been marked as a duplicate of this bug. ***
David Kilzer (:ddkilzer)
Comment 22 2007-12-04 14:26:03 PST
(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
Xan Lopez
Comment 23 2007-12-18 09:24:49 PST
(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?
David Kilzer (:ddkilzer)
Comment 24 2007-12-18 09:28:30 PST
(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!
Xan Lopez
Comment 25 2007-12-18 09:45:31 PST
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(-)
Geoffrey Garen
Comment 26 2007-12-18 10:26:15 PST
Comment on attachment 17975 [details] 0001-Use-less-strict-memory-operand-constraint-on-inline.patch r=me, based on above comments
David Kilzer (:ddkilzer)
Comment 27 2007-12-18 18:59:25 PST
$ svn commit JavaScriptCore Sending JavaScriptCore/ChangeLog Sending JavaScriptCore/wtf/TCSpinLock.h Transmitting file data .. Committed revision 28847.
Note You need to log in before you can comment on or make changes to this bug.