RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS
Jeremy Manson
jeremymanson at google.com
Fri Jul 10 17:15:19 UTC 2015
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.
New Rev:
http://cr.openjdk.java.net/~jmanson/8079301/webrev.04/
Jeremy
On Fri, Jul 10, 2015 at 5:52 AM, Dmitry Dmitriev <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>
>>>
>>> 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