RFR: 8277854: The upper bound of GCCardSizeInBytes should be limited to 512 for 32-bit platforms

[Jie Fu]

On Sat, 27 Nov 2021 00:47:55 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Good questions!
>> Thanks @Hamlin-Li for your review.
>> 
>> 
>>> Several initial comments:
>>> 
>>> * Not sure if the lower bound should also be distinguished between 32 and 64. It's not because of crash, but as you're touching it, should we consider this?
>> 
>> Not sure if there is any benefit to distinguish the lower bounds between 32-bit and 64-bit platforms.
>> But this pr aims at fixing the crash caused by the incorrect upper bound on 32-bit platforms.
>> So a separate pr seems better if the lower bound needs to be re-considered.
>> 
>>> * Does a CSR needed?
>> 
>> Maybe not since no VM flags are added or removed and jdk18 hasn't been released yet.
>> But I'm not sure.
>> 
>>> * A regression test would be helpful.
>> 
>> There is already a jtreg test runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java for this regression.
>> 
>> Thanks.
>> Best regards,
>> Jie
>
>> > * Not sure if the lower bound should also be distinguished between 32 and 64. It's not because of crash, but as you're touching it, should we consider this?
>> 
>> Not sure if there is any benefit to distinguish the lower bounds between 32-bit and 64-bit platforms. But this pr aims at fixing the crash caused by the incorrect upper bound on 32-bit platforms. So a separate pr seems better if the lower bound needs to be re-considered.
> 
> I'm fine with it.
> 
>> 
>> > * Does a CSR needed?
>> 
>> Maybe not since no VM flags are added or removed and jdk18 hasn't been released yet. But I'm not sure.
> 
> Please follow Thomas's suggestion.
> 
>> 
>> > * A regression test would be helpful.
>> 
>> There is already a jtreg test runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java for this regression.
>> 
> 
> I was wondering why it's not caught by regionion test until I saw Thomas' response.
> 
> Thanks for clarifying and fixing this.

Thanks @Hamlin-Li for your review.

-------------

PR: https://git.openjdk.java.net/jdk/pull/6569