RFR: 8079301: Some command line options not settable via JAVA_TOOL_OPTIONS

Dmitry Dmitriev dmitry.dmitriev at oracle.com
Thu Jul 9 21:52:23 UTC 2015 

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/
>
> Updated test:
>
> http://cr.openjdk.java.net/~jmanson/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> 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
>>
>> "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>
>>> 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>
>>>> 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
>>>>> wrote:
>>>>>
>>>>>   Hi David,
>>>>>> Thanks for taking a look.
>>>>>>
>>>>>> On Mon, May 4, 2015 at 8:32 PM, David Holmes <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/
>>>>>>
>>>>>>
>>>>>>   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/
>>>>>>>>
>>>>>>>> Jeremy
>>>>>>>>
>>>>>>>>
>>>>>>>>

More information about the hotspot-runtime-dev mailing list