[8u/9] RFR(S): 8159244: Partially initialized string object created by C2's string concat optimization may escape

[Tobias Hartmann]

Thanks, Vladimir!

Best regards,
Tobias

On 14.06.2016 18:16, Vladimir Kozlov wrote:
> Looks good to me.
> 
> Thanks,
> Vladimir
> 
> On 6/14/16 3:08 AM, Tobias Hartmann wrote:
>> Hi Vladimir,
>>
>> thanks for the review!
>>
>> On 13.06.2016 19:09, Vladimir Kozlov wrote:
>>> Nice work, Tobias.
>>>
>>> Use next tag:
>>>
>>> @requires vm.gc == "Parallel" | vm.gc == "null"
>>>
>>> It will run the test when UseParallelGC is specified or no GC is specified by test environment. Otherwise you may get problem if CMS or other (not G1) is specified.
>>
>> Right, I added the precondition to the test.
>>
>>> We should generate similar barriers code we generate at the end of constructors. See Parse::do_exits() what is applicable for String constructor in your case.
>>
>> In Parse::do_exits() we emit a MemBarRelease if the constructor writes to a final field to ensure that the effect of the initialization is committed to memory before any code publishes a reference to the newly constructed object. I agree that we should do the same after constructing/initializing a new String object because the String.value field is final.
>>
>> From my understanding, a MemBarRelease is basically a combination of a LoadStoreBarrier and a StoreStoreBarrier. See sparc.ad:
>>
>>  __ membar( Assembler::Membar_mask_bits(Assembler::LoadStore | Assembler::StoreStore) );
>>
>> So I wondered why a StoreStoreBarrier is not sufficient in this case. For the record, I found this nice description of a case where a StoreStore is not sufficient: http://www.hboehm.info/c++mm/no_write_fences.html
>>
>> New webrevs:
>> http://cr.openjdk.java.net/~thartmann/8159244/webrev.9.01
>> http://cr.openjdk.java.net/~thartmann/8159244/webrev.8u.01
>>
>> Thanks,
>> Tobias
>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 6/13/16 2:26 AM, Tobias Hartmann wrote:
>>>> Hi,
>>>>
>>>> please review the following patch:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8159244
>>>> http://cr.openjdk.java.net/~thartmann/8159244/webrev.9.00/
>>>> http://cr.openjdk.java.net/~thartmann/8159244/webrev.8u.00/
>>>>
>>>> C2's String concatenation optimization replaces a series of StringBuilder.append() calls by creating a single char buffer array (or byte array with Compact Strings) and emitting direct loads/stores to/from this array. The final StringBuilder.toString() call is replaced by a new String allocation which is initialized to the buffer array (see [1] -> [2], CallStaticJava is replaced).
>>>> Depending on the scheduling of instructions, it may happen that a reference to the newly allocated String object escapes before the String.value field is initialized (see [2], '334 StoreP' stores the String object, '514 StoreP' initializes the String.value field). In a highly concurrent setting, another thread may try to dereference String.value from such a partially initialized String object and crash.
>>>>
>>>> The solution is to add a StoreStore barrier after the String object initialization to prevent subsequent stores to float above (we do the same for the Object.clone intrinsic). I verified correctness of the C2 graph (see [3]) and the generated assembly code (compare baseline [7] and fix [8]).
>>>>
>>>> TestStringObjectInitialization.java reproduces this problem with JDK 7, 8 and 9 (see [4], [5], [6]) in approximately 1 out of 10 runs. I had to disable Indify String Concat, Compressed Oops and G1 to trigger the bug with JDK 9. Tested with JPRT and RBT.
>>>>
>>>> Thanks,
>>>> Tobias
>>>>
>>>> [1] https://bugs.openjdk.java.net/secure/attachment/60305/graph_baseline_before%20SC.png
>>>> [2] https://bugs.openjdk.java.net/secure/attachment/60306/graph_baseline_after_sc.png
>>>> [3] https://bugs.openjdk.java.net/secure/attachment/60304/graph_fix.png
>>>> [4] https://bugs.openjdk.java.net/secure/attachment/60298/JDK7_hs_err_pid17491.log
>>>> [5] https://bugs.openjdk.java.net/secure/attachment/60264/JDK8u_hs_err_pid23015.log
>>>> [6] https://bugs.openjdk.java.net/secure/attachment/60263/JDK9_hs_err_pid383.log
>>>> [7] https://bugs.openjdk.java.net/secure/attachment/60303/baseline.asm
>>>> [8] https://bugs.openjdk.java.net/secure/attachment/60302/fix.asm
>>>>