RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Jul 10 21:16:11 UTC 2015
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