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

Gerard Ziemski gerard.ziemski at oracle.com
Mon Jun 1 21:29:48 UTC 2015

Thank you David for more feedback. My answers are provided inline:

Subject: Re: Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments Date: Fri, 29 May 2015 14:39:11 +1000 From: David Holmes<david.holmes at oracle.com> Organization: Oracle Corporation To: Gerard Ziemski<gerard.ziemski at oracle.com>,hotspot-dev at openjdk.java.net Developers<hotspot-dev at openjdk.java.net> CC: Coleen Phillimore<coleen.phillimore at oracle.com>, Dmitry Dmitriev<dmitry.dmitriev at oracle.com>, Kim Barrett<kim.barrett at oracle.com>, Alexander Harlap<alexander.harlap at oracle.com>

Hi Gerard, Meta-comment: I had expected to see constraint functions subsume range checks - ie that the range check would be folded into the constraints function so that all the constraints are clearly seen in one place. Not sure splitting things across two checks aids in understandability. This isn't a blocker but I'd be interested to hear other views on this. Keeping ranges separate from constraints allows us to easily print them out on demand - a requirement from SQA point of view (see PrintFlagsRanges)

A general comment, note that in cases like:

"intx %s="INTXFORMAT" is outside the allowed range [ "INTXFORMAT" ... "INTXFORMAT" ]\n", we now need to add spaces between the macros like INTXFRORMAT and the double-quotes - see 8081202. Not sure who will be pushing first but it's an easy fix to make.

Done.

A few minor specific comments:

src/share/vm/runtime/commandLineFlagConstraintsCompiler.hpp Has the comment: 32 * Here we have runtime arguments constraints functions, - should say 'compiler' Done.

---

src/share/vm/runtime/commandLineFlagConstraintsCompiler.cpp Not obvious all the includes are needed ie java.hpp and os.hpp

Done.

--- src/share/vm/runtime/commandLineFlagConstraintsGC.hpp Has the comment: 32 * Here we have runtime arguments constraints functions, - should say 'GC' I'm a little surprised the #if INCLUDEALLGCS only covers the G1 options. I guess we don't as aggressively exclude the code for the other GC's.

Done.

--- src/share/vm/runtime/commandLineFlagConstraintsGC.cpp

Not obvious all the includes are needed ie java.hpp and os.hpp, and c1/c2 globals?

Done.

---

Nothing else jumped out at me, but I didn't verify the non-runtime constraints.

Thank you - I will be posting up a new webrev soon.

cheers

More information about the hotspot-dev mailing list