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

[Kim Barrett]

> On Sep 19, 2018, at 12:14 PM, Gerard Ziemski <gerard.ziemski at oracle.com> wrote:
> 
> Thank you for the review!
> 
>> On Sep 15, 2018, at 4:11 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>> 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.

I *think* you wrote most of this code; I don’t know how much input there was from the compiler
team in that though.  OK for the revised version.

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

The format changes are okay, but the actual constraint checking code
has multiple problems.  Those are out of scope for this change, and
should be reported as new bugs.  I filed:
https://bugs.openjdk.java.net/browse/JDK-8211034

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

Good.  Thanks for checking.

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

BTW, the correct URL has “_rev2”, e.g.
http://cr.openjdk.java.net/~gziemski/8204294_rev2