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

[Vince Harron]

Thanks Pavel.

> the root cause is unclear ownership semantics between Threads, Processes
and TSC

Who is going to work on this problem?  How do we solve this problem?
Document our best guess and refactor to fit?  (And it that doesn't work,
try again)

Vince









On Wed, Feb 18, 2015 at 3:46 AM, Pavel Labath <labath at google.com> wrote:

> Greetings,
>
> I have been following this discussion, and would like to add my 2 cents.
> I'll start with my thoughts on virtual functions.
>
> > On 17 February 2015 at 18:44, 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.
>
>
> And a more quote from the standard.
>
> > Member functions, including virtual functions (10.3), can be called
> during construction or destruction (12.6.2). When a virtual function is
> called directly or indirectly from a
>
> >  constructor or from a destructor, including during the construction or
> destruction of the class’s non-static data members, and the object to which
> the call applies is the object (call it
>
> >  x) under construction or destruction, the function called is the final
> overrider in the constructor’s or destructor’s class and not one overriding
> it in a more-derived class.
>
>
> Judging from this, there is nothing undefined about the situation you
> mentioned. While ~NativeProcessLinux is in progress any call to its virtual
> functions will resolve "as expected". This is especially true if you are
> still executing the body of the destructor (as would be the case if you
> placed the Join() call in ~NativeProcessLinux), as all the member variables
> are still fully constructed, so you cannot get undefined behavior if the
> virtual functions do member access.
>
> A different case would be if you were calling virtual functions of the
> (now non-existing) NativeProcessLinux object after its destructor has
> completed, and the destruction has moved on to its Base classes. In some
> cases the behavior would be undefined, in others "defined, but surprising".
> And if this were to happen (which, as far as i can see, is not the case),
> then I would argue that the bug is in the fact that ThreadStateCoordinator
> was still alive after the destruction of NativeProcessLinux  -- the
> coordinator is owned by nativeprocess, so it should never outlive it
>
> Therefore I think it is a bad idea to create a "destructor" function just
> to avoid doing something in a destructor. The presence of Terminate()
> defeats the purpose of having a shared pointer to the process: a shared
> pointer should delete an object, once the last pointer to it goes out of
> scope, but right now you cannot properly delete the process object in any
> other way except by calling ~GDBRemoteCommunicationServerLLGS(). If someone
> happened to be holding a NativeProcessProtocolSP expecting it to be a
> functional process, it will be surprised that it is non functional after
> GDBRemoteCommunicationServerLLGS has been gone.
>
> Since it seems that the root of the problem was something else (a null ptr
> dereference in NativeThreadLinux), and this has already been addressed, I
> would recommend moving the Join() back to the destructor. Unless there are
> other issues which would require its presence... (?)
>
> PS:
> After my discussion with Tamas, I have come to think that the root cause
> is unclear ownership semantics between Threads, Processes and TSC: a
> process holds shared_ptrs to threads, which seems to imply that it is
> sharing the ownership of them with someone else. However, from the code it
> would seem that the threads should never outlive the process (or the TSC
> for that matter). Therefore, it would seem that a Process (or maybe the
> TSC) is the perfect candidate for the "owner" of it's threads, which would
> have the sole responsibility for destroying them (which could be
> represented by a unique_ptr). And then, when the lifetime of threads gets
> tied to the lifetime of their process, they no longer need to hold a shared
> (or weak) pointer, they can be happy with a regular pointer. However, this
> is definitely an issue far out of scope of this CL.
>
> This completes my braindump. Sorry about the length, I did not expect it
> to be this long when I started. I realize some of the claims might be too
> strong for someone who just started and is getting to know the codebase,
> but I figure I might as well send it, since I spent so much time thinking
> about it.
>
>
> http://reviews.llvm.org/D7692
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>


-- 

Vince Harron | Technical Lead Manager | vharron at google.com | 858-442-0868
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150218/c3e79df5/attachment.html>