RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS
Dmitry Dmitriev
dmitry.dmitriev at oracle.com
Fri Jul 10 12:52:11 UTC 2015
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