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

[Ioi Lam]

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.


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

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