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

[Volker Simonis]

Hi David,

no problem. I saw it just went trough.

Thanks for your support,
Volker

On Mon, Dec 14, 2015 at 9:43 PM, David Holmes <david.holmes at oracle.com> wrote:
> 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
>>>>>>
>>>>
>