Mailing-List: contact dev-help@harmony.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@harmony.apache.org
Received-SPF: neutral (herse.apache.org: 216.86.168.178 is neither permitted
 nor denied by domain of geir@pobox.com)
Message-ID: <4569DA50.60207@pobox.com>
Date: Sun, 26 Nov 2006 13:17:52 -0500
From: "Geir Magnusson Jr." <geir@pobox.com>
Reply-To: geir@pobox.com
User-Agent: Thunderbird 1.5.0.8 (Macintosh/20061025)
MIME-Version: 1.0
To: dev@harmony.apache.org
Subject: Re: svn commit: r479181 -
 /harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
References: <20061125195939.5167A1A9846@eris.apache.org>
 <4568E034.6000008@pobox.com> <200611260508.17208.gshimansky@gmail.com>
In-Reply-To: <200611260508.17208.gshimansky@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


Gregory Shimansky wrote:
> On Sunday 26 November 2006 03:30 Geir Magnusson Jr. wrote:
>> 1) Why add the SuspendDisabledChecker if not using it?
>>
>> 2) exactly where did you add the assertion?  :)
> 
> It is a hidden assertion class :)

Oh, come on.  An assertion as a side effect?

> 
> Look at the file vm/vmcore/include/suspend_checker.h. There are 2 classes 
> SuspendEnabledChecker and SuspendDisabledChecker. When declared, such 
> variable in constructor checks for suspend status, in destructor it checks 
> that the status is the same. It is often convenient to write just this one 
> line to make sure that all returns from the function have the same suspend 
> status because local variable destructor is executed on every function exit.
> 
> In release, when assert is a noop, these constructors/destructors are 
> optimized into noop as well.
> 
> I saw that this function uses raw ManagedObject pointers. This is dangerous in 
> case when suspend is enabled (equal to GC being enabled) as the object may be 
> moved at any time. So I decided to add this assertion. If it fails some time 
> it will signal that this function is called in a wrong unsafe mode.

Ok - but don't you think that the assert() would be clearer and just as 
efficient?

geir

> 
>> gshimansky@apache.org wrote:
>>> Author: gshimansky
>>> Date: Sat Nov 25 11:59:38 2006
>>> New Revision: 479181
>>>
>>> URL: http://svn.apache.org/viewvc?view=rev&rev=479181
>>> Log:
>>> Fixed 32-bitness in classloader tracing. Added assertion before using a
>>> raw heap object pointer
>>>
>>>
>>> Modified:
>>>    
>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>>
>>> Modified:
>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>> URL:
>>> http://svn.apache.org/viewvc/harmony/enhanced/drlvm/trunk/vm/vmcore/src/c
>>> lass_support/classloader.cpp?view=diff&rev=479181&r1=479180&r2=479181
>>> =========================================================================
>>> ===== ---
>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>> (original) +++
>>> harmony/enhanced/drlvm/trunk/vm/vmcore/src/class_support/classloader.cpp
>>> Sat Nov 25 11:59:38 2006 @@ -589,11 +589,13 @@
>>>
>>>  ClassLoader* ClassLoader::AddClassLoader( ManagedObject* loader )
>>>  {
>>> +    SuspendDisabledChecker sdc;
>>> +
>>>      LMAutoUnlock aulock( &(ClassLoader::m_tableLock) );
>>>      ClassLoader* cl = new UserDefinedClassLoader();
>>>      TRACE2("classloader.unloading.add", "Adding class loader "
>>>          << cl << " (" << loader << " : "
>>> -        << ((VTable*)(*(unsigned**)(loader)))->clss->get_name()->bytes
>>> << ")"); +        << loader->vt()->clss->get_name()->bytes << ")");
>>>      cl->Initialize( loader );
>>>      if( m_capacity <= m_nextEntry )
>>>          ReallocateTable( m_capacity?(2*m_capacity):32 );
>