RFR(S) 8204294: [REDO] - JVMFlag::printError missing ATTRIBUTE_PRINTF

[Gerard Ziemski]

Thank you for the review!

> On Sep 15, 2018, at 4:11 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
>> On Sep 14, 2018, at 1:40 PM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
>> 
>> Hi all,
>> 
>> Please review this redo of a simple fix that adds ATTRIBUTE_PRINTF to "JVMFlag::printError” and also fixes the resulting mismatch format errors that it triggers (as it is supposed to).
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8204294
>> Webrev: http://cr.openjdk.java.net/~gziemski/8204294_rev1/index.html
>> Testing: Mach5 hs_tier1,2,3,4,5,6 (completed successfully)
>> 
>> 
>> Cheers
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/cms/jvmFlagConstraintsCMS.cpp
> 153                           "a multiple of " INT32_FORMAT "\n",
> 
> HeapWordSize is of type int.  Use %d directly, rather than
> INT32_FORMAT.

Fixed.


> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp
>  97                         "between 0 and %d\n",
>  98                         AllocatePrefetchDistance, 512);
> 
> Why is this hard-coding the 0 but not 512 in the string.  Or better,
> make 512 a named constant and replace both uses here.
> 
> Or even better, why is this a constraint function at all?  Couldn't
> this apparently platform-generic restriction just be a range
> constraint, e.g. range(-1, 512)?

The compiler related flags are handled by the compiler team, so I’d prefer to just fix the warning/error and leave any further improvements to the code to them.


> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp
> 147 JVMFlag::Error OnStackReplacePercentageConstraintFunc(intx value, bool verbose) {
> 
> The clauses being changed are only reachable via a narrowing
> conversion, which has implementation-defined value.  Fixing that is
> probably out of scope for this change.

Without the fix we get warning/error, so we need to do something here - how do we proceed then?


> ------------------------------------------------------------------------------
> src/hotspot/share/runtime/flags/jvmFlagConstraintsCompiler.cpp
> 398                         "RTMTotalCountIncrRate (" INT32_FORMAT
> 
> RTMTotalCountIncrRate is of type int.  Use %d directly, rather than
> INT32_FORMAT.

Fixed.

> 
> ------------------------------------------------------------------------------
> 
> Have you checked for uses of printError() in code specific to platforms
> not tested by Oracle?

The only code using it is in hotspot/share/runtime/flags/* and hotspot/share/gc* 
It doesn’t seem to exist in platform specific code, but it was a good idea to check.


Webrev: http://cr.openjdk.java.net/~gziemski/8204294_rev1/index.html
Testing: Mach5 hs_tier1,2,3,4,5,6 (running...)


cheers