[aarch64-port-dev ] RFR: 8234794: AArch64: runtime/memory/ReadFromNoaccessArea.java crashes

[Ioi Lam]

Hi Nick, sorry for the delay. The changes look good to me.

I am running test tiers 1/2 with our CI pipeline and will report the 
results back.

Thanks
- Ioi

On 12/16/19 10:25 PM, Nick Gasson wrote:
> Hi,
>
> Any comments on the most recent webrev below?
>
> Thanks,
> Nick
>
> On 12/12/2019 16:31, Nick Gasson wrote:
>> On 12/12/2019 04:16, Andrew Haley wrote:
>>> On 12/11/19 7:08 AM, Nick Gasson wrote:
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8234794
>>>> Webrev: http://cr.openjdk.java.net/~ngasson/8234794/webrev.01/
>>>
>>> Thanks. A few nits:
>>
>> Please see the updated webrev:
>>
>> http://cr.openjdk.java.net/~ngasson/8234794/webrev.02/
>>
>>>
>>> This recalculates klass_decode_mode() a thousand times on only a small
>>> program. Be aware that C2 creates a new Assembler instance for most 
>>> patterns.
>>>
>>> _klass_decode_mode could be calculated only once. Make it static and it
>>> will be.
>>
>> Yes. Also added an assert that Metaspace::initialized() in case we 
>> call it too early.
>>
>>>
>>> This inverted logic:
>>>
>>> #if !(defined(AARCH64) || defined(AIX))
>>>    return ReservedSpace(size, alignment, large_pages, requested_addr);
>>> #else // AARCH64 || AIX
>>>
>>> looks a bit cumbersome. It might be nice to do it the other way up, so
>>> that reserve_preferred_space is called from a static generic allocate
>>> method.
>>>
>>> Rather than explicitly defined(AARCH64) || defined(AIX) you might 
>>> like to
>>> consider a named macro
>>>
>>> #if OS_CPU_USES_PREFERRED_SPACE
>>>    return reserve_preferred_space( ...
>>> endif
>>>
>>
>> OK. Metaspace::reserve_preferred_space() is now only defined if 
>> PREFERRED_METASPACE_ALIGNMENT is set. The generic method is 
>> Metaspace::reserve_space(). MetaspaceShared::reserve_shared_space() 
>> is now just a trivial wrapper for this so maybe it can be removed.
>>
>> I put the definition in the CPU's globalDefinitions.hpp because we 
>> don't have one for os_cpu, and we need it for every AArch64 system 
>> anyway.
>>
>> Also fixed the break/return inconsistency suggested by Jiangli.
>>
>>
>> Thanks,
>> Nick