RFR(S): 8040920: Uninitialised memory in hotspot/src/share/vm/code/dependencies.cpp

[Morris Meyer]

Vladimir,

Here is a revised version with your suggestions.

I added outer ResourceMarks to

  void Dependencies::log_all_dependencies()

and

void Dependencies::write_dependency_to(CompileLog* log,
                                        DepType dept,
GrowableArray<DepArgument>* args,
                                        Klass* witness)

as they are the only callers to

void Dependencies::write_dependency_to(CompileLog* log,
                                        DepType dept,
GrowableArray<ciBaseObject*>* args,
                                        Klass* witness);

which does not have a ResourceMark - so these aren't nested cases.

void Dependencies::DepStream::log_dependency(Klass* witness)

does call

void Dependencies::write_dependency_to(CompileLog* log,
                                        DepType dept,
GrowableArray<DepArgument>* args,
                                        Klass* witness)

which makes it a nested usage of ResourceMark - and I've added a 
guarantee() to ensure the outer ResourceMark is not affected by the 
inner callee.

void Dependencies::DepStream::print_dependency(Klass* witness, bool 
verbose);

has a new GrowableArray usage and I also added an outer ResourceMark 
here - and a guarantee() that goes along with the inner ResourceMark 
used in the existing code of

void Dependencies::print_dependency(DepType dept, 
GrowableArray<DepArgument>* args,
                                     Klass* witness);

This revision has been tested with JPRT.

Thanks for the review!

         --morris

JBS - https://bug.openjdk.java.net/browse/JDK-8040920
WEBREV - http://cr.openjdk.java.net/~morris/JDK-8040920.08

On 7/22/14, 7:05 PM, Vladimir Kozlov wrote:
> how you determined where you should place ResourceMark? You missed one 
> in log_dependency().
>
> write_dependency_to() methods are used only by Dependencies so they 
> could be private. Then we would guarantee that they are called in 
> known places which may have ResourceMark already.
>
> thanks,
> Vladimir
>
> On 7/21/14 3:42 PM, Morris Meyer wrote:
>> Christian and Vladimir,
>>
>> I have incorporated these comments into this latest webrev.  It has been
>> put through JPRT west.
>>
>>          --mm
>>
>> WEBREV - http://cr.openjdk.java.net/~morris/JDK-8040920.07
>> JBS - https://bug.openjdk.java.net/browse/JDK-8040920
>>
>>
>> On 4/30/14, 6:41 PM, Christian Thalinger wrote:
>>> On Apr 30, 2014, at 11:55 AM, Vladimir Kozlov
>>> <vladimir.kozlov at oracle.com> wrote:
>>>
>>>> On 4/30/14 2:35 PM, Christian Thalinger wrote:
>>>>> On Apr 30, 2014, at 11:26 AM, Vladimir Kozlov
>>>>> <vladimir.kozlov at oracle.com> wrote:
>>>>>
>>>>>> Morris,
>>>>>>
>>>>>> How you verified that you put ResourceMark in all needed places?
>>>>>> That is what I don't like about GrowableArray.
>>>>> He could put a ResourceMark in front of every GrowableArray.  That
>>>>> would do it.
>>>> Nested ResourceMark has bad effect on external GrowableArray: if it
>>>> grows after inner ResourceMark that space will be released after
>>>> inner scope ends. You will get memory stomps because of that. You
>>>> need to be sure that external GrowableArray never grows inside inner
>>>> ResourceMark scope.
>>> Ugh.  You’re right.  Yeah, as I said we don’t need a GrowableArray
>>> here but the regular Array is only for metadata.
>>>
>>>> Vladimir
>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 4/30/14 11:47 AM, Morris Meyer wrote:
>>>>>>> Vladimir and Christian,
>>>>>>>
>>>>>>> Thank you for the reviews.
>>>>>>>
>>>>>>> This version has your comments incorporated and has been put 
>>>>>>> through
>>>>>>> JPRT and passes Parfait cleanly.
>>>>>>>
>>>>>>>          --mm
>>>>>>>
>>>>>>> JBS - https://bugs.openjdk.java.net/browse/JDK-8040920
>>>>>>> WEBREV - http://cr.openjdk.java.net/~morris/JDK-8040920.05
>>>>>>>
>>>>>>>
>>>>>>> On 4/24/14, 2:16 PM, Vladimir Kozlov wrote:
>>>>>>>> Using GrowableArray requires adding ResourceMark otherwise it
>>>>>>>> will be
>>>>>>>> memory leak. Method clear() does not free resources.
>>>>>>>>
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> On 4/24/14 9:30 AM, Morris Meyer wrote:
>>>>>>>>> Folks,
>>>>>>>>>
>>>>>>>>> Could I get a review for this parfait issue?  I've refactored
>>>>>>>>> dependencies to use a GrowableArray to protect from
>>>>>>>>> inconsistencies in
>>>>>>>>> passed in argument pairs (int nargs, DepArgument args[])
>>>>>>>>>
>>>>>>>>> Thanks to Christian Thalinger and Vladimir Kozlov for looking at
>>>>>>>>> earlier
>>>>>>>>> versions of this.
>>>>>>>>>
>>>>>>>>>          --morris
>>>>>>>>>
>>>>>>>>> JBS - https://bugs.openjdk.java.net/browse/JDK-8040920
>>>>>>>>> WEBREV - http://cr.openjdk.java.net/~morris/JDK-8040920.04
>>>>>>>>>
>>