RFR: 8193266: AArch64: TestOptionsWithRanges.java SIGSEGV

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

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

jesper.wilhelmsson


> On 12 Jan 2018, at 15:25, Stuart Monteith <[hidden email]> wrote:
>
> 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?

If it's for JDK 10 it should be pushed to jdk/jdk10
/Jesper

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

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 Rahul Raghavan
Hello,
        Would it be possible for this to be reviewed? I'd like to get it in for
JDK11. I don't believe there were any outstanding issues.

I've updated and reapplied the patch against current jdk/hs

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

Thanks,
    Stuart


On 11/01/18 08:20, Rahul Raghavan wrote:

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

Andrew Haley
On 03/19/2018 07:34 AM, Stuart Monteith wrote:
> Would it be possible for this to be reviewed? I'd like to get it in for
> JDK11. I don't believe there were any outstanding issues.
>
> I've updated and reapplied the patch against current jdk/hs
>
> http://cr.openjdk.java.net/~smonteith/8193266/webrev-6/

Yes, thank you.

I'm sorry for the delay: I lost track of the conversation.

--
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
In reply to this post by Stuart Monteith
Stuart,

Changes looks good to me.

-Dmitry

On 19.03.2018 10:34, Stuart Monteith wrote:

> Hello,
> Would it be possible for this to be reviewed? I'd like to get it in for
> JDK11. I don't believe there were any outstanding issues.
>
> I've updated and reapplied the patch against current jdk/hs
>
> http://cr.openjdk.java.net/~smonteith/8193266/webrev-6/
>
> Thanks,
>     Stuart
>
>
> On 11/01/18 08:20, Rahul Raghavan wrote:
>> < 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


--
Dmitry Samersoff
http://devnull.samersoff.net
* There will come soft rains ...

Reply | Threaded
Open this post in threaded view
|

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

Stuart Monteith
Thank you for everyone's attention. I've put the updated patch here:

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

I've added Andrew Haley and Dmitry Samersoff as the reviewer.

BR,
   Stuart

On 19 March 2018 at 13:51, Dmitry Samersoff <[hidden email]> wrote:

> Stuart,
>
> Changes looks good to me.
>
> -Dmitry
>
> On 19.03.2018 10:34, Stuart Monteith wrote:
>> Hello,
>>       Would it be possible for this to be reviewed? I'd like to get it in for
>> JDK11. I don't believe there were any outstanding issues.
>>
>> I've updated and reapplied the patch against current jdk/hs
>>
>> http://cr.openjdk.java.net/~smonteith/8193266/webrev-6/
>>
>> Thanks,
>>     Stuart
>>
>>
>> On 11/01/18 08:20, Rahul Raghavan wrote:
>>> < 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
>
>
> --
> Dmitry Samersoff
> http://devnull.samersoff.net
> * There will come soft rains ...
>
Reply | Threaded
Open this post in threaded view
|

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

Yangfei (Felix)
Hi,

  Looks like the " Summary:" entry is missing in your changeset.  
  I have created a new changeset adding the this entry and pushed.  

Thanks,
Felix


>
> Thank you for everyone's attention. I've put the updated patch here:
>
> http://cr.openjdk.java.net/~smonteith/8193266/webrev-7/
>
> I've added Andrew Haley and Dmitry Samersoff as the reviewer.
>

12