RFR 8130459(M): Add additional validation after heap creation
sangheon.kim
sangheon.kim at oracle.com
Sat Jul 25 02:11:55 UTC 2015
On 07/24/2015 06:10 PM, Kim Barrett wrote:
> On Jul 24, 2015, at 8:49 PM, sangheon.kim <sangheon.kim at oracle.com> wrote:
>> Hi Kim,
>>
>> On 07/24/2015 04:25 PM, Kim Barrett wrote:
>>> On Jul 23, 2015, at 8:53 PM, sangheon.kim <sangheon.kim at oracle.com> wrote:
>>>> Hi Kim,
>>>>
>>>> Updated webrev includes:
>>>> - Moved functions related to range/constraints from CommandLineFlags to CommandLineFlagConstraintList / CommandLineFlagRangeList.
>>>> - 2 functions are changed to 'const'.
>>>> - 2 typos.
>>>>
>>>> webrev.04:
>>>> http://cr.openjdk.java.net/~sangheki/8130459/webrev.04
>>>>
>>>> Incremental:
>>>> http://cr.openjdk.java.net/~sangheki/8130459/webrev.04_to_03
>>> ------------------------------------------------------------------------------
>>> src/share/vm/runtime/commandLineFlagConstraintList.cpp
>>> 343 // Skip if we already checked.
>>> 344 if (type < _validating_type) {
>>> 345 return true;
>>> 346 }
>>>
>>> That's not quite what I had in mind when I suggested the type should
>>> be verified to be less than _validating_type. I think it's a program
>>> error for that test to fail, e.g. it should be checked with a
>>> assert/guarantee (I would use guarantee). For example, if we were to
>>> (presumably unintentionally) perform constraint checking out of order
>>> then the out of order check would simply not be performed - ever -
>>> with the code above.
>> I see.
>>
>> How about below?
>> // First check will be for 'AfterErgo' and initial value of '_validating_type' is 'AtParse'.
>> guarantee(type > _validating_type, "Constraint check is out of order.");
>> _validating_type = type;
> Yes, that’s what I had in mind.
Okay, I changed like above.
>
>>> src/share/vm/runtime/thread.cpp
>>> 3333 bool constraint_result = CommandLineFlagConstraintList::check_constraints(CommandLineFlagConstraint::AfterErgo);
>>> 3334 Arguments::post_after_ergo_constraint_check(constraint_result);
>>> 3335 if (!constraint_result) {
>>> 3336 return JNI_EINVAL;
>>> 3337 }
>>>
>>> Simpler would be
>>>
>>> if (!CommandLineFlagConstraintList::check_constraints(CommandLineFlagConstraint::AfterErgo)) {
>>> return JNI_EINVAL;
>>> }
>>> Arguments::post_after_ergo_constraint_check();
>>>
>>> with associated change of post_after_ergo_constraint_check to
>>> eliminate the unused(!) argument.
>> I don't have strong opinion on this as it is unused for now.
>> However as you know its intend is to leave a way to utilize the result of constraint check for future use.
>> Still we will have to cover runtime/gc/compiler team's flags. :)
> We can change things later if there’s a need to do so.
>
> However, in this case, I don’t think such a need will arise. If the constraint check fails,
> we should be heading toward the exit, and shouldn’t be doing post-check stuff anyway.
Okay, I changed as you suggested.
webrev.05
http://cr.openjdk.java.net/~sangheki/8130459/webrev.05/
webrev.05_to_04
http://cr.openjdk.java.net/~sangheki/8130459/webrev.05_to_04/
Thanks,
Sangheon
>
More information about the hotspot-dev
mailing list