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

[Morris Meyer]

Vladimir,

Enclosed is the version that takes into account your issues with 
log_dependency().  This has been tested with JPRT.

Thanks for the review.

         --morris meyer

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

On 8/6/14, 6:09 PM, Vladimir Kozlov wrote:
> On 8/5/14 10:28 AM, Morris Meyer wrote:
>> 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
>
> No, it is also called from log_dependency() (the one in 
> dependencies.hpp). That is why asked about missed RM in it.
>
>>
>> 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);
>
> New guarantees are fine.
>
> Thanks,
> Vladimir
>
>>
>> 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
>>>>>>>>>>>
>>>>
>>