RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Jul 14 19:46:27 UTC 2015
On 7/12/15 10:27 AM, Dmitry Dmitriev wrote:
> 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.
I see - JavaVMInitArgs is a C struct in jni.h
If you wrap JavaVMInitArgs for the options for environment variables in
a scoped object that calls the destructor to clean up JavaVMInitArgs,
this would eliminate all these extra calls that you have to make to
free_memory_for_vm_init_args:
class EnvInitArgs : public StackObj {
JavaVMInitArgs _args;
public:
~EnvInitArgs() {
// Code to conditionally delete option memory that you have in
free_memory_for_vm_init_args
}
JavaVMInitArgs* init_args() const { return &_args; }
}
You can put this in the constructor:
+ vm_args->version = JNI_VERSION_1_2;
+ vm_args->options = NULL;
+ vm_args->nOptions = 0;
+ vm_args->ignoreUnrecognized = false;
I think this would be neater than having to check for JNI_OK and call
free everywhere.
Coleen
>
> 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