RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS
Dmitry Dmitriev
dmitry.dmitriev at oracle.com
Fri Jul 10 11:36:43 UTC 2015
Hello Jeremy,
Thank you for fixing that! But in this case we still have minor issue
when quotes processing failed(lines 3438-3444). Here is my suggestion:
leave 'while' cycle(lines 3423-3462) as I was in your previous
webrev(without 'start' and etc.) and call strdup when copy 'options' to
'options_arr' on lines 3472-3474, i.e. make lines 3472-3474 like this:
3472 for (int i = 0; i < options->length(); i++) {
3473 options_arr[i] = options->at(i);
3474 options_arr[i].optionString = os::strdup(options_arr[i].optionString);
3475 }
What you think about that?
Regards,
Dmitry
On 10.07.2015 3:23, Jeremy Manson wrote:
> Hey Dmitry,
>
> Thanks for looking at it. The leak was deliberate (under the theory
> that the command line flags were leaked, too), but it was silly that I
> didn't fix it.
>
> Anyway, fixed. See:
>
> http://cr.openjdk.java.net/~jmanson/8079301/webrev.03
> <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.03>
>
> Jeremy
>
> On Thu, Jul 9, 2015 at 2:52 PM, Dmitry Dmitriev
> <dmitry.dmitriev at oracle.com <mailto:dmitry.dmitriev at oracle.com>> wrote:
>
> Hello Jeremy,
>
> Correct me if I am wrong, but I see small memory leak in your
> implementation.
> In Arguments::parse_options_environment_variable function memory
> for 'buffer' allocated by strdup function on line 3415, but now it
> not freed in this function because data from this buffer is used
> later. On the other hand when we are done with env variables, we
> free only options array(lines 3703-3705) and not free previously
> allocated memory for 'buffer' because we lost track for it.
>
> Thanks,
> Dmitry
>
>
> On 09.07.2015 21:20, Jeremy Manson wrote:
>
> Thanks for looking at this, Coleen.
>
> I merged this with the latest version of hs-rt, but Ron is or
> I am going to
> have to do some merging for 8061999. The changes are relatively
> orthogonal, but they touch a couple of the same places. How
> far away is
> Ron's checkin?
>
> Updated webrev:
>
> http://cr.openjdk.java.net/~jmanson/8079301/webrev.01/
> <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.01/>
>
> Updated test:
>
> http://cr.openjdk.java.net/~jmanson/8079301t/webrev.01/
> <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.01/>
>
> (Mild complaining on my part: the conflicts wouldn't have been
> an issue if
> someone had looked at this two months ago, when I first
> asked. I suspect
> Ron hadn't even started his change at that point. And if external
> developers were able to submit changes to Hotspot, we wouldn't
> have had to
> bother Oracle engineers at all. Neither of these is your
> fault or your
> problem, of course - I'm grateful you were able to take a look.)
>
> Jeremy
>
> On Wed, Jul 8, 2015 at 6:19 AM, Coleen Phillimore <
> coleen.phillimore at oracle.com
> <mailto:coleen.phillimore at oracle.com>> wrote:
>
> I'll sponsor it but can you repost the RFR?
>
> There has been a fair bit of work to command line
> processing lately so I'd
> like to have the others look at it too. Have you merged
> this up with the
> latest version of hs-rt repository and were there
> conflicts? Also, have
> you looked at the RFR:
>
> "Open code review for 8061999 Enhance VM option parsing to
> allow options
> to be specified"
>
> and are there conflicts with this?
>
> The hotspot change looks good to me.
>
>
> http://cr.openjdk.java.net/~jmanson/8079301t/webrev.00/test/com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java.udiff.html
> <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.00/test/com/sun/management/HotSpotDiagnosticMXBean/CheckOrigin.java.udiff.html>
>
> "UseCerealGC" LOL
> Can you use a different option than PrintOopAddress in
> this test? I don't
> know why this command line option exists or would be
> useful for, and seems
> to be a good candidate for removal. If you need a valid
> argument, that is
> unlikely to go away, TraceExceptions would be good.
>
> Thanks,
> Coleen
>
>
> On 7/7/15 8:00 PM, Jeremy Manson wrote:
>
> No love? Is there a code owner here I should pester
> more directly?
>
> Jeremy
>
> On Fri, Jun 26, 2015 at 3:00 PM, Martin Buchholz
> <martinrb at google.com <mailto:martinrb at google.com>>
> wrote:
>
> As usual with Google patches, they are in use
> locally. This one has been
>
> stable for a while without complaints. Please
> sponsor.
>
> On Fri, Jun 26, 2015 at 1:53 PM, Jeremy Manson
> <jeremymanson at google.com
> <mailto:jeremymanson at google.com>>
> wrote:
>
> All this talk about IgnoreUnrecognizedVMOptions
> reminded me that this
>
> review is still outstanding. Any takers?
>
> Jeremy
>
> On Tue, May 5, 2015 at 10:01 AM, Jeremy Manson
> <jeremymanson at google.com
> <mailto:jeremymanson at google.com>
> wrote:
>
> Hi David,
>
> Thanks for taking a look.
>
> On Mon, May 4, 2015 at 8:32 PM, David
> Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>>
> wrote:
>
> Hi Jeremy,
>
> On 5/05/2015 6:51 AM, Jeremy Manson wrote:
>
> Not sure who might be willing to
> sponsor this. David? You did the
>
> last
> one. :)
>
> Might be a while before I can
> get to it. I did have a quick look
>
> (and
> will need a longer one).
>
> I understand. I'm just happy it's
> possible to upstream this stuff at
> all[1].
>
> [1] With the eternal caveat that I'd be
> happier if we didn't *have* to
> go through you folks, but we've learned to
> live with it.
>
>
>
> Context: A number of command line
> options are not settable via
>
> JAVA_TOOL_OPTIONS and _JAVA_OPTIONS:
>
> - PrintVMOptions
>
> So you have changed the
> semantics of this to print out the
> options
>
> from
> the command-line and each of the two
> env vars. Seems reasonable but
> may be
> better to know which option came from
> where as we can now see the same
> option (or conflicting variants
> thereof) multiple times.
>
> I'd be happy to do this. Any
> preferred syntax? Does someone
>
> actually
> own this feature?
>
> I'm unclear what the current processing
> order is for the different
>
> sources of options, and whether the
> changes affect that order?
>
> Nope. I go through sad contortions
> to keep the processing order the
>
> same. It's JAVA_TOOL_OPTIONS, then
> command line, then _JAVA_OPTIONS.
> See
> lines 2549-2567.
>
>
> - IgnoreUnrecognizedVMOptions
>
> - PrintFlagsInitial
>
> Unclear what "initial" actually
> means - is it the default?
>
> More or less. If you stick, for example,
> IgnoreUnrecognizedVMOptions
> in
> there first, it will get printed out as
> "true". I haven't really
> changed
> the semantics here either - I've just
> added processing of the envvars.
>
> - NativeMemoryTracking
>
> - PrintFlagsWithComments
>
> This is because these flags have
> to be processed before processing
> other
> flags, and no one ever bothered to
> do that for the flags coming from
> environment variables. This patch
> fixes that problem.
>
> I have a test, but it is a
> modification to a JDK test instead
> of a HS
> test,
> so it can go in separately (I guess).
>
> They can be pushed together to
> different repos in the same forest.
>
> Okay. Here's the test change:
>
> http://cr.openjdk.java.net/~jmanson/8079301t/webrev.00/
> <http://cr.openjdk.java.net/%7Ejmanson/8079301t/webrev.00/>
>
>
> As I said I'll have to get back to this.
> And hopefully someone else in
>
> runtime will take a good look as well ;-)
>
> I'd be happy to toss it over to
> whomever owns this, if anyone.
>
> Thanks!
>
> Jeremy
>
>
>
> Bug:
>
> https://bugs.openjdk.java.net/browse/JDK-8079301
>
> Webrev:
> http://cr.openjdk.java.net/~jmanson/8079301/webrev.00/
> <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.00/>
>
> Jeremy
>
>
>
>
>
More information about the hotspot-runtime-dev
mailing list