Revision3: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments (original) (raw)

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/g1globals.hpp _71 product(double, G1ConcMarkStepDurationMillis, 10.0, _ _72 "Target duration of individual concurrent marking steps " _ _73 "in milliseconds.") _ _74 range(1.0, (double)maxuintx) _ 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/c2globals.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 uintmax. Fixed, though I believe that the value should be actually max_intx.

------------------------------------------------------------------------------ src/share/vm/runtime/globals.hpp 375 static const char* flagerrorstr(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(sizet, 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::vmpagesize(), maxuintx) _

Type is sizet, 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(sizet, MarkStackSizeMax, NOTLP64(4M) LP64ONLY(512M), _ _1847 "Maximum size of marking stack") _ _1848 range(1, (maxjint - 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 applyconstraintandcheckrangeTYPE 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 getstatuserror 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