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

[Gerard Ziemski]

hi Alexander,

Yes, handling it this way was an alternative, but I thought it required 
a C++ compiler with some specific feature support?  C++ 11?

It was my understanding that such solution was more restrictive, than 
the one I used.

Thank you.

On 5/21/2015 11:26 AM, Alexander Harlap wrote:
> And what about this:
>
> void emit_range_bool(const char* /* name */)            { }
>
> Alex
>
> On 5/21/2015 12:09 PM, Gerard Ziemski wrote:
>> 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
>>
>
>
>