14521 2007-07-04 09:28:21 -0700 JavaScriptCore fails to build on Linux/PPC gcc 4.1.2 2007-12-18 18:59:25 -0800 1 1 1 Unclassified WebKit JavaScriptCore 523.x (Safari 3) Other Linux RESOLVED FIXED P2 Normal --- 1 xan.lopez webkit-unassigned ddkilzer mh+webkit mjs mrowe patrys oldest_to_newest 5603 0 xan.lopez 2007-07-04 09:28:21 -0700 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' 59251 1 patrys 2007-10-23 07:53:49 -0700 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 59253 2 ddkilzer 2007-10-23 08:36:30 -0700 *** Bug 14974 has been marked as a duplicate of this bug. *** 59254 3 ddkilzer 2007-10-23 08:38:15 -0700 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 60908 4 17187 mh+webkit 2007-11-11 09:11:10 -0800 Created attachment 17187 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... 60919 5 ddkilzer 2007-11-11 12:43:14 -0800 (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. 60920 6 xan.lopez 2007-11-11 12:46:57 -0800 Works ok in Linux/PPC too (but no clue about its correctness either). 60921 7 ddkilzer 2007-11-11 12:57:45 -0800 (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. 60923 8 mh+webkit 2007-11-11 13:32:32 -0800 FWIW, the PPC inline code has been added, according to the changelog, by mjs in revision 10701. 60925 9 ddkilzer 2007-11-11 14:09:13 -0800 (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? 60945 10 17187 mjs 2007-11-11 19:04:55 -0800 Comment on attachment 17187 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 60955 11 mrowe 2007-11-11 22:03:31 -0800 Landed in r27708. 60956 12 mrowe 2007-11-11 22:20:56 -0800 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) 60958 13 ddkilzer 2007-11-11 22:27:42 -0800 (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? 60959 14 ap 2007-11-11 22:35:46 -0800 (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> 60960 15 ddkilzer 2007-11-11 22:37:35 -0800 (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. 60972 16 mrowe 2007-11-12 02:31:10 -0800 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. 60974 17 mh+webkit 2007-11-12 02:44:11 -0800 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. 60984 18 ddkilzer 2007-11-12 05:55:07 -0800 (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. 61006 19 17187 darin 2007-11-12 12:10:11 -0800 Comment on attachment 17187 Possible patch Clearing review flag. 61037 20 ddkilzer 2007-11-12 17:50:50 -0800 (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. 63262 21 mrowe 2007-12-04 11:33:37 -0800 *** Bug 16293 has been marked as a duplicate of this bug. *** 63283 22 ddkilzer 2007-12-04 14:26:03 -0800 (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 64633 23 xan.lopez 2007-12-18 09:24:49 -0800 (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? 64638 24 ddkilzer 2007-12-18 09:28:30 -0800 (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! 64639 25 17975 xan.lopez 2007-12-18 09:45:31 -0800 Created attachment 17975 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(-) 64647 26 17975 ggaren 2007-12-18 10:26:15 -0800 Comment on attachment 17975 0001-Use-less-strict-memory-operand-constraint-on-inline.patch r=me, based on above comments 64686 27 ddkilzer 2007-12-18 18:59:25 -0800 $ svn commit JavaScriptCore Sending JavaScriptCore/ChangeLog Sending JavaScriptCore/wtf/TCSpinLock.h Transmitting file data .. Committed revision 28847. 17187 2007-11-11 09:11:10 -0800 2007-11-12 12:10:11 -0800 Possible patch diff text/plain 411 mh+webkit ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL3d0Zi9UQ1NwaW5Mb2NrLmggYi9KYXZhU2NyaXB0 Q29yZS93dGYvVENTcGluTG9jay5oCmluZGV4IDQzN2YwN2IuLjE1ZDM0NjggMTAwNjQ0Ci0tLSBh L0phdmFTY3JpcHRDb3JlL3d0Zi9UQ1NwaW5Mb2NrLmgKKysrIGIvSmF2YVNjcmlwdENvcmUvd3Rm L1RDU3BpbkxvY2suaApAQCAtMTA2LDcgKzEwNiw3IEBAIHN0cnVjdCBUQ01hbGxvY19TcGluTG9j ayB7CiAgICAgICAoImlzeW5jXG5cdCIKICAgICAgICAiZWllaW9cblx0IgogICAgICAgICJzdHcg JTEsICUwIgotICAgICAgIDogIj1vIiAocHJpdmF0ZV9sb2Nrd29yZF8pIAorICAgICAgIDogIj1t IiAocHJpdmF0ZV9sb2Nrd29yZF8pCiAgICAgICAgOiAiciIgKDApCiAgICAgICAgOiAibWVtb3J5 Iik7CiAjZW5kaWYK 17975 2007-12-18 09:45:31 -0800 2007-12-18 10:26:15 -0800 0001-Use-less-strict-memory-operand-constraint-on-inline.patch 0001-Use-less-strict-memory-operand-constraint-on-inline.patch text/plain 1631 xan.lopez RnJvbSA1NmI4ODNmYmI1ZTg2Y2Q4MTc4NzIxNjA1NzdiYzUwYzM2YjFiZDIyIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBYYW4gTG9wZXogPHhhbkBnbm9tZS5vcmc+CkRhdGU6IFR1ZSwg MTggRGVjIDIwMDcgMTk6NDI6NDYgKzAyMDAKU3ViamVjdDogW1BBVENIXSBVc2UgbGVzcyBzdHJp Y3QgbWVtb3J5IG9wZXJhbmQgY29uc3RyYWludCBvbiBpbmxpbmUgQVNNIGZvciBUQ1NwaW5Mb2Nr LmguCgotLS0KIEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyAgICAgICAgfCAgIDE1ICsrKysrKysr KysrKysrKwogSmF2YVNjcmlwdENvcmUvd3RmL1RDU3BpbkxvY2suaCB8ICAgIDYgKysrKystCiAy IGZpbGVzIGNoYW5nZWQsIDIwIGluc2VydGlvbnMoKyksIDEgZGVsZXRpb25zKC0pCgpkaWZmIC0t Z2l0IGEvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9n CmluZGV4IDJkNDRkYjIuLmExOGM4ZTcgMTAwNjQ0Ci0tLSBhL0phdmFTY3JpcHRDb3JlL0NoYW5n ZUxvZworKysgYi9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOCBAQAorMjAw Ny0xMi0xOCAgWGFuIExvcGV6ICA8eGFuQGdub21lLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXggaHR0cDovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTQ1MjEKKyAgICAgICAgQnVnIDE0NTIxOiBKYXZhU2NyaXB0Q29yZSBm YWlscyB0byBidWlsZCBvbiBMaW51eC9QUEMgZ2NjIDQuMS4yCisgICAgICAgIAorICAgICAgICAq IHd0Zi9UQ1NwaW5Mb2NrLmg6CisgICAgICAgIChUQ01hbGxvY19TcGluTG9jazo6VW5sb2NrKToK KworICAgICAgICBVc2UgbGVzcyBzdHJpY3QgbWVtb3J5IG9wZXJhbmQgY29uc3RyYWludCBvbiBp bmxpbmUgYXNtIGdlbmVyYXRpb24uCisgICAgICAgIFBMQVRGT1JNKERBUldJTikgbGVmdCB1bnBh dGNoZWQgZHVlIHRvIEFwcGxlJ3MgR0NDIGJ1Zy4KKworICAgICAgICBQYXRjaCBieSBEYXZpZCBL aWx6ZXIgPGRka2lsemVyQHdlYmtpdC5vcmc+CisKIDIwMDctMTItMTcgIERhcmluIEFkbGVyICA8 ZGFyaW5AYXBwbGUuY29tPgogCiAgICAgICAgIC0gc3BlY3VsYXRpdmUgYnVpbGQgZml4IGZvciBu b24tZ2NjIHBsYXRmb3JtcwpkaWZmIC0tZ2l0IGEvSmF2YVNjcmlwdENvcmUvd3RmL1RDU3Bpbkxv Y2suaCBiL0phdmFTY3JpcHRDb3JlL3d0Zi9UQ1NwaW5Mb2NrLmgKaW5kZXggZWY2NDc0ZS4uNTI3 ZGZhZSAxMDA2NDQKLS0tIGEvSmF2YVNjcmlwdENvcmUvd3RmL1RDU3BpbkxvY2suaAorKysgYi9K YXZhU2NyaXB0Q29yZS93dGYvVENTcGluTG9jay5oCkBAIC0xMDIsNyArMTAyLDExIEBAIHN0cnVj dCBUQ01hbGxvY19TcGluTG9jayB7CiAgICAgICAoImlzeW5jXG5cdCIKICAgICAgICAiZWllaW9c blx0IgogICAgICAgICJzdHcgJTEsICUwIgotICAgICAgIDogIj1vIiAobG9ja3dvcmRfKSAKKyNp ZiBQTEFURk9STShEQVJXSU4pCisgICAgICAgOiAiPW8iIChsb2Nrd29yZF8pCisjZWxzZQorICAg ICAgIDogIj1tIiAobG9ja3dvcmRfKSAKKyNlbmRpZgogICAgICAgIDogInIiICgwKQogICAgICAg IDogIm1lbW9yeSIpOwogI2VuZGlmCi0tIAoxLjUuMy40Cgo=