KLapshin added a comment.
In http://reviews.llvm.org/D12968#261762, @labath wrote:
> I have committed http://reviews.llvm.org/D13056 with the event hijacking
> portion from your patch. I think your use case should be working now. I am
> planning to return to this later, as I believe there are
KLapshin added a comment.
In http://reviews.llvm.org/D12968#259931, @clayborg wrote:
> This can't be the real fix for this. We must be able to trust the empty()
> function call. We must have someone playing with the collection without
> locking the mutex. Please find the real bug.
Hello
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
This can't be the real fix for this. We must be able to trust the empty()
function call. We must have someone playing with the collection without locking
the mutex. Please find
labath added a comment.
This is definitely not a proper fix for this problem, it merely sweeps the
problem under the carpet. If something like this makes a difference then it
means something has gone horribly wrong a long time ago. I haven't been able to
reproduce this locally, but I'd
KLapshin added a comment.
@clayborg, @labath,
After more deep investigation I think setting listener for hijacking also looks
like workaround. Setting additional listener just change code flow path and
buggy path not executed, thus no crash. At top level - crash appeared in
labath added inline comments.
Comment at: source/Target/Process.cpp:3934
@@ +3933,3 @@
+
+ListenerSP listener_sp (new
Listener("lldb.Process.HaltForDestroyOrDetach.hijack"));
+HijackProcessEvents(listener_sp.get());
ki.stfu wrote:
> btw:
KLapshin added a comment.
@labath,
I updated patch against actual source in SVN - i.e. with taking into account
your change (Halt*->Stop*).
Regarding matter of this patch - I just tried to make process stop synchronous,
see Process::ResumeSynchronous() - same technique was applied month ago.
KLapshin marked an inline comment as done.
KLapshin added a comment.
Done.
Repository:
rL LLVM
http://reviews.llvm.org/D12968
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
KLapshin added a comment.
In http://reviews.llvm.org/D12968#251719, @labath wrote:
> In http://reviews.llvm.org/D12968#251696, @KLapshin wrote:
>
> > Regarding matter of this patch - I just tried to make process stop
> > synchronous, see Process::ResumeSynchronous() - same technique was applied
labath added a comment.
In http://reviews.llvm.org/D12968#251696, @KLapshin wrote:
> Regarding matter of this patch - I just tried to make process stop
> synchronous, see Process::ResumeSynchronous() - same technique was applied
> months ago (@ki.stfu).
>
> Agreed what crash may be related to
KLapshin removed rL LLVM as the repository for this revision.
KLapshin updated this revision to Diff 35491.
http://reviews.llvm.org/D12968
Files:
source/Target/Process.cpp
Index: source/Target/Process.cpp
===
---
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
If you are going to hijack you do need to do it before you call Halt().
Repository:
rL LLVM
http://reviews.llvm.org/D12968
___
KLapshin updated this revision to Diff 35392.
Repository:
rL LLVM
http://reviews.llvm.org/D12968
Files:
source/Target/Process.cpp
Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
KLapshin added a comment.
Greg, I reworked initial workaround solution, now this is exact fix.
Repository:
rL LLVM
http://reviews.llvm.org/D12968
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
KLapshin updated the summary for this revision.
KLapshin updated this revision to Diff 35390.
Repository:
rL LLVM
http://reviews.llvm.org/D12968
Files:
source/Target/Process.cpp
Index: source/Target/Process.cpp
===
---
KLapshin added a subscriber: labath.
KLapshin added a comment.
Due to @labath reworked and replaced HaltForDestroyOrDetach to
StopHaltForDestroyOrDetach method (see http://reviews.llvm.org/D13056) and his
patch already approved by @clayborg and crash still reproducible with just race
condition
clayborg added a comment.
To work around it you might be able to call SBProcess::Halt() first?
Repository:
rL LLVM
http://reviews.llvm.org/D12968
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg added a comment.
You should be able to just call Destroy(), we shouldn't crash. We will need to
fix the core it allow this since people can/will call this.
Repository:
rL LLVM
http://reviews.llvm.org/D12968
___
lldb-commits mailing
dawn resigned from this revision.
dawn edited reviewers, added: clayborg; removed: dawn.
dawn added a comment.
I'll watch and learn from Ilia and Greg :)
Repository:
rL LLVM
http://reviews.llvm.org/D12968
___
lldb-commits mailing list
KLapshin created this revision.
KLapshin added reviewers: dawn, ki.stfu, abidh.
KLapshin added a subscriber: lldb-commits.
KLapshin set the repository for this revision to rL LLVM.
This patch fixes lldb core crash in Listener waiting for process ended and
operates with already invalid data if
ki.stfu added a subscriber: clayborg.
ki.stfu added a comment.
For me it looks like a workaround because here is assumed the
SBProcess::Destroy will do all required work.
@clayborg, is it permissible to make a Process::Destroy(force_kill=false) for a
running process?
If so, it's a bug in lldb
21 matches
Mail list logo
