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

[Gerard Ziemski]

hi David,

Thank you for taking the time to review this considerable change. I have 
responded to your points below:


On 5/18/2015 12:28 AM, David Holmes wrote:
>
> A few comments:
>
> For constructs like:
>
> void emit_range_bool(const char* name)            { (void)name; /* NOP 
> */ }
>
> I seem to recall that this was not sufficient to avoid "unused" 
> warnings with some compilers - which is presumably why you did it?

That's indeed the reason why I did. A search on internet reveals that 
this seems to be the preferred way of handling such case, and it works 
for all the currently building JPRT platforms. Is that enough? If there 
is a question of whether this works on some unusual compiler, can we 
leave it up to the the engineers dealing with that compiler, which 
certainly would make them more of experts in such situation than me? If 
not, how would you recommend we handle this differently now?


>
> ---
>
> In src/share/vm/runtime/globals.hpp it says:
>
>   see "checkRanges" function in arguments.cpp
>
> but there is no such function in arguments.cpp

Done.

>
> ---
>
> src/share/vm/runtime/arguments.cpp
>
> The set_object_alignment() functions seems to have some redundant 
> constraint assertions. As does verify_object_alignment() - seems to me 
> that everything in verify_object_alignment should either be in the 
> constraint function for ObjectAlignmentInBytes or one for 
> SurvivorAlignmentInBytes - though the combination of verification and 
> the actual setting of SurvivorAlignmentInBytes may be a problem in the 
> new architecture. If you can't get rid of verify_object_alignment() 
> I'd be tempted to not process it the new way at all, as splitting the 
> constraint checking just leads to confusion IMO.

I moved the code to constraints and in fact this little bit of 
refactoring looks very nice indeed.


>
> The changes to test/runtime/contended/Options.java suggest to me that 
> a constraint is missing on ContendedPaddingWidth - that it is a 
> multiple of 8 (BytesPerLong). That is (still) checked in arguments.cpp 
> (and given it is still checked there is no need to have removed it 
> from the test).

I could argue whether the tests is really loosing a value in displaying 
only one error at the time, but I tried to modify my feature, so that it 
could also detect and report more than one violation at the time, and it 
turned out as doable in these cases, so this is now resolved, with no 
perceived functionality regression.


>
> ---
>
> Some of the test changes, such as:
>
> test/gc/g1/TestStringDeduplicationTools.java
>
> seem to be losing some of what they test. Not only is the test 
> checking the value is detected as erroneous, but it also detects that 
> the user is told in what way it is erroneous. The updated test doesn't 
> validate that part of the argument processing logic

Done, with same fix as above.


>
> ----
>
> There is some inconsistency in the test changes, sometimes you use:
>
>   shouldContain("outside the allowed range")
>
> and sometimes:
>
>   shouldContain("is outside the allowed range")

Done.


cheers