[Lldb-commits] [PATCH] Prevent LLGS from crashing when exiting - make NativeProcessLinux to wait until ThreadStateCoordinator is fully stopped before entering ~NativeProcessLinux.

[Tamas Berghammer]

Thanks for the explanation (especially for the call stack). Now I see why
your change fix the issue and it looks good, but I still don't like it.
Possibly I would suggest to check for nullptr in the SetRunning function as
it is dereferencing a pointer (returned by GetProcess) what can be nullptr,
but I am not sure if it is better or not.

P.S.: Calling a virtual function in a destructor is almost never undefined
behavior (it is not UB here) just it have surprising effect, and this is
why most book suggest not to do it.

On Tue, Feb 17, 2015 at 6:44 PM, Oleksiy Vyalov <ovyalov at google.com> wrote:

>
> On Tue, Feb 17, 2015 at 10:01 AM, Tamas Berghammer <tberghammer at google.com
> > wrote:
>
>> As far as I know NativeProcessLinux is still fully functional during the
>> execution of it's destructor. The only difference is that the members in
>> GDBRemoteCommunicationServerLLGS defined after the NativeProcessLinux smart
>> pointer are already destructed. If this ordering is the problem then it can
>> be solved with changing the order of the member declarations inside
>> GDBRemoteCommunicationServerLLGS.
>>
>>
> Yes, an instance is functional when its destructor is being called but
> there some limitations:
>
>    - You may hit undefined behavior if virtual function are called when
>    destruction is in progress -
>    http://www.artima.com/cppsource/nevercall.html, i.e. if some virtual
>    function of NativeProcessLinux are called by TSC when ~NativeProcessLinux
>    is in progress - it might be a problem.
>    - NativeProcessLinux is derived from NativeProcessProtocol which in
>    its turn holds list of threads - "std::vector<NativeThreadProtocolSP>
>    m_threads;". Each instance of NativeThreadProtocol maintains a weak pointer
>    to its process -
>    https://github.com/llvm-mirror/lldb/blob/master/include/lldb/Host/common/NativeThreadProtocol.h#L77.
>    NativeThreadProtocol::GetProcess gives you a locked NativeProcessProtocolSP
>    (or (NativeProcessProtocolSP(nullptr) if shared_ptr is no longer alive)).
>
>            Here the situation which I faced when I added just
> m_coordinator_thread.Join (nullptr); without Terminate call:
>
>    - ~NativeProcessLinux is calling StopMonitor
>       - NativeThreadLinux::SetRunning is called from TSC thread -
>       https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/Process/Linux/NativeProcessLinux.cpp#L2141
>       - NativeThreadLinux::SetRunning is trying to get process shared_ptr
>       via GetProcess()
>       - Since GDBRemoteCommunicationServerLLGS
>       holds lldb_private::NativeProcessProtocolSP m_debugged_process_sp; and
>       we're inside of ~NativeProcessLinux - so, shared_ptr is no longer valid
>       and GetProcess() returns NativeProcessProtocolSP(nullptr)
>       - SIGSEGV when trying to call GetProcess()->GetWatchpointMap();
>
>
>
>
>
>> I am confused if the introduction of the Terminate method made any
>> difference.
>>
>>
>   I assume we may put more safety nets
> into NativeThreadProtocol::GetProcess - add more assertations to check
> value of shared_ptr or check return value of GetProcess for nullptr. But to
> be sure that NativeProcessLinux is still accessible via shared_ptr and
> virtual methods can be safely executed on NativeProcessLinux I decided to
> introduce Terminate method.
>
>
>> http://reviews.llvm.org/D7692
>>
>> EMAIL PREFERENCES
>>   http://reviews.llvm.org/settings/panel/emailpreferences/
>>
>>
>>
>
>
> --
> Oleksiy Vyalov | Software Engineer | ovyalov at google.com
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150217/c8271518/attachment.html>