RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

Stuart Monteith
Hello,
   The Aarch64 backend decompresses klass pointers using XOR if the
size of the compressed classes space is less then the base address.
However, in JDK 10  CompressedClassSpaceSize doesn't represent the
range of the compressed shared classes area, which is set to 4GB.

   https://bugs.openjdk.java.net/browse/JDK-8193266
   http://cr.openjdk.java.net/~smonteith/8193266/webrev/

I've speculatively put up a patch which deals with the issue on
Aarch64. While I understand the problem with the generated aarch64
code, I don't believe understand the CDS code sufficiently at this
point to determine if there is a bug within the CDS code regarding the
CompressedClassSpaceSize parameter.

Thanks,
    Stuart
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

aph-2
Hi,

On 08/12/17 19:07, Stuart Monteith wrote:

>    The Aarch64 backend decompresses klass pointers using XOR if the
> size of the compressed classes space is less then the base address.
> However, in JDK 10  CompressedClassSpaceSize doesn't represent the
> range of the compressed shared classes area, which is set to 4GB.
>
>    https://bugs.openjdk.java.net/browse/JDK-8193266
>    http://cr.openjdk.java.net/~smonteith/8193266/webrev/
>
> I've speculatively put up a patch which deals with the issue on
> Aarch64. While I understand the problem with the generated aarch64
> code, I don't believe understand the CDS code sufficiently at this
> point to determine if there is a bug within the CDS code regarding the
> CompressedClassSpaceSize parameter.

Good catch!

Hmm.  This sort-of papers over the problem, in that it disables XOR-
encoding of class metadata.  I would have thought it makes more sense
to set narrow_klass_base high enough so that this does not happen.  We
really want narrow_klass_shift to be zero if we can, which should be
OK if the shared klasses are in a 4G range.

I think that I see the logic of your patch, but I think it's a very
complicated way of saying

  if (narrow_klass_base() > 1ul << 32 << narrow_klass_shift())

Also, beware of putting any more complexity into
use_XOR_for_compressed_class_base because it's executed very often.
We should perhaps do it once, when we allocate the metaspace.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

aph-2
On 09/12/17 11:37, Andrew Haley wrote:
> Hmm.  This sort-of papers over the problem, in that it disables XOR-
> encoding of class metadata.  I would have thought it makes more sense
> to set narrow_klass_base high enough so that this does not happen.  We
> really want narrow_klass_shift to be zero if we can, which should be
> OK if the shared klasses are in a 4G range.

And I now see that in the very odd code in question the shift is set
to 3, regardless of the fact that it's not necessary.  Something to
do with AOT, which I guess always does the shift.  I'd prefer
AArch64 always to set the shift to zero, and we could then search
for a suitable base.  We never need a shift for compressed classes,
given that 4G seems to be hard-wired as a maximum size.

We could establish the convention that the shift is always zero
on AArch64, and then this problem would go away completely.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

aph-2
On 09/12/17 19:02, Andrew Haley wrote:

> We could establish the convention that the shift is always zero
> on AArch64, and then this problem would go away completely.

Ahh, I just realized that this too is too simple, because we don't
know where the metaspace is going to be, so we have to generate code
which can run wherever the metaspace is.  It's tricky.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

Stuart Monteith
I've copied and pasted the calculation for UnscaledClassSpaceMax in
use_XOR_for_compressed_class_base to match the variable of the same
name in MetaspaceShared::initialize_dumptime_shared_and_meta_spaces  -
I did this for ease of reference while we decide what to do.

I guess you're referring to this piece of code:


As far as I can tell, the parameter CompressedClassSpaceSize is ignored.

On 10 December 2017 at 09:46, Andrew Haley <[hidden email]> wrote:

> On 09/12/17 19:02, Andrew Haley wrote:
>
>> We could establish the convention that the shift is always zero
>> on AArch64, and then this problem would go away completely.
>
> Ahh, I just realized that this too is too simple, because we don't
> know where the metaspace is going to be, so we have to generate code
> which can run wherever the metaspace is.  It's tricky.
>
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

aph-2
On 11/12/17 13:43, Stuart Monteith wrote:
> I've copied and pasted the calculation for UnscaledClassSpaceMax in
> use_XOR_for_compressed_class_base to match the variable of the same
> name in MetaspaceShared::initialize_dumptime_shared_and_meta_spaces  -
> I did this for ease of reference while we decide what to do.

Yes, I get that.  I don't want to pessimize the "normal" use of OpenJDK
AArch64 for the sake of this weird case.  I guess that as long as no-
one explicitly sets the base of the metaspace below 32 bits it doesn't
matter anyway.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

Stuart Monteith
Hello,
   Here's most latest patch:

http://cr.openjdk.java.net/~smonteith/8193266/webrev-2/

Things to note:
   I've moved the constant "UnscaledClassSpaceMax" from metaspace.cpp
and metaspaceShared.cpp
MetaspaceShared::initialize_dumptime_shared_and_meta_spaces. This is
the same value as UnscaledOopHeapMax. Perhaps that should be used
instead of UnscaledClassSpaceMax in all three places.
   I'm using the constant in MacroAssembler_aarch64.hpp in the
MacroAssembler constructor instead of CompressedClassSpaceSize.
   The calculation has changed - "1u" has changed to "1ul" as the
address "0x100000000" is not representable as a 32 bit value.
   The test has changed from ">" to ">=" as the greatest offset for
4GB is "0xffffffff", and so 0x100000000 and upwards are all acceptable
for XORing offsets to.

The intention is to prevent bad code from being generated if the
compressed class addresses range changes.

BR,
   Stuart

On 11 December 2017 at 13:54, Andrew Haley <[hidden email]> wrote:

> On 11/12/17 13:43, Stuart Monteith wrote:
>> I've copied and pasted the calculation for UnscaledClassSpaceMax in
>> use_XOR_for_compressed_class_base to match the variable of the same
>> name in MetaspaceShared::initialize_dumptime_shared_and_meta_spaces  -
>> I did this for ease of reference while we decide what to do.
>
> Yes, I get that.  I don't want to pessimize the "normal" use of OpenJDK
> AArch64 for the sake of this weird case.  I guess that as long as no-
> one explicitly sets the base of the metaspace below 32 bits it doesn't
> matter anyway.
>
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

Dmitry Samersoff-2
Stuart,

I can confirm that the patch works in our environment.

-Dmitry

On 13.12.2017 18:56, Stuart Monteith wrote:

> Hello,
>    Here's most latest patch:
>
> http://cr.openjdk.java.net/~smonteith/8193266/webrev-2/
>
> Things to note:
>    I've moved the constant "UnscaledClassSpaceMax" from metaspace.cpp
> and metaspaceShared.cpp
> MetaspaceShared::initialize_dumptime_shared_and_meta_spaces. This is
> the same value as UnscaledOopHeapMax. Perhaps that should be used
> instead of UnscaledClassSpaceMax in all three places.
>    I'm using the constant in MacroAssembler_aarch64.hpp in the
> MacroAssembler constructor instead of CompressedClassSpaceSize.
>    The calculation has changed - "1u" has changed to "1ul" as the
> address "0x100000000" is not representable as a 32 bit value.
>    The test has changed from ">" to ">=" as the greatest offset for
> 4GB is "0xffffffff", and so 0x100000000 and upwards are all acceptable
> for XORing offsets to.
>
> The intention is to prevent bad code from being generated if the
> compressed class addresses range changes.
>
> BR,
>    Stuart
>
> On 11 December 2017 at 13:54, Andrew Haley <[hidden email]> wrote:
>> On 11/12/17 13:43, Stuart Monteith wrote:
>>> I've copied and pasted the calculation for UnscaledClassSpaceMax in
>>> use_XOR_for_compressed_class_base to match the variable of the same
>>> name in MetaspaceShared::initialize_dumptime_shared_and_meta_spaces  -
>>> I did this for ease of reference while we decide what to do.
>>
>> Yes, I get that.  I don't want to pessimize the "normal" use of OpenJDK
>> AArch64 for the sake of this weird case.  I guess that as long as no-
>> one explicitly sets the base of the metaspace below 32 bits it doesn't
>> matter anyway.
>>
>> --
>> Andrew Haley
>> Java Platform Lead Engineer
>> Red Hat UK Ltd. <https://www.redhat.com>
>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671


Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

aph-2
In reply to this post by Stuart Monteith
On 13/12/17 15:56, Stuart Monteith wrote:
>    I've moved the constant "UnscaledClassSpaceMax" from metaspace.cpp
> and metaspaceShared.cpp
> MetaspaceShared::initialize_dumptime_shared_and_meta_spaces. This is
> the same value as UnscaledOopHeapMax. Perhaps that should be used
> instead of UnscaledClassSpaceMax in all three places.
>    I'm using the constant in MacroAssembler_aarch64.hpp in the
> MacroAssembler constructor instead of CompressedClassSpaceSize.

I don't thing that really helps.  We don't need a global constant
which is the worst possible case for UnscaledClassSpaceMax because
we already know what that is: it's 2**32.

We need to know how much space is in use.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

Stuart Monteith
Ok, the constant was just an attempt at reducing the likelihood of the
backend generating bad addresses if the range changes. We have a
testcase that might trip if the future changes.

We'll need the 4GB limit during dumping the shared metaspace. During
runtime, we'll need the maximum size of the compressed metaspace. I
suppose that's the intention behind using CompressedClassSpaceSize.
I've just been checking to see if that still holds.

BR,
   Stuart


On 18 December 2017 at 09:15, Andrew Haley <[hidden email]> wrote:

> On 13/12/17 15:56, Stuart Monteith wrote:
>>    I've moved the constant "UnscaledClassSpaceMax" from metaspace.cpp
>> and metaspaceShared.cpp
>> MetaspaceShared::initialize_dumptime_shared_and_meta_spaces. This is
>> the same value as UnscaledOopHeapMax. Perhaps that should be used
>> instead of UnscaledClassSpaceMax in all three places.
>>    I'm using the constant in MacroAssembler_aarch64.hpp in the
>> MacroAssembler constructor instead of CompressedClassSpaceSize.
>
> I don't thing that really helps.  We don't need a global constant
> which is the worst possible case for UnscaledClassSpaceMax because
> we already know what that is: it's 2**32.
>
> We need to know how much space is in use.
>
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

Stuart Monteith
Hello,
   I've redone the patch such that we're passing different values
depending on whether we are dumping or are using shared classes. There
is a point before this initialisation where the  default is used,
which is 4GB:

http://cr.openjdk.java.net/~smonteith/8193266/webrev-3/


BR,
   Stuart

On 18 December 2017 at 13:41, Stuart Monteith
<[hidden email]> wrote:

> Ok, the constant was just an attempt at reducing the likelihood of the
> backend generating bad addresses if the range changes. We have a
> testcase that might trip if the future changes.
>
> We'll need the 4GB limit during dumping the shared metaspace. During
> runtime, we'll need the maximum size of the compressed metaspace. I
> suppose that's the intention behind using CompressedClassSpaceSize.
> I've just been checking to see if that still holds.
>
> BR,
>    Stuart
>
>
> On 18 December 2017 at 09:15, Andrew Haley <[hidden email]> wrote:
>> On 13/12/17 15:56, Stuart Monteith wrote:
>>>    I've moved the constant "UnscaledClassSpaceMax" from metaspace.cpp
>>> and metaspaceShared.cpp
>>> MetaspaceShared::initialize_dumptime_shared_and_meta_spaces. This is
>>> the same value as UnscaledOopHeapMax. Perhaps that should be used
>>> instead of UnscaledClassSpaceMax in all three places.
>>>    I'm using the constant in MacroAssembler_aarch64.hpp in the
>>> MacroAssembler constructor instead of CompressedClassSpaceSize.
>>
>> I don't thing that really helps.  We don't need a global constant
>> which is the worst possible case for UnscaledClassSpaceMax because
>> we already know what that is: it's 2**32.
>>
>> We need to know how much space is in use.
>>
>> --
>> Andrew Haley
>> Java Platform Lead Engineer
>> Red Hat UK Ltd. <https://www.redhat.com>
>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

aph-2
On 03/01/18 13:53, Stuart Monteith wrote:
>    I've redone the patch such that we're passing different values
> depending on whether we are dumping or are using shared classes. There
> is a point before this initialisation where the  default is used,
> which is 4GB:
>
> http://cr.openjdk.java.net/~smonteith/8193266/webrev-3/

That looks good, except that you seem to have deleted a line
(by mistake?) in universe.hpp.

If I were doing this, I'd also take the opportunity to calculate
can_use_XOR once, and take it out of the Assembler's constructor.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

Stuart Monteith
Hello,
 Yes, I deleted a line by mistake. I've moved the calculation, as you
suggest, and hold the result in Universe:

  http://cr.openjdk.java.net/~smonteith/8193266/webrev-4/

I've changed the type of use_XOR_for_compressed_class_base from
uint64_t to bool, I saw no reason for it to be anything else.

BR,
  Stuart

On 3 January 2018 at 14:27, Andrew Haley <[hidden email]> wrote:

> On 03/01/18 13:53, Stuart Monteith wrote:
>>    I've redone the patch such that we're passing different values
>> depending on whether we are dumping or are using shared classes. There
>> is a point before this initialisation where the  default is used,
>> which is 4GB:
>>
>> http://cr.openjdk.java.net/~smonteith/8193266/webrev-3/
>
> That looks good, except that you seem to have deleted a line
> (by mistake?) in universe.hpp.
>
> If I were doing this, I'd also take the opportunity to calculate
> can_use_XOR once, and take it out of the Assembler's constructor.
>
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

coleen.phillimore
Hi I was going to offer to sponsor this since it touches shared code but
I'm not sure I like that there's AARCH64 specific code in
universe.cpp/hpp.   And the name is somewhat offputting, suggesting
implementation details of one target leaking into shared code.

set_use_XOR_for_compressed_class_base

I think webrev-3 looked more reasonable, and could elide the #ifdef
AARCH64 in the shared code for that version.   And the indentation is
better.

Coleen

On 1/4/18 6:35 AM, Stuart Monteith wrote:

> Hello,
>   Yes, I deleted a line by mistake. I've moved the calculation, as you
> suggest, and hold the result in Universe:
>
>    http://cr.openjdk.java.net/~smonteith/8193266/webrev-4/
>
> I've changed the type of use_XOR_for_compressed_class_base from
> uint64_t to bool, I saw no reason for it to be anything else.
>
> BR,
>    Stuart
>
> On 3 January 2018 at 14:27, Andrew Haley <[hidden email]> wrote:
>> On 03/01/18 13:53, Stuart Monteith wrote:
>>>     I've redone the patch such that we're passing different values
>>> depending on whether we are dumping or are using shared classes. There
>>> is a point before this initialisation where the  default is used,
>>> which is 4GB:
>>>
>>> http://cr.openjdk.java.net/~smonteith/8193266/webrev-3/
>> That looks good, except that you seem to have deleted a line
>> (by mistake?) in universe.hpp.
>>
>> If I were doing this, I'd also take the opportunity to calculate
>> can_use_XOR once, and take it out of the Assembler's constructor.
>>
>> --
>> Andrew Haley
>> Java Platform Lead Engineer
>> Red Hat UK Ltd. <https://www.redhat.com>
>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

aph-2
Hi,

On 04/01/18 14:26, [hidden email] wrote:

>  I was going to offer to sponsor this since it touches shared code but
> I'm not sure I like that there's AARCH64 specific code in
> universe.cpp/hpp.   And the name is somewhat offputting, suggesting
> implementation details of one target leaking into shared code.
>
> set_use_XOR_for_compressed_class_base
>
> I think webrev-3 looked more reasonable, and could elide the #ifdef
> AARCH64 in the shared code for that version.   And the indentation is
> better.

I hate the #ifdef AARCH64 stuff too, but it's always a sign that there
is something wrong with the front-end to back-end modularization.  We
can handle the use_XOR_for_compressed_class_base later: we really
should have a way to communicate with the back ends when the memory
layout is initialized.  We can go with webrev-3.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

Stuart Monteith
In reply to this post by coleen.phillimore
Thank you for looking at it Coleen. I share your dissatisfaction with
webrev-4. I an uneasy about solving the problem in this way, as before
given narrow_klass_base and CompressedClassSpaceSize, you would know
what addresses to expect. However, this contract changed when during
dumping the class space size changes to 4GB, and
CompressedClassSpaceSize no longer matched this size. So webrev-3 is
my attempt to work round this by ignoring CompressedClassSpaceSize and
have another value tell us what we really want to know.
The name is perhaps less than ideal, but it really does contain an
implementation detail leaking into shared code. Perhaps I
misunderstood Andrew's intentions.

BR,
   Stuart

On 4 January 2018 at 14:26,  <[hidden email]> wrote:

> Hi I was going to offer to sponsor this since it touches shared code but I'm
> not sure I like that there's AARCH64 specific code in universe.cpp/hpp.
> And the name is somewhat offputting, suggesting implementation details of
> one target leaking into shared code.
>
> set_use_XOR_for_compressed_class_base
>
> I think webrev-3 looked more reasonable, and could elide the #ifdef AARCH64
> in the shared code for that version.   And the indentation is better.
>
> Coleen
>
>
> On 1/4/18 6:35 AM, Stuart Monteith wrote:
>>
>> Hello,
>>   Yes, I deleted a line by mistake. I've moved the calculation, as you
>> suggest, and hold the result in Universe:
>>
>>    http://cr.openjdk.java.net/~smonteith/8193266/webrev-4/
>>
>> I've changed the type of use_XOR_for_compressed_class_base from
>> uint64_t to bool, I saw no reason for it to be anything else.
>>
>> BR,
>>    Stuart
>>
>> On 3 January 2018 at 14:27, Andrew Haley <[hidden email]> wrote:
>>>
>>> On 03/01/18 13:53, Stuart Monteith wrote:
>>>>
>>>>     I've redone the patch such that we're passing different values
>>>> depending on whether we are dumping or are using shared classes. There
>>>> is a point before this initialisation where the  default is used,
>>>> which is 4GB:
>>>>
>>>> http://cr.openjdk.java.net/~smonteith/8193266/webrev-3/
>>>
>>> That looks good, except that you seem to have deleted a line
>>> (by mistake?) in universe.hpp.
>>>
>>> If I were doing this, I'd also take the opportunity to calculate
>>> can_use_XOR once, and take it out of the Assembler's constructor.
>>>
>>> --
>>> Andrew Haley
>>> Java Platform Lead Engineer
>>> Red Hat UK Ltd. <https://www.redhat.com>
>>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

Stuart Monteith
In reply to this post by aph-2
I've removed the AARCH64 conditionals, added the empty line I removed,
and changed the type of "use_XOR_for_compressed_class_base" to bool.

http://cr.openjdk.java.net/~smonteith/8193266/webrev-5/

BR,
   Stuart


On 4 January 2018 at 14:45, Andrew Haley <[hidden email]> wrote:

> Hi,
>
> On 04/01/18 14:26, [hidden email] wrote:
>>  I was going to offer to sponsor this since it touches shared code but
>> I'm not sure I like that there's AARCH64 specific code in
>> universe.cpp/hpp.   And the name is somewhat offputting, suggesting
>> implementation details of one target leaking into shared code.
>>
>> set_use_XOR_for_compressed_class_base
>>
>> I think webrev-3 looked more reasonable, and could elide the #ifdef
>> AARCH64 in the shared code for that version.   And the indentation is
>> better.
>
> I hate the #ifdef AARCH64 stuff too, but it's always a sign that there
> is something wrong with the front-end to back-end modularization.  We
> can handle the use_XOR_for_compressed_class_base later: we really
> should have a way to communicate with the back ends when the memory
> layout is initialized.  We can go with webrev-3.
>
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

Rahul Raghavan
< Just resending below review request email from Stuart for 8193266
including aarch64-port-dev also. Thanks.>


-- On Saturday 06 January 2018 12:13 AM, Stuart Monteith wrote:
I've removed the AARCH64 conditionals, added the empty line I removed,
and changed the type of "use_XOR_for_compressed_class_base" to bool.

http://cr.openjdk.java.net/~smonteith/8193266/webrev-5/

BR,
     Stuart

> On 4 January 2018 at 14:45, Andrew Haley <[hidden email]> wrote:
>> Hi,
>>
>> On 04/01/18 14:26, [hidden email] wrote:
>>>   I was going to offer to sponsor this since it touches shared code but
>>> I'm not sure I like that there's AARCH64 specific code in
>>> universe.cpp/hpp.   And the name is somewhat offputting, suggesting
>>> implementation details of one target leaking into shared code.
>>>
>>> set_use_XOR_for_compressed_class_base
>>>
>>> I think webrev-3 looked more reasonable, and could elide the #ifdef
>>> AARCH64 in the shared code for that version.   And the indentation is
>>> better.
>>
>> I hate the #ifdef AARCH64 stuff too, but it's always a sign that there
>> is something wrong with the front-end to back-end modularization.  We
>> can handle the use_XOR_for_compressed_class_base later: we really
>> should have a way to communicate with the back ends when the memory
>> layout is initialized.  We can go with webrev-3.
>>
>> --
>> Andrew Haley
>> Java Platform Lead Engineer
>> Red Hat UK Ltd. <https://www.redhat.com>
>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

coleen.phillimore
In reply to this post by Stuart Monteith

This looks good to me.   I will sponsor this fix, once we confirm the
reviewers and you commit the patch and rerun the webrev (so it's in
jdk10.patch).   I see this is for jdk10.

thanks,
Coleen

On 1/5/18 1:43 PM, Stuart Monteith wrote:

> I've removed the AARCH64 conditionals, added the empty line I removed,
> and changed the type of "use_XOR_for_compressed_class_base" to bool.
>
> http://cr.openjdk.java.net/~smonteith/8193266/webrev-5/
>
> BR,
>     Stuart
>
>
> On 4 January 2018 at 14:45, Andrew Haley <[hidden email]> wrote:
>> Hi,
>>
>> On 04/01/18 14:26, [hidden email] wrote:
>>>   I was going to offer to sponsor this since it touches shared code but
>>> I'm not sure I like that there's AARCH64 specific code in
>>> universe.cpp/hpp.   And the name is somewhat offputting, suggesting
>>> implementation details of one target leaking into shared code.
>>>
>>> set_use_XOR_for_compressed_class_base
>>>
>>> I think webrev-3 looked more reasonable, and could elide the #ifdef
>>> AARCH64 in the shared code for that version.   And the indentation is
>>> better.
>> I hate the #ifdef AARCH64 stuff too, but it's always a sign that there
>> is something wrong with the front-end to back-end modularization.  We
>> can handle the use_XOR_for_compressed_class_base later: we really
>> should have a way to communicate with the back ends when the memory
>> layout is initialized.  We can go with webrev-3.
>>
>> --
>> Andrew Haley
>> Java Platform Lead Engineer
>> Red Hat UK Ltd. <https://www.redhat.com>
>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Reply | Threaded
Open this post in threaded view
|

Re: [aarch64-port-dev ] RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

Stuart Monteith
Thanks Coleen,
  So once somebody here manages to review it, I can commit the patch.
I might have this backwards, but should the patch target jdk/jdk or
jdk/jdk10?

Thanks again,
     Stuart


On 11 January 2018 at 12:29,  <[hidden email]> wrote:

>
> This looks good to me.   I will sponsor this fix, once we confirm the
> reviewers and you commit the patch and rerun the webrev (so it's in
> jdk10.patch).   I see this is for jdk10.
>
> thanks,
> Coleen
>
>
> On 1/5/18 1:43 PM, Stuart Monteith wrote:
>>
>> I've removed the AARCH64 conditionals, added the empty line I removed,
>> and changed the type of "use_XOR_for_compressed_class_base" to bool.
>>
>> http://cr.openjdk.java.net/~smonteith/8193266/webrev-5/
>>
>> BR,
>>     Stuart
>>
>>
>> On 4 January 2018 at 14:45, Andrew Haley <[hidden email]> wrote:
>>>
>>> Hi,
>>>
>>> On 04/01/18 14:26, [hidden email] wrote:
>>>>
>>>>   I was going to offer to sponsor this since it touches shared code but
>>>> I'm not sure I like that there's AARCH64 specific code in
>>>> universe.cpp/hpp.   And the name is somewhat offputting, suggesting
>>>> implementation details of one target leaking into shared code.
>>>>
>>>> set_use_XOR_for_compressed_class_base
>>>>
>>>> I think webrev-3 looked more reasonable, and could elide the #ifdef
>>>> AARCH64 in the shared code for that version.   And the indentation is
>>>> better.
>>>
>>> I hate the #ifdef AARCH64 stuff too, but it's always a sign that there
>>> is something wrong with the front-end to back-end modularization.  We
>>> can handle the use_XOR_for_compressed_class_base later: we really
>>> should have a way to communicate with the back ends when the memory
>>> layout is initialized.  We can go with webrev-3.
>>>
>>> --
>>> Andrew Haley
>>> Java Platform Lead Engineer
>>> Red Hat UK Ltd. <https://www.redhat.com>
>>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
>
>
12