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

[Kim Barrett]

On Jun 15, 2015, at 1:26 PM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
> 
>> 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.

Even after reading your discussion below, my preference would be to separate that
issue out and deal with it as a followup.  The size of this change set is causing me
to glaze over and not feel like I’m doing an effective review any more.

>> 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>

My opinion: <float.h> is just not big enough to be seriously concerned about.
Using something that has some meaning and is not a very nearly randomly
chosen value is more important.  If we really want to avoid <float.h>, pick a
stupidly large but “meaningful” upper bound like an hour (or 24 hours, or… )
or 10000 seconds or some such.  Note that any change from DBL_MAX is
at least theoretically a user-impacting change, so ought to go through the
process for that.  (Maybe that will push you to <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.

Right, sorry about the type mismatch.

>> 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”).

find_flag and fuzzy_match are declared in the .hpp and defined in the .cpp.

>> 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 (?)

The code I’m looking at (hs-rt tip) uses (size_t)os::vm_page_size() for the min.
That change was relatively recent:
8074459: Flags handling memory sizes should be of type size_t

Maybe an incorrect “merge” with that change set in your changes?

>> 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.

That doesn’t make it any less odd looking.  I think it is worth an RFE to understand
that value and add a comment explaining it (or changing it to something less
surprising, if that’s warranted).  Or at least a comment questioning the value.
Not that I think anyone’s going to look at it anytime soon.

>> 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.

The expansion presently wraps the “if” statement in an extra level of braces, to avoid
leaving a dangling if.  That’s good, but I found it non-obvious, perhaps in part due to
the formatting.  But the “usual idiom” for that sort of thing is the do/while(0) idiom, e.g

#define UseConcMarkSweepGCWorkaroundIfNeeded(initial, max) \
  do { \
    if (initial == 7) && (max == 6)) { \
      return Flag::SUCCESS; \
    } \
  } while (0)

If written that way, it would have been immediately obvious (to me) what was
being done, since do/while(0) is a "well-known” and easily recognized pattern
chunk.

>> 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.

Sorry, I overlooked or forgot the earlier discussion in this area.

> 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.

My opinion: If there is some (possible) contention over this area, I think that’s all the more
reason at this point to address it separately, as long as we do so soon.  Having the whole
change lingering for this important but narrow piece seems less than ideal.