[RFC] Patch to fix register allocation (PR680)

[Dr Andrew John Hughes]

On 19:25 Fri 25 Mar     , Deepak Bhole wrote:
> Hi,
> 
> I have put all the details in PR680:
> http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=680
> 
> To summarize, the current inline assembly code in
> release_store_fence(volatile jbyte*  p, jbyte  v) is using an incorrect
> output operand. It works with the current gcc by pure luck, but with gcc
> 4.6, it fails.
> 
> Attached patches for icedtea6 and icedtea7 fix the issue. I have also
> tested this with icedtea6+hs20 and there were no issues.
> 
> Thanks to Jakub Jelinek for help with the issue.
> 

Have you run this by the HotSpot developers?

> IcedTea6 ChangeLog:
> 2011-03-25  Deepak Bhole <dbhole at redhat.com>
> 
>     PR680: Register allocation in release_store_fence(jbyte*, jbyte) is
>     incorrect
>     * Makefile.am: Apply new pr680-gcc-register-allocation-fix.patch patch.
>     * NEWS: Updated
>     * patches/pr680-gcc-register-allocation-fix.patch: New patch. Fixes
>     register allocation by constraining output to one of a/b/c/d registers.
> 

Looks fine in theory but I would like some feedback from the HotSpot developers first.

> IcedTea7 ChangeLog:
> 2011-03-25  Deepak Bhole <dbhole at redhat.com>
> 
>     PR680: Register allocation in release_store_fence(jbyte*, jbyte) is
>     incorrect
>     * Makefile.am: Apply new icedtea-pr680-gcc-register-allocation-fix.patch
>     patch.
>     * NEWS: Updated
>     * patches/icedtea-pr680-gcc-register-allocation-fix.patch: New patch. 
>     Fixes register allocation by constraining output to one of a/b/c/d
>     registers.
> 

For IcedTea7, patches should go into the IcedTea forest where
possible, not as patches in the tree.  This also uses the obsolete
'icedtea-' prefix which has been removed on 6 and will be on 7.

The most sensible route with 7 is to get it pushed into the main
OpenJDK7 tree and then pull in the patch.  This is what we do with
Shark fixes.

> In addition to the tips, I would also recommend this for 1.10 since gcc 4.6
> will be out soon and 1.10 will be the stable release for the next little while.
> 
> Okay to commit?
> 

No.

> Cheers,
> Deepak

> diff -r 8c01fdae79ef Makefile.am
> --- a/Makefile.am	Fri Mar 25 13:44:12 2011 +0100
> +++ b/Makefile.am	Fri Mar 25 19:07:42 2011 -0400
> @@ -331,7 +331,8 @@
>  	patches/openjdk/7027667-AAShapePipeRegTest.patch \
>  	patches/openjdk/7019861-AA-regression-fix.patch \
>  	patches/openjdk/6768387-jtable_not_serializable.patch \
> -	patches/mark_sun_toolkit_privileged_code.patch
> +	patches/mark_sun_toolkit_privileged_code.patch \
> +	patches/pr680-gcc-register-allocation-fix.patch
>  
>  if WITH_ALT_HSBUILD
>  ICEDTEA_PATCHES += \
> diff -r 8c01fdae79ef NEWS
> --- a/NEWS	Fri Mar 25 13:44:12 2011 +0100
> +++ b/NEWS	Fri Mar 25 19:07:42 2011 -0400
> @@ -20,6 +20,7 @@
>      make check jtreg_checks="langtools hotspot".
>      If none is provided make check runs all testsuites.
>    - Fixed AccessControlContext which was thrown while working with Color class in a PropertyEditor
> +  - PR680: Register allocation in release_store_fence(jbyte*, jbyte) is incorrect
>  * CACAO
>    - Ignore all unknown options, but report them.
>    - Fixes build for newer gcc (at least 4.4) on PPC64, breaks older gcc.
> diff -r 8c01fdae79ef patches/pr680-gcc-register-allocation-fix.patch
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/patches/pr680-gcc-register-allocation-fix.patch	Fri Mar 25 19:07:42 2011 -0400
> @@ -0,0 +1,12 @@
> +diff -ur openjdk.orig/hotspot/src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp openjdk/hotspot/src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp
> +--- openjdk.orig/hotspot/src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp	2011-02-28 11:03:15.000000000 -0500
> ++++ openjdk/hotspot/src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp	2011-03-25 16:15:31.461400000 -0400
> +@@ -148,7 +148,7 @@
> + // Must duplicate definitions instead of calling store_fence because we don't want to cast away volatile.
> + inline void     OrderAccess::release_store_fence(volatile jbyte*  p, jbyte  v) {
> +   __asm__ volatile (  "xchgb (%2),%0"
> +-                    : "=r" (v)
> ++                    : "=q" (v)
> +                     : "0" (v), "r" (p)
> +                     : "memory");
> + }

> diff -r eeb7d84e7ae5 Makefile.am
> --- a/Makefile.am	Mon Mar 21 20:47:22 2011 +0000
> +++ b/Makefile.am	Fri Mar 25 19:07:13 2011 -0400
> @@ -281,7 +281,8 @@
>  	patches/parisc.patch \
>  	patches/sh4-support.patch \
>  	patches/jtreg-httpTest.patch \
> -	patches/icedtea-update-bootclasspath.patch
> +	patches/icedtea-update-bootclasspath.patch \
> +	patches/icedtea-pr680-gcc-register-allocation-fix.patch
>  
>  # Conditional patches
>  
> diff -r eeb7d84e7ae5 NEWS
> --- a/NEWS	Mon Mar 21 20:47:22 2011 +0000
> +++ b/NEWS	Fri Mar 25 19:07:13 2011 -0400
> @@ -6,6 +6,8 @@
>    - Match Shark in icedtea6, makes OSR work by removing vestigal check.
>    - LLVM 2.7 non-product fixes.
>    - Correct suffix for the llvm.atomic.cmp.swap intrinsic.
> +* Bug fixes
> +  - PR680: Register allocation in release_store_fence(jbyte*, jbyte) is incorrect
>  
>  New in release 1.13 (2010-07-29)
>  
> diff -r eeb7d84e7ae5 patches/icedtea-pr680-gcc-register-allocation-fix.patch
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/patches/icedtea-pr680-gcc-register-allocation-fix.patch	Fri Mar 25 19:07:13 2011 -0400
> @@ -0,0 +1,12 @@
> +diff -ur openjdk.orig/hotspot/src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp openjdk/hotspot/src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp
> +--- openjdk.orig/hotspot/src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp	2011-02-28 11:03:15.000000000 -0500
> ++++ openjdk/hotspot/src/os_cpu/linux_x86/vm/orderAccess_linux_x86.inline.hpp	2011-03-25 16:15:31.461400000 -0400
> +@@ -148,7 +148,7 @@
> + // Must duplicate definitions instead of calling store_fence because we don't want to cast away volatile.
> + inline void     OrderAccess::release_store_fence(volatile jbyte*  p, jbyte  v) {
> +   __asm__ volatile (  "xchgb (%2),%0"
> +-                    : "=r" (v)
> ++                    : "=q" (v)
> +                     : "0" (v), "r" (p)
> +                     : "memory");
> + }


-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37