Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.
Looks like patch was not committed.
https://reviews.llvm.org/D17635
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
zturner added a comment.
I can remove that comment for you, no worries.
http://reviews.llvm.org/D17635
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
zturner added a comment.
Petr, is this one ok to go in?
http://reviews.llvm.org/D17635
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Honsik added a comment.
Yes please, with that comment change jingham has mentioned.
Do you want me to create new diff?
http://reviews.llvm.org/D17635
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
jingham added a comment.
More precisely, the "Public running lock..." part of the comment.
http://reviews.llvm.org/D17635
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.
I marked a comment left over from a previous draft of the patch that isn't
needed. Other than that, this looks fine.
Comment at: source/Target/Process.cpp:3561-3562
@@
Honsik updated this revision to Diff 50373.
Honsik added a comment.
Sorry for the delay, I have been on holiday.
I have modified the patch as jingham requested. The public run lock is reset
back on error in both Resume and ResumeSynchronous methods.
http://reviews.llvm.org/D17635
Files:
jingham added a comment.
I don't think you can manipulate the public run lock in PrivateResume like
this. PrivateResume gets run in a bunch of places (like calling functions)
that are way below the level the public run lock. You probably need to catch
errors from PrivateResume in Resume and
jingham requested changes to this revision.
jingham added a reviewer: jingham.
jingham added a comment.
This revision now requires changes to proceed.
I agree with Zachary, it would be better to put it in PrivateResume before the
call to WillResume. Having this happen in Process::PrivateResume
zturner added a comment.
It doesn't look like `Process::PrivateResume()` returns an error if the process
is alive unless `WillResume()` returns an error, which is up to the individual
process implementation. So maybe the short circuit needs to happen there.
This isn't really my area though
Honsik added a comment.
I tried to put this check in PrivateResume, but its not that simple because of
the public RUN lock. I am not that sure if it is safe to always unclock the
lock inside PrivateResume.
http://reviews.llvm.org/D17635
___
jingham added a subscriber: jingham.
jingham added a comment.
It's okay to short-circuit this here, but why was PrivateResume not returning
an error when the process was not alive. That error should have gotten
propagated to the caller, obviating the need for this short-circuit.
12 matches
Mail list logo