RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS
Dmitry Dmitriev
dmitry.dmitriev at oracle.com
Fri Jul 10 18:40:04 UTC 2015
Jeremy, thank you for fixing that! Please, see my comments inline for 2
item and one more comment.
On 10.07.2015 20:15, Jeremy Manson wrote:
> Thanks for the detailed look, Dmitry. I have taken your advice, with
> a couple of exceptions:
>
> 1) I named the method free_memory_for_vm_init_args instead
> of free_memory_for_environment_variables.
> 2) I did not free the memory in 3661-3663 or in 3679-3681, because
> those are error paths where the memory may not have been allocated in
> the first place. So freeing it might lead to a crash.
Yes, you are right. But I think we can guard from this by adding check
for args->options in new free_memory_for_vm_init_args function and free
memory in all execution paths. If args->options is not NULL, then memory
is allocated and must be freed:
3485 static void free_memory_for_vm_init_args(JavaVMInitArgs* args) {
3486 if( args->options != NULL ) {
3487 for (int i = 0; i < args->nOptions; i++) {
3488 os::free(args->options[i].optionString);
3489 }
3490 FREE_C_HEAP_ARRAY(JavaVMOption, args->options);
3491 }
3492 }
As I see you add free memory for error paths at lines(webrev.04)
3668-3671 and 3687-3691. Thus, with that guard you can add two calls to
free_memory_for_vm_init_args function before "return JNI_EINVAL;" on
line 3696:
3693 #ifdef ASSERT
3694 // Parse default .hotspotrc settings file
3695 if (!process_settings_file(".hotspotrc", false, args->ignoreUnrecognized)) {
3696 return JNI_EINVAL;
3697 }
3698 #else
And one more comment. Unfortunately I oversight this before... You not
check return value from 'match_special_option_and_act' new function on
lines 3673-3675, but in one case it can return error(JNI_ERR) when
"-XX:NativeMemoryTracking" is specified and INCLUDE_NMT is not defined.
Thus, I think that check for return value should be added(with freeing
memory for env variables in case of error).
Thank you,
Dmitry
>
> New Rev:
>
> http://cr.openjdk.java.net/~jmanson/8079301/webrev.04/
> <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.04/>
>
> Jeremy
>
> On Fri, Jul 10, 2015 at 5:52 AM, Dmitry Dmitriev
> <dmitry.dmitriev at oracle.com <mailto:dmitry.dmitriev at oracle.com>> wrote:
>
> Jeremy,
>
> Two additional comments which I didn't catch earlier...
>
> 1) Order of processing command line options is
> following(Arguments::parse_vm_init_args function): options from
> JAVA_TOOL_OPTIONS environment variable, then options from command
> line and then options from _JAVA_OPTIONS environment variable. And
> last argument wins!
>
> You not change this order in Arguments::parse_vm_init_args
> function, but it seems that order in several other places is changed.
>
> Lines 3665-3667:
>
> 3665 match_special_option_and_act(args, &flags_file);
> 3666 match_special_option_and_act(&java_tool_options_args,
> &flags_file);
> 3667 match_special_option_and_act(&java_options_args, &flags_file);
>
> Here we see that order is following: options from command line,
> then options from JAVA_TOOL_OPTIONS environment variable and then
> options from _JAVA_OPTIONS environment variable. Thus, in this
> case "-XX:+IgnoreUnrecognizedVMOptions" option in
> JAVA_TOOL_OPTIONS environment variable will have bigger priority
> than in command line(last argument wins), but this is differ from
> processing options in Arguments::parse_vm_init_args function.
> Thus, I think that you need to swap 3665 and 3666 lines:
>
> 3665 match_special_option_and_act(&java_tool_options_args,
> &flags_file);
> 3666 match_special_option_and_act(args, &flags_file);
> 3667 match_special_option_and_act(&java_options_args, &flags_file);
>
> Similar situation on lines 3696-3700:
>
> 3696 if (PrintVMOptions) {
> 3697 print_options(args);
> 3698 print_options(&java_tool_options_args);
> 3699 print_options(&java_options_args);
> 3700 }
>
> Command line options are printed before options from
> JAVA_TOOL_OPTIONS environment variable. Thus, I think that you
> need to swap 3697 and 3698 lines:
>
> 3696 if (PrintVMOptions) {
> 3697 print_options(&java_tool_options_args);
> 3698 print_options(args);
> 3699 print_options(&java_options_args);
> 3700 }
>
>
> 2) One more thing about error processing :)
> I think it will be great to free allocated memory for environment
> variables when error occured on lines 3661-3663, 3679-3681 and
> 3685-3687.
> My suggestion is to add some static function which free memory for
> env variables, something like that:
> static void free_memory_for_environment_variables( JavaVMInitArgs
> *options_args ) {
> for (int i = 0; i < options_args->nOptions; i++) {
> os::free(options_args->options[i].optionString);
> }
> FREE_C_HEAP_ARRAY(JavaVMOption, options_args->options);
> }
>
> And change lines 3661-3663 to following:
>
> 3661 if (code != JNI_OK) {
> 3662 free_memory_for_environment_variables(&java_tool_options_args);
> 3663 return code;
> 3664 }
>
> Lines 3679-3681 to following:
>
> 3679 if (!process_settings_file(flags_file, true,
> args->ignoreUnrecognized)) {
> 3680 free_memory_for_environment_variables(&java_tool_options_args);
> 3681 free_memory_for_environment_variables(&java_options_args);
> 3682 return JNI_EINVAL;
> 3683 }
>
> Lines 3685-3687 to following:
>
> 3685 if (!process_settings_file(".hotspotrc", false,
> args->ignoreUnrecognized)) {
> 3680 free_memory_for_environment_variables(&java_tool_options_args);
> 3681 free_memory_for_environment_variables(&java_options_args);
> 3686 return JNI_EINVAL;
> 3687 }
>
> And lines 3706-3715 to following:
>
> 3706 // Done with the envvars
> 3707 free_memory_for_environment_variables(&java_tool_options_args);
> 3708 free_memory_for_environment_variables(&java_options_args);
>
>
> Thank you,
> Dmitry
>
>
> On 10.07.2015 14:36, Dmitry Dmitriev wrote:
>
> 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>
> <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>
> <mailto: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/>
> <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/>
> <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>
> <mailto: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>
> <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> <mailto: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>
> <mailto: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>
> <mailto: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>
> <mailto: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/>
> <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/>
> <http://cr.openjdk.java.net/%7Ejmanson/8079301/webrev.00/>
>
> Jeremy
>
>
>
>
>
>
>
>
More information about the hotspot-runtime-dev
mailing list