PR3634: Shenandoah still broken on s390 with aarch64-shenandoah-jdk8u181-b16

[Andrew Hughes]

On Mon, 12 Nov 2018 at 17:03, Aleksey Shipilev <shade at redhat.com> wrote:
>
> On 11/12/2018 05:34 PM, Andrew Hughes wrote:
> > The build of Shenandoah is still broken on s390 (the 31-bit platform)
> > after the latest merge, due to a mismatch in the MAX2 macro:
> >
> > https://icedtea.classpath.org/bugzilla/show_bug.cgi?id=3634
> >
> > The working solution I have for this at present (and used in our
> > latest RPMs & IcedTea release) is to update ShenandoahMinRegionSize
> > and ShenandoahMaxRegionSize to be size_t instead of uintx. This
> > shouldn't make a difference on other platforms where they are the
> > same. It does on s390 where size_t is bigger than uintx.
> >
> > https://cr.openjdk.java.net/~andrew/shenandoah-8/pr3634/webrev.01/
>
> I think this would not work without:
>   https://bugs.openjdk.java.net/browse/JDK-8054823
>
> If you run Shenandoah tests with patch right now, then gc/shenandoah/options/TestRegionSizeArgs.java
> would fail with "Improperly specified VM option 'ShenandoahMinRegionSize=255K"
>
> In other words, we have to keep it uintx, and cast where appropriate. For example, like this:
>
> # HG changeset patch
> # User shade
> # Date 1542042122 -3600
> #      Mon Nov 12 18:02:02 2018 +0100
> # Node ID d215450bcdcad401497f2269f9448104ac483b29
> # Parent  bffbdb3f8f1354f92a16913ee55ce8d507ce8474
> Cast Shenandoah{Min,Max}RegionSize to size_t
>
> diff -r bffbdb3f8f13 -r d215450bcdca src/share/vm/gc_implementation/shenandoah/shenandoahHeapRegion.cpp
> --- a/src/share/vm/gc_implementation/shenandoah/shenandoahHeapRegion.cpp        Sun Nov 11 13:20:44
> 2018 +0100
> +++ b/src/share/vm/gc_implementation/shenandoah/shenandoahHeapRegion.cpp        Mon Nov 12 18:02:02
> 2018 +0100
> @@ -508,8 +508,8 @@
>      region_size = max_heap_size / ShenandoahTargetNumRegions;
>
>      // Now make sure that we don't go over or under our limits.
> -    region_size = MAX2(ShenandoahMinRegionSize, region_size);
> -    region_size = MIN2(ShenandoahMaxRegionSize, region_size);
> +    region_size = MAX2<size_t>(ShenandoahMinRegionSize, region_size);
> +    region_size = MIN2<size_t>(ShenandoahMaxRegionSize, region_size);
>
>    } else {
>      if (ShenandoahHeapRegionSize > initial_heap_size / MIN_NUM_REGIONS) {
>
> Can you try if this helps s390? If so, we would push it to sh/jdk8u, and it can then be either
> cherry-picked to integration repo, or come with the next bulk integration.
>
> -Aleksey
>
>

Yes, it builds with that fix instead.
-- 
Andrew :)

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

Web Site: http://fuseyism.com
Twitter: https://twitter.com/gnu_andrew_java
PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222