RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS
Dmitry Dmitriev
dmitry.dmitriev at oracle.com
Sun Jul 12 14:27:26 UTC 2015
Hello Coleen,
I think that adding destructor not help with this, since all this code
with free_memory_for_vm_init_args is a special workaround for processing
options in env variables and not applied to the options from command
line which passed in 'args' to the Arguments::parse function.
About match_special_option_and_act. Probably it can be processed in the
following way(raw idea):
code = match_special_option_and_act(&java_tool_options_args, &flags_file);
if (code == JNI_OK) {
// call next
code = match_special_option_and_act(args, &flags_file);
}
if (code == JNI_OK) {
// call next
code = match_special_option_and_act(&java_options_args, &flags_file);
}
if (code != JNI_OK) {
free_memory_for_vm_init_args(&java_tool_options_args);
free_memory_for_vm_init_args(&java_options_args);
return code;
}
Thanks,
Dmitry
On 11.07.2015 0:16, Coleen Phillimore wrote:
>
> Hi, I have a couple of suggestions.
>
> Can you add a destructor to JavaVMInitArgs which will delete the
> options things if they exist? Then you won't need these
> free_memory_for_vm_init_args functions. I think this would work.
>
> Secondly, I was also going to comment about
> match_special_option_and_act not using it's return value. The
> destructor code would make this less cumbersome. I like the
> refactoring though.
>
> Thanks,
> Coleen
>
> On 7/10/15 2:40 PM, Dmitry Dmitriev wrote:
>> 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