Bug 14521 – JavaScriptCore fails to build on Linux/PPC gcc 4.1.2

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Xan Lopez]

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'

[Patryk Zawadzki]

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)]

*** Bug 14974 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

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]

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)]

(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]

Works ok in Linux/PPC too (but no clue about its correctness either).

[David Kilzer (:ddkilzer)]

(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]

FWIW, the PPC inline code has been added, according to the changelog, by mjs in revision 10701.

[David Kilzer (:ddkilzer)]

(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 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)]

Landed in r27708.

[Mark Rowe (bdash)]

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)]

(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?

[Alexey Proskuryakov]

(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>

[David Kilzer (:ddkilzer)]

(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)]

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]

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)]

(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 on attachment 17187 [details]
Possible patch

Clearing review flag.

[David Kilzer (:ddkilzer)]

(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)]

*** Bug 16293 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

(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]

(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)]

(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]

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 on attachment 17975 [details]
0001-Use-less-strict-memory-operand-constraint-on-inline.patch

r=me, based on above comments

[David Kilzer (:ddkilzer)]

$ svn commit JavaScriptCore
Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/wtf/TCSpinLock.h
Transmitting file data ..
Committed revision 28847.