ramana-nvr added a comment.
Sorry, I am not helpful to you in providing a unit test case for this patch. I
am still learning about test suite framework and/or trying to get the lldb test
suite up and running.
Regarding how I got to this, it is not that I have run into some issue due to
the ori
jasonmolenda added a subscriber: ramana-nvr.
jasonmolenda added a comment.
That's a good point Pavel. I tried to write one (below) but I never saw what
the original failure mode was. Venkata, can you help to make a test case that
fails before the patch and works after? Or explain what bug was
That's a good point Pavel. I tried to write one (below) but I never saw what
the original failure mode was. Venkata, can you help to make a test case that
fails before the patch and works after? Or explain what bug was being fixed
exactly? I could see that the code was wrong from reading it,
labath added a comment.
Could you also add a test case for this?
I think it should be possible to test this via the gdb-client
(`test/testcases/functionalities/gdb_remote_client/`) test suite. If I
understood the previous comments correctly, you'll need to mock a server that
sends a `thread-pcs
This revision was automatically updated to reflect the committed changes.
Closed by commit rL336956: Remove incorrect thread-pc-values clearing (authored
by jmolenda, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D48868?vs=154947&id=1
I do not have commit access to LLDB. Could someone please commit the below
changes?
On Thu, Jul 12, 2018 at 2:32 AM, Jason Molenda via Phabricator <
revi...@reviews.llvm.org> wrote:
> jasonmolenda accepted this revision.
> jasonmolenda added a comment.
> This revision is now accepted and ready to
jasonmolenda accepted this revision.
jasonmolenda added a comment.
This revision is now accepted and ready to land.
looks good.
https://reviews.llvm.org/D48868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/m
ramana-nvr updated this revision to Diff 154947.
ramana-nvr added a comment.
For now, leaving the m_thread_pcs.clear() in UpdateThreadIDList() unchanged as
it is not causing any problems.
https://reviews.llvm.org/D48868
Files:
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
Index: s
ramana-nvr added a comment.
Sorry, I wasn't clear in my previous comment.
In https://reviews.llvm.org/D48868#1158236, @jasonmolenda wrote:
> I don't have strong feelings about removing the m_thread_pcs() in
> UpdateThreadIDList(), but I think I would prefer leaving it in to changing it
> unles
jasonmolenda added a comment.
I think the m_thread_pcs.clear() in UpdateThreadIDList() is to clear out any
old thread pc values that we might have populated in the past. The only way I
can see this happening is if the remote stub SOMETIMES returned the thread-pcs:
list in the stop packet. Upd
ramana-nvr added a comment.
In https://reviews.llvm.org/D48868#1156416, @jasonmolenda wrote:
> If jThreadsInfo isn't implemented, we fall back to looking for the
> thread-pcs: and threads: key-value pairs in the stop packet that we received.
>
> We look for the threads: list and if it is pres
jasonmolenda added a comment.
(just to be clear, the m_thread_pcs.clear() in
ProcessGDBRemote::UpdateThreadIDList that I called a bug -- today we only have
two ways of populating that, jThreadsInfo or the thread-pcs: value in the stop
packet. So clearing it unconditionally here, and then popul
jasonmolenda added a comment.
I could imagine there being a bug in this ProcessGDBRemote::UpdateThreadIDList
-> ProcessGDBRemote::UpdateThreadIDsFromStopReplyThreadsValue sequence when
talking to a gdb-remote stub that does not implement the lldb-enhancement
jThreadsInfo. If jThreadsInfo isn't
labath added a comment.
Could you elaborate on how/why is this wrong? E.g. in which situations does it
matter whether we clear the PC list?
https://reviews.llvm.org/D48868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.
ramana-nvr created this revision.
ramana-nvr added a reviewer: labath.
The function ProcessGDBRemote::UpdateThreadIDsFromStopReplyThreadsValue(),
which is being called after the thread PCs are updated, is clearing the thread
PC list and that is wrong.
https://reviews.llvm.org/D48868
Files:
15 matches
Mail list logo