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

[David Holmes]

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
>>>>
>>