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

[Vladimir Kozlov]

You don't need guarante() in caller log_dependency() at line 403 because 
you already have one in callee log_dependency() at line 377.
Otherwise it seems fine.

Thanks,
Vladimir

On 8/7/14 11:26 AM, Morris Meyer wrote:
> 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
>>>>>>>>>>>>
>>>>>
>>>
>