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

[coleen.phillimore at oracle.com]

Gerard, This looks good.
Thanks!
Coleen

On 9/21/18 5:23 PM, Kim Barrett wrote:
>> 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
>
>