RFR(s): 8152182: Possible overflow in initialzation of _rescan_task_size and _marking_task_size

[Jesper Wilhelmsson]

Latest changes looks good to me as well.
/Jesper

Den 7/4/16 kl. 23:31, skrev Jon Masamitsu:
>
>
> On 04/07/2016 11:22 AM, sangheon wrote:
>> Hi Jon,
>>
>> On 04/07/2016 10:36 AM, Jon Masamitsu wrote:
>>> Sangheon,
>>>
>>> Thanks for the changes. They look good.  A couple
>>> of minor suggestions below.
>>>
>>> Jon
>>>
>>> On 04/06/2016 09:29 PM, sangheon wrote:
>>>> Hi Jon,
>>>>
>>>> Thank you for the review.
>>>>
>>>> On 04/05/2016 05:02 PM, Jon Masamitsu wrote:
>>>>> Sangheon,
>>>>>
>>>>> It occurs to me that if you created a function in CMS code
>>>>> check_rescan_task_size_alignment()
>>>>> to do the check
>>>>>
>>>>>> 506 const size_t rescan_task_size = cms->cmsSpace()->rescan_task_size();
>>>>>> 507 const size_t alignment = CardTableModRefBS::card_size * BitsPerWord;
>>>>>> 508
>>>>>> 509 if ((size_t)round_to((intptr_t)rescan_task_size, alignment) !=
>>>>>> rescan_task_size) {
>>>>>
>>>>> then you won't have to expose the particular alignment requirements in the
>>>>> constraint function.  The code could look more like const int
>>>>> HeapWordSize        = sizeof(HeapWord);
>>>>>
>>>>>
>>>>> if ( check_rescan_task_size_alignment() ) {
>>>>>
>>>>> 513 CommandLineError::print(verbose,
>>>>> 514 "Rescan task size (" SIZE_FORMAT " = CMSRescanMultiple * "
>>>>> 515 "CardTableModRefBS::card_size_in_words * BitsPerWord) must be "
>>>>> 516 "aligned to CardTableModRefBS::card_size * BitsPerWord (" SIZE_FORMAT
>>>>> "). "
>>>>> 517 "Round-down value for CMSRescanMultiple is " SIZE_FORMAT "\n",
>>>>> 518 rescan_task_size, alignment, round_down_value);
>>>>> 519 status = Flag::VIOLATES_CONSTRAINT;
>>>>>
>>>>> }
>>>>>
>>>>>
>>>>> You might be able to do something similar in the other constraint function.
>>>> Your suggestion especially not exposing the alignment inspired me to make it
>>>> simpler. :)
>>>> Task size is 'flag *CardTableModRefBS::card_size_in_words * BitsPerWord' and
>>>> we have to check the alignment with CardTableModRefBS::card_size *
>>>> BitsPerWord. As CardTableModRefBS::card_size_in_words =
>>>> CardTableModRefBS::card_size / sizeof(HeapWord), if these flags are a
>>>> multiple of sizeof(HeapWord), the task size will be aligned.
>>>>
>>>>>
>>>>> Also for
>>>>>
>>>>> 472 if (value > addr_ergo_max) {
>>>>> 473 CommandLineError::print(verbose,
>>>>> 474 "%s (" SIZE_FORMAT ") must be "
>>>>> 475 "less than or equal to ergonomic maximum (" SIZE_FORMAT ") "
>>>>> 476 "based on start address corresponds to the old generation of the Java
>>>>> heap\n",
>>>>> 477 name, value, addr_ergo_max);
>>>>> 478 return Flag::VIOLATES_CONSTRAINT;
>>>>> 479 }
>>>>>
>>>>> Printing the addr_ergo_max might be confusing to the  users since it will
>>>>> be a very large
>>>>> number.  Not sure what  to do unless you can print a maximum value of the
>>>>> flag based
>>>>> on the maximum heap size (instead of based on MAX_SIZE).
>>>> I agree that using max heap will be better, so I fixed.
>>>> Considering your comment above (not exposing alignment information as it is
>>>> not nice), I had to add a new method to get ergonomic maximum values for
>>>> these task sizes. We can check their constraint without this function, but
>>>> can't print out ergo max. Or have to use
>>>> CardTableModRefBS::card_size_in_words * BitsPerWord on its calculation.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~sangheki/8152182/webrev.02
>>>> http://cr.openjdk.java.net/~sangheki/8152182/webrev.02_to_01
>>> http://cr.openjdk.java.net/~sangheki/8152182/webrev.02_to_01/src/share/vm/runtime/commandLineFlagConstraintsGC.cpp.frames.html
>>>
>>> 469 "based on reserved area corresponds to the old generation of the Java
>>> heap\n",
>>> "reserved area" is correct but perhaps not understandable for the average
>>> users. Maybe something like "which is based on the maximum size of the old
>>> generation of the Java heap\n"
>> Fixed.
>>
>>>
>>> 492 value, sizeof(HeapWord)); From share/vm/utilities/globalDefinitions.hpp
>>> you could use HeapWorkSize const int HeapWordSize = sizeof(HeapWord);
>> I just repeated to use where it was calculated but I like is better.
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~sangheki/8152182/webrev.03
>> http://cr.openjdk.java.net/~sangheki/8152182/webrev.03_to_02
>
> Looks good.  Thanks. for the changes.
>
> Jon
>>
>> Thanks,
>> Sangheon
>>
>>
>>> Jon
>>>>
>>>> Testing: JPRT, runtime/commandline JTREG on all platforms.
>>>>
>>>> Thanks,
>>>> Sangheon
>>>>
>>>>
>>>>>
>>>>> Ask if that's not clear.
>>>>>
>>>>> Jon
>>>>>
>>>>>
>>>>>
>>>>> On 4/5/2016 10:24 AM, sangheon wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Please review this change for CMSRescanMultiple and CMSConcMarkMultiple
>>>>>> flags.
>>>>>>
>>>>>> Both flags are set by "CardTableModRefBS::card_size_in_words * BitsPerWord
>>>>>> * flag" which potentially would make an overflow with their maximum value
>>>>>> without setting range. And these flags also would make an arithmetic
>>>>>> overflow when calculating with the size and the start address of reserved
>>>>>> area. In addition, CMSRescanMultiple needs an alignment check.
>>>>>>
>>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8152182
>>>>>> Webrev: http://cr.openjdk.java.net/~sangheki/8152182/webrev.00
>>>>>> Testing: JPRT, runtime/commandline JTREG for all platforms
>>>>>>
>>>>>> Thanks,
>>>>>> Sangheon
>>>>>
>>>>
>>>
>>
>