Mailing-List: contact dev-help@harmony.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@harmony.apache.org
Received-SPF: pass (herse.apache.org: domain of oliver.deakin@googlemail.com
 designates 64.233.182.185 as permitted sender)
DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
        s=beta; d=googlemail.com;
        h=received:message-id:date:from:user-agent:mime-version:to:subject:references:in-reply-to:content-type:content-transfer-encoding;
        b=YgMqyKA6LNApyGsKsVrTqIm578Fcnk06Bd06duuKjX3LML0SJGqZ9GmKzPjvnTuBoh1LHf0h6/n3eknGMdISWnATj28L4Nuq/k56ourXdatRDOaXsFLMcE+tl2Usi16YhgDccNh+9zLORMcTMEFwblPKRakaUv9cWNaMhQUzKvw=
Message-ID: <457FD207.2000406@googlemail.com>
Date: Wed, 13 Dec 2006 10:12:23 +0000
From: Oliver Deakin <oliver.deakin@googlemail.com>
User-Agent: Thunderbird 1.5.0.8 (Windows/20061025)
MIME-Version: 1.0
To: dev@harmony.apache.org
Subject: Re: [drlvm][em64t] drlvm broken on em64t?
References: <457EC663.5040209@pobox.com>
 <457EE270.2040602@pobox.com>	 <elmpad$tof$1@sea.gmane.org>
 <457EEC45.20308@pobox.com>	 <eln5bt$ee3$1@sea.gmane.org>
 <457F1E88.7050807@pobox.com>
	 <c3755b3a0612121334w3d56cebbk66b34dc0b3189506@mail.gmail.com>
 <c3755b3a0612121337h17b26246l383304578a3f14ec@mail.gmail.com>
 <eln8kp$ph5$1@sea.gmane.org> <457F2FCF.2040905@pobox.com>
 <elnc0q$6k4$1@sea.gmane.org> <457F350A.5090104@pobox.com>
In-Reply-To: <457F350A.5090104@pobox.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Geir Magnusson Jr. wrote:
>
>
> Gregory Shimansky wrote:
>> Geir Magnusson Jr. wrote:
>>> I found it - comment below :
>>>
>>> Gregory Shimansky wrote:
>>>>
>>>> I've created a patch to classlib which fixes the problem with 
>>>> drlvm. Now if you could test it with IBM VME, I'll commit it:
>>>>
>>>> Index: modules/luni/src/main/native/luni/shared/luniglob.c
>>>> ===================================================================
>>>> --- modules/luni/src/main/native/luni/shared/luniglob.c (revision 
>
> [SNIP]
>
>>>
>>> This puts a bandage on the problem - it seems like we changed the 
>>> API of GetSystemProperty() because this code clearly expected not to 
>>> get an error if the property wasn't set.
>>
>> I've read Oliver's comment to r486100 and it didn't look like this 
>> code shouldn't expect an error from GetSystemProperty:
>>
>> ======================================
>> Previously we just replaced whatever was already in the 
>> org.apache.harmony.boot.class.path property with our own 
>> bootclasspath. We should not assume that this property is empty 
>> before we use it - it depends on the VM's bcp initialisation order 
>> and how it utilises this property internally.
>> ======================================
>>
>> The "We should not assume" doesn't mean "it should always be this 
>> way" in my understanding.
>
> Right - the thing I was asking if the behavior of GetSystemProperty() 
> changed.
>
> Looking at Ollie's change in r486100, he seems to think that it's not 
> an error to not have the property set, but our impl of it does.
>
> That's the issue I'm trying to get to - does the API of 
> GetSystemProperty() specify that it returns an error code if the 
> property isn't set?
>
> If so, then Ollie's code is wrong.  If not, then it's either that the 
> API is ambiguous (Ollie's assumption wasn't unreasonable) or our impl 
> of GetSystemProperty() is wrong.
>

Apologies for the break - I must admit that I did not test the change on 
DRLVM before the commit.
The IBM VME implementation of GetSystemProperty() does not return an 
error code in the case of
a non-existent property. I had assumed that this must have been defined 
behaviour for that function,
and that it would also work on DRLVM - I guess the API must be ambiguous 
here then, since we
appear to have two different outcomes for this case.

I don't think we should treat it as an error if the property is not set. 
It makes sense to just return
something that indicates the property is not set (ie NULL) rather than 
returning an error code.
We should still check for errors after the GetSystemProperty() call, but 
this should not be an
indicator of a non-existent property (as this isn't really an error IMHO).

In this case (and just having seen the API clarification Tim has sent 
out) I would suggest
the following patch:

Index: luniglob.c
===================================================================
--- luniglob.c    (revision 486569)
+++ luniglob.c    (working copy)
@@ -269,14 +269,18 @@
         /* Make a string version of the CP separator */
         char cpSeparator[] = {(char)hysysinfo_get_classpathSeparator 
(), '\0'};
        
-        /* Read current value of bootclasspath property */
+        /* Read current value of bootclasspath property - sets
+           bootstrapClassPath = NULL if the property does not exist */
         rcGetProperty = (*vmInterface)->GetSystemProperty (vmInterface,
             BOOTCLASSPATH_PROPERTY,
             &bootstrapClassPath);
 
-        /* Gregory - no property is found, VM bootclasspath is not 
defined */
+        /* There has been an error getting the property - cleanup and 
exit */
         if (VMI_ERROR_NONE != rcGetProperty)
-            bootstrapClassPath = NULL;
+        {
+            returnCode = JNI_ERR;
+            goto cleanup;
+        }
 
         qsort(props, number, sizeof(key_value_pair), props_compare);


Comments?

Regards,
Oliver


> geir
>
>

-- 
Oliver Deakin
IBM United Kingdom Limited