Revision3: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
Gerard Ziemski
gerard.ziemski at oracle.com
Mon Jun 15 17:26:04 UTC 2015
hi Kim, David,
I am responding to Kim’s comments in-line, but David please see the last comment, which addresses an issue that you have brought up earlier in rev 2:
> On Jun 15, 2015, at 10:41 AM, Gerard Ziemski<gerard.ziemski at oracle.com> wrote:
>
>
>
>
> -------- Forwarded Message --------
> Subject: Re: Revision3: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
> Date: Sat, 13 Jun 2015 16:54:39 -0400
> From: Kim Barrett<kim.barrett at oracle.com>
> To: Gerard Ziemski<gerard.ziemski at oracle.com>
> CC: hotspot-dev<hotspot-dev at openjdk.java.net>
>
> On Jun 13, 2015, at 10:32 AM, Gerard Ziemski<gerard.ziemski at oracle.com>
> wrote:
>> Here is a revision 3 of the feature taking into account feedback from Kim Barrett, David Holmes, Derek White and Bengt Rutisson. I have also merged the changes to the latest jdk9.
[...]
> Here are a few comments on Revision3. I think all of these can (and
> should) be handled via followup CRs, and that none of these should
> prevent Revision3 from being pushed as is.
I think I should spin up rev 4 - please see the last issue here, which
you have raised.
>
> If another round of review for this change does prove to be needed (I
> hope not), please provide an incremental webrev in the review package.
>
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1_globals.hpp
> 71 product(double, G1ConcMarkStepDurationMillis, 10.0, \
> 72 "Target duration of individual concurrent marking steps " \
> 73 "in milliseconds.") \
> 74 range(1.0, (double)max_uintx) \
>
> The new upper bound here seems like a completely random number to me.
>
The existing ad-hoc code just checked the min, and left the upper bound unchecked. Since we need an upper bound for range check, I had to provide some reasonable value and (double)max_uintx seemed a better choice than the mathematically correct DBL_MAX, which would require me to add #import <float.h>
> ------------------------------------------------------------------------------
> src/share/vm/opto/c2_globals.hpp
> 110 notproduct(intx, IndexSetWatch, 0, \
> 111 "Trace all operations on this IndexSet (-1 means all, 0 none)") \
> 112 range(-1, 0) \
>
> I suspect the upper bound here should be uint_max.
>
Fixed, though I believe that the value should be actually max_intx.
> ------------------------------------------------------------------------------
> src/share/vm/runtime/globals.hpp
> 375 static const char* flag_error_str(Flag::Error error) {
>
> All other functions for the Flag are declared in the .hpp and defined
> in the .cpp file. I don't see any reason for this function to be
> different.
>
>
“flag_error_str” is a static method and I simply followed the pattern set by the other static methods (ie. “find_flag”, “fuzzy_match”).
> ------------------------------------------------------------------------------
> src/share/vm/runtime/globals.hpp
> 1563 product(size_t, HeapSizePerGCThread, ScaleForWordSize(64*M), \
> 1564 "Size of heap (bytes) per GC thread used in calculating the " \
> 1565 "number of GC threads") \
> 1566 range((uintx)os::vm_page_size(), max_uintx) \
>
> Type is size_t, but range values are using uintx.
>
The original ad-hoc code used "(uintx)os::vm_page_size()” for the min, but did not check the upper bound, so I provided max_uintx for the max.
Also, HeapSizePerGCThread is assigned to a local variable of type “uintx” and there is no such thing as max_size_t (?)
> ------------------------------------------------------------------------------
> src/share/vm/runtime/globals.hpp
> 1846 product(size_t, MarkStackSizeMax, NOT_LP64(4*M) LP64_ONLY(512*M), \
> 1847 "Maximum size of marking stack") \
> 1848 range(1, (max_jint - 1)) \
>
> That's a very odd looking maximum.
>
>
It comes from the original ad-hoc code.
> ------------------------------------------------------------------------------
> src/share/vm/services/classLoadingService.cpp
> 35 #include "utilities/defaultStream.hpp"
>
> I think utilities/ostream.hpp might be sufficient here.
>
OK.
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
> 100 #define UseConcMarkSweepGCWorkaroundIfNeeded(initial, max) { \
> 101 if ((initial == 7) && (max == 6)) { \
> 102 return Flag::SUCCESS; \
> 103 } \
> 104 }
>
> The do/while(0) macro idiom is the usual way to encapsulate
> statements. It took me a couple of tries to parse this, where it
> would have been immediately obvious with the do/while(0) idiom.
>
>
Can you please provide a concrete example of how I should use "do/while(0)” here? I don't understand how that would help with the readability - we’re talking here about a simple macro that short-circuits a method if the “if” statement passes.
> ------------------------------------------------------------------------------
> src/share/vm/runtime/globals.cpp
>
> In the various apply_constraint_and_check_range_TYPE functions, the
> present pattern seems to be to find and check the range (if it
> exists), then find and apply the constraint (if it exists), and then
> combine the results.
>
> I think it might be better to not apply the constraint if there is a
> range check failure. That way, constraint functions can be written to
> assume that the values have been range checked, and only need to worry
> about whatever additional constraints might exist, without the need to
> potentially deal with out of range values.
>
> Whichever behavior is used should be documented as part of the
> interface to this facility, since it affects how one writes constraint
> functions. It might also, in some cases, affect whether it is useful
> to provide a range spec in conjunction with a constraint function.
>
> Such a change might also eliminate the need for the get_status_error
> helper.
>
> [Obviously this is pretty high priority if handled as a followup
> change, since it has a somewhat significant impact on how one writes
> constraint functions.]
>
I added this behavior to address David’s concern from webrev 2, but to be honest I had doubts about it.
Originally, I had the code checking the ranges first, and only checking the constraints if the range check passed. However, that required me to change "test/runtime/CompressedOops/ObjectAlignment.java” and "/test/runtime/contended/Options.java”, by removing the cases that checked both ranges and constraints, which David pointed out as loosing functionality. The previous ad-hoc code was free to do whatever it wanted and in those 2 cases (ObjectAlignmentInBytes and ContendedPaddingWidth) the test was free to check both the range and constraint at the same time.
So I tried to do the same in my framework, but that made the code a bit more complex (ie. need for “get_status_error” helper).
Most importantly, however, I much prefer the design where the constraint check is only attempted if the range passes (ie. constraint is guaranteed that the value is within the range)
I will revert the code back to what it was like in webrev 2, and these 2 tests will technically loose some functionality, however, there are specific test cases in each of the tests that do exercise both the constraint and the range, though one at the time.
I will rev up to #4 and will come up with rev2 -> rev4 diffs shortly.
Thank you for your patience!
cheers
More information about the hotspot-dev
mailing list