RFR (S): 8024943: ciReplay: fails to dump replay data during safepointing

[Vladimir Ivanov]

Thank you, Chris.

Best regards,
Vladimir Ivanov

On 10/4/13 12:51 AM, Christian Thalinger wrote:
> Looks good.
>
> On Oct 3, 2013, at 7:30 AM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
>
>> Updated webrev:
>>   http://cr.openjdk.java.net/~vlivanov/8024943/webrev.01/
>>
>> Verified that replay data is dumped correctly in the following combinations:
>>   - thread_in_native
>>   - thread_in_vm
>>   - thread_in_vm + Compile_lock
>>
>> Regarding ciEnv in invalid state (leaving aside the cases when the instance is corrupted), the only case when it is possible is when the VM crashed in ~ciEnv(). Don't know whether fixing it is worth the effort - in both cases we don't get valid replay data file.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 10/3/13 12:57 AM, John Rose wrote:
>>> On Oct 2, 2013, at 1:15 PM, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
>>>
>>>> http://cr.openjdk.java.net/~vlivanov/8024943/webrev.00/
>>>> 15 lines changed: 11 ins; 0 del; 4 mod
>>>>
>>>> In some situations VM fails to dump compiler replay file (produces empty replay_pid%p.log). The reason is that ciEnv::dump_replay_data tries to do thread state transition to VM unconditionally and acquire Compile_lock. It doesn't always work in context of VM error reporter.
>>>>
>>>> The fix is to provide new method specifically for VM error reporter (ciEnv::dump_replay_data_unsafe) which doesn't perform aforementioned actions.
>>>>
>>>> Testing: manual (checked that a valid dump is produced for a crash during safepointing).
>>>
>>> Is is possible that you've pushed a bug further into the CI code?  I would like you to take a look at this (if you haven't already) and either make code adjustments or describe current assumptions in comments, for the next maintainer.
>>>
>>> What I mean about "pushed a bug" is that the CI is (generally speaking) sensitive to the thread state.  (Note the GUARDED_VM_ENTRY macros everywhere.)  The dump_replay_data function calls a bunch of dump_replay_data subroutines on lots of little data structures in the CI.  Are any of them also sensitive to thread state?  If so, perhaps you want to have your
>>>
>>> Also, the ciEnv::current() call returns something to the error reporter, and if that something is non-null, it is assumed to be (up to high probability, of course) a valid ciEnv structure.  Are there any places where the ciEnv guy is in an inconsistent state but still published by ciEnv::current()?  Probably not, but it might be worth a comment noting the appropriate conditions.  My specific worry is that the call to remove_symbols in ~ciEnv might leave a space where the dumper would not see valid state.
>>>
>>> This crash dumper stuff cannot be nailed down 100%, of course, but this might be a good moment to make sure we've at least stomped on it a bit.
>>>
>>> Reviewed, pending resolution of the above.
>>>
>>> Thanks,
>>> — John
>>>
>