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

[Volker Simonis]

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.

I'd need a sponsor though, because this is a shared changed and
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
>>>
>