RFR(XS) 8210523 runtime/appcds/cacheObject/DifferentHeapSizes.java crash

[Jiangli Zhou]

Hi Ioi,


On 9/10/18 4:23 PM, Ioi Lam wrote:
>
>
> On 9/10/2018 10:37 AM, Jiangli Zhou wrote:
>> Hi Ioi,
>>
>> The fix looks correct. Please rename 
>> start_address_with_archived_oop_encoding_mode() to 
>> start_address_with_relocated_oop_encoding_mode(). The original name 
>> might be a confusing factor that caused the bug (adding the delta to 
>> the result of start_address_with_archived_oop_encoding_mode()).
>>
>
> start_address_with_archived_oop_encoding_mode() internally calls 
> HeapShared::decode_with_archived_oop_encoding_mode(), so the naming is 
> consistent there. I didn't want to use "relocated ..." because 
> relocation may or may not happen. In many cases, delta == 0.
With the default CDS archive, relocation occurs more often at runtime 
unless smaller java heap size is explicitly specified. 
start_address_with_archived_oop_encoding_mode() does causes confusion. 
It took me a while to refresh my memory and realized it not the 'true' 
dump time encoding. Maybe 
start_address_with_adjusted_archived_oop_encoding_mode()?
>
>
>> Some more comments will be beneficial for readers who are not 
>> familiar with the region relocation code. We should explain that the 
>> first HeapShared::init_narrow_oop_decoding() call sets the relocation 
>> decoding mode and all addresses decoded using it afterwards are 
>> relocated. The second HeapShared::init_narrow_oop_decoding() resets 
>> the relocation decoding mode and adjusts alignment.
>>
>
> Hmm, I could do
>
>   // sets the relocation decoding mode and all addresses decoded using 
> the calculated delta
>   log_info(cds)("CDS heap data relocation delta = " INTX_FORMAT " 
> bytes", delta);
>   HeapShared::init_narrow_oop_decoding(narrow_oop_base() + delta, 
> narrow_oop_shift());
>
> But that doesn't seem to add any info that's not already in the code ....
>
> Commenting about this is tricky, and the involved code is rather small 
> (just FileMapInfo::map_heap_regions_impl and 
> HeapShared::decode_with_archived_oop_encoding_mode()). I feel that 
> writing comments about what it does will just be translating the code 
> into English, which will probably get out of date. I think it should 
> be sufficient to understand the code with the existing comments and 
> the -Xlog:cds output.
>
> What do you think?
The code section is small, but the details are intricate. We can improve 
the comments later also, I'll leave the decision to you.

Thanks,
Jiangli
>
> Thanks
> - Ioi
>
>> Thanks,
>>
>> Jiangi
>>
>> On 9/9/18 4:21 PM, Ioi Lam wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8210523
>>> http://cr.openjdk.java.net/~iklam/jdk12/8210523-DifferentHeapSizes-crash.v01/ 
>>>
>>>
>>> The bug is here:
>>>
>>>    address relocated_strings_bottom = 
>>> start_address_with_archived_oop_encoding_mode(si);
>>> -  if (!is_aligned(relocated_strings_bottom + delta, 
>>> HeapRegion::GrainBytes)) {
>>> +  if (!is_aligned(relocated_strings_bottom, HeapRegion::GrainBytes)) {
>>>
>>> relocated_strings_bottom was already relocated, so you don't need 
>>> the "+ delta"
>>> here. The bug was hidden because in all of our test runs, delta had 
>>> been
>>> a multiple of HeapRegion::GrainBytes, so that didn't affect the 
>>> result of the
>>> is_align() call.
>>>
>>> Then, just by chance (probably due to Address Space Layout 
>>> Randomization
>>> on MacOS), we have a delta / GrainBytes == (29509025792 / 4194304.0) 
>>> = 7035.5,
>>> so the bug is uncovered.
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>
>