Bug 14521 - JavaScriptCore fails to build on Linux/PPC gcc 4.1.2
Summary: JavaScriptCore fails to build on Linux/PPC gcc 4.1.2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 523.x (Safari 3)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 14974 16293 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-07-04 09:28 PDT by Xan Lopez
Modified: 2007-12-18 18:59 PST (History)
5 users (show)

See Also:


Attachments
Possible patch (411 bytes, patch)
2007-11-11 09:11 PST, Mike Hommey
no flags Details | Formatted Diff | Diff
0001-Use-less-strict-memory-operand-constraint-on-inline.patch (1.59 KB, patch)
2007-12-18 09:45 PST, Xan Lopez
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xan Lopez 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'
Comment 1 Patryk Zawadzki 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
Comment 2 David Kilzer (:ddkilzer) 2007-10-23 08:36:30 PDT
*** Bug 14974 has been marked as a duplicate of this bug. ***
Comment 3 David Kilzer (:ddkilzer) 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

Comment 4 Mike Hommey 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...
Comment 5 David Kilzer (:ddkilzer) 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.

Comment 6 Xan Lopez 2007-11-11 12:46:57 PST
Works ok in Linux/PPC too (but no clue about its correctness either).
Comment 7 David Kilzer (:ddkilzer) 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.
Comment 8 Mike Hommey 2007-11-11 13:32:32 PST
FWIW, the PPC inline code has been added, according to the changelog, by mjs in revision 10701.
Comment 9 David Kilzer (:ddkilzer) 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?
Comment 10 Maciej Stachowiak 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
Comment 11 Mark Rowe (bdash) 2007-11-11 22:03:31 PST
Landed in r27708.
Comment 12 Mark Rowe (bdash) 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)

Comment 13 David Kilzer (:ddkilzer) 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?

Comment 15 David Kilzer (:ddkilzer) 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.

Comment 16 Mark Rowe (bdash) 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.
Comment 17 Mike Hommey 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.
Comment 18 David Kilzer (:ddkilzer) 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.

Comment 19 Darin Adler 2007-11-12 12:10:11 PST
Comment on attachment 17187 [details]
Possible patch

Clearing review flag.
Comment 20 David Kilzer (:ddkilzer) 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.

Comment 21 Mark Rowe (bdash) 2007-12-04 11:33:37 PST
*** Bug 16293 has been marked as a duplicate of this bug. ***
Comment 22 David Kilzer (:ddkilzer) 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

Comment 23 Xan Lopez 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? 
Comment 24 David Kilzer (:ddkilzer) 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!

Comment 25 Xan Lopez 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(-)
Comment 26 Geoffrey Garen 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
Comment 27 David Kilzer (:ddkilzer) 2007-12-18 18:59:25 PST
$ svn commit JavaScriptCore
Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/wtf/TCSpinLock.h
Transmitting file data ..
Committed revision 28847.