harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gregory Shimansky <gshiman...@gmail.com>
Subject Re: [drlvm][em64t] drlvm broken on em64t?
Date Wed, 13 Dec 2006 13:08:58 GMT
Oliver Deakin wrote:
> 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?

I like the patch. If we agree in the other thread that GetSystemProperty 
should just return NULL, I'll change VMI implementation for drlvm, then 
please commit this patch along with Tim's clarification.

-- 
Gregory


Mime
View raw message