RFR(S): 8145015: jni_GetStringCritical asserts for empty strings

[David Holmes]

Hi Volker,

This didn't get through last night due to an internal technical problem. 
I will be resubmitting it today. Sorry about the delay. JPRT is quite 
busy at the moment.

David

On 14/12/2015 10:31 PM, David Holmes wrote:
> Hi Volker,
>
> On 14/12/2015 6:27 PM, Volker Simonis wrote:
>> Hi David,
>>
>> thanks for the review.
>>
>> "const jchar*" means a pointer to a constant character. I removed the
>> 'const' because that way I could get rid of the extra 'buf' variable
>> which was previously used to allocate and fill the temporary string.
>> With "const jchar * ret" it wouldn't be possible to change the
>> characters pointed to by 'ret'.
>>
>> Returning "jachr *ret" as "const jchar * ret" on the other hand is no
>> problem - it is just a narrowing conversion saying that once we return
>> "ret" the characters it points to can not be changed any more.
>>
>> So I think the code is correct.
>
> Okay. I keep forgetting that const isn't saying something about the
> object at the end of the pointer, but about what you're allowed to do to
> the object through the pointer.
>
>> I'd need a sponsor though, because this is a shared changed and
>
> I'm pushing this to hs-rt for you now.
>
> Cheers,
> David
>
>> unfortunately we still can not push shared code changes (I'll put this
>> sentence in every mail now although I have little to no hope that it
>> will change something :(
>>
>> Thank you and best regards,
>> Volker
>>
>>
>>
>> On Mon, Dec 14, 2015 at 8:25 AM, David Holmes
>> <david.holmes at oracle.com> wrote:
>>> +1 from me. My only query is whether:
>>>
>>> 3194   jchar* ret;
>>>
>>> should retain the const, as the return type is "const jchar *"?
>>>
>>> Thanks,
>>> David
>>>
>>>
>>> On 14/12/2015 5:11 PM, Tobias Hartmann wrote:
>>>>
>>>> Hi Volker,
>>>>
>>>> thanks for taking care of this! Your fix looks good to me (not a
>>>> reviewer).
>>>>
>>>> Best,
>>>> Tobias
>>>>
>>>> On 09.12.2015 18:53, Volker Simonis wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> can somebody please review and sponsor the following fix:
>>>>>
>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2015/8145015/
>>>>> https://bugs.openjdk.java.net/browse/JDK-8145015
>>>>>
>>>>> The problem is that change "8141132: JEP 254: Compact Strings" removed
>>>>> the handling for empty strings from jni_GetStringCritical(). As a
>>>>> consequence, the functions will now assert with:
>>>>>
>>>>> assert(is_within_bounds(which)) failed: index 0 out of bounds 0
>>>>>
>>>>> if called with a string of length zero (because it tries to access the
>>>>> zeroth element of a zero-length array).
>>>>>
>>>>> The fix is trivial, just use 's_value->base(T_CHAR)' instead of
>>>>> 's_value->char_at_addr(0)'.
>>>>>
>>>>> While doing this fix I also noticed that jni_GetStringCritical()
>>>>> doesn't handle out-of-memory situations when creating a new character
>>>>> array (before this wasn't necessary because it always returned the
>>>>> original character array, but now with compressed strings it may have
>>>>> to allocate a new array). So I've also added the new check by using
>>>>> NEW_C_HEAP_ARRAY_RETURN_NULL() instead of NEW_C_HEAP_ARRAY().
>>>>>
>>>>> Notice that I also allocate one extra character for zero termination.
>>>>> While it seems that this is not strictly necessary, it is now the same
>>>>> code like in jni_GetStringChars(). And it again simplifies the
>>>>> handling of zero-length strings. Without the extra character it would
>>>>> be hard to distinguish out-of-memory errors from the allocation of
>>>>> zero bytes, because malloc() is free to return NULL if called with an
>>>>> allocation size of zero.
>>>>>
>>>>> Thank you and best regards,
>>>>> Volker
>>>>>
>>>