[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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 original code. For my private port, I had to create a thread ID list similar to m_thread_ids and during code browsing for that I happened to see the code in concern and observed that clearing m_thread_pcs is wrong there. After the process gets stopped, while trying to set the thread stop info, we also update the thread list (ProcessGDBRemote::SetThreadStopInfo() ---> Process::UpdateThreadListIfNeeded() ---> ProcessGDBRemote::UpdateThreadList()) and here we can reach UpdateThreadIDList() if m_thread_ids.empty() is true i.e. if we somehow failed to populate m_thread_ids while parsing the stop info packet and need to populate the m_thread_ids now. Repository: rL LLVM https://reviews.llvm.org/D48868 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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, but I never
understood how you got to this.
Index:
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestStopPCs.py
===
---
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestStopPCs.py
(nonexistent)
+++
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestStopPCs.py
(working copy)
@@ -0,0 +1,45 @@
+from __future__ import print_function
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestThreadSelectionBug(GDBRemoteTestBase):
+def test(self):
+class MyResponder(MockGDBServerResponder):
+def haltReason(self):
+return
"T02thread:1ff0d;threads:1ff0d,2ff0d;thread-pcs:10001bc00,10002bc00;"
+
+def threadStopInfo(self, threadnum):
+if threadnum == 0x1ff0d:
+return
"T02thread:1ff0d;threads:1ff0d,2ff0d;thread-pcs:10001bc00,10002bc00;0:0,1:00bc010001;"
+if threadnum == 0x2ff0d:
+return
"T00thread:2ff0d;threads:1ff0d,2ff0d;thread-pcs:10001bc00,10002bc00;0:0,1:00bc020001;"
+
+def qXferRead(self, obj, annex, offset, length):
+if annex == "target.xml":
+return """
+
+ i386:x86-64
+
+
+
+
+""", False
+else:
+return None, False
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+ self.runCmd("log enable gdb-remote packets")
+process = self.connect(target)
+
+self.assertEqual(process.GetNumThreads(), 2)
+th0 = process.GetThreadAtIndex(0)
+th1 = process.GetThreadAtIndex(1)
+self.assertEqual(th0.GetThreadID(), 0x1ff0d)
+self.assertEqual(th1.GetThreadID(), 0x2ff0d)
+self.assertEqual(th0.GetFrameAtIndex(0).GetPC(), 0x10001bc00)
+self.assertEqual(th1.GetFrameAtIndex(0).GetPC(), 0x10002bc00)
Index:
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
===
---
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
(revision 337215)
+++
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
(working copy)
@@ -130,6 +130,8 @@
return self.QEnableErrorStrings()
if packet == "?":
return self.haltReason()
+if packet == "s":
+return self.haltReason()
if packet[0] == "H":
return self.selectThread(packet[1], int(packet[2:], 16))
if packet[0:6] == "qXfer:":
@@ -144,6 +146,9 @@
return self.vAttach(int(pid, 16))
if packet[0] == "Z":
return self.setBreakpoint(packet)
+if packet.startswith("qThreadStopInfo"):
+threadnum = int (packet[15:], 16)
+return self.threadStopInfo(threadnum)
return self.other(packet)
def interrupt(self):
@@ -204,6 +209,9 @@
def setBreakpoint(self, packet):
raise self.UnexpectedPacketException()
+def threadStopInfo(self, threadnum):
+return ""
+
def other(self, packet):
# empty string means unsupported
return ""
> On Jul 16, 2018, at 3:15 AM, Pavel Labath via Phabricator
> wrote:
>
> 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` field, but does not implement a `jThreadsInfo` packet.
>
>
> Repository:
> rL LLVM
>
> https://reviews.llvm.org/D48868
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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 being fixed
exactly? I could see that the code was wrong from reading it, but I never
understood how you got to this.
Index:
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestStopPCs.py
==
-
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestStopPCs.py
(nonexistent)
+++
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestStopPCs.py
(working copy)
@@ -0,0 +1,45 @@
+from __future__ import print_function
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestThreadSelectionBug(GDBRemoteTestBase):
+def test(self):
+class MyResponder(MockGDBServerResponder):
+def haltReason(self):
+return
"T02thread:1ff0d;threads:1ff0d,2ff0d;thread-pcs:10001bc00,10002bc00;"
+
+def threadStopInfo(self, threadnum):
+if threadnum == 0x1ff0d:
+return
"T02thread:1ff0d;threads:1ff0d,2ff0d;thread-pcs:10001bc00,10002bc00;0:0,1:00bc010001;"
+if threadnum == 0x2ff0d:
+return
"T00thread:2ff0d;threads:1ff0d,2ff0d;thread-pcs:10001bc00,10002bc00;0:0,1:00bc020001;"
+
+def qXferRead(self, obj, annex, offset, length):
+if annex == "target.xml":
+return """
+
+ i386:x86-64
+
+
+
+
+""", False
+else:
+return None, False
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+ self.runCmd("log enable gdb-remote packets")
+process = self.connect(target)
+
+self.assertEqual(process.GetNumThreads(), 2)
+th0 = process.GetThreadAtIndex(0)
+th1 = process.GetThreadAtIndex(1)
+self.assertEqual(th0.GetThreadID(), 0x1ff0d)
+self.assertEqual(th1.GetThreadID(), 0x2ff0d)
+self.assertEqual(th0.GetFrameAtIndex(0).GetPC(), 0x10001bc00)
+self.assertEqual(th1.GetFrameAtIndex(0).GetPC(), 0x10002bc00)
Index:
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
=
-
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
(revision 337215)
+++
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
(working copy)
@@ -130,6 +130,8 @@
return self.QEnableErrorStrings()
if packet == "?":
return self.haltReason()
+if packet == "s":
+return self.haltReason()
if packet[0] == "H":
return self.selectThread(packet[1], int(packet[2:], 16))
if packet[0:6] == "qXfer:":
@@ -144,6 +146,9 @@
return self.vAttach(int(pid, 16))
if packet[0] == "Z":
return self.setBreakpoint(packet)
+if packet.startswith("qThreadStopInfo"):
+threadnum = int (packet[15:], 16)
+return self.threadStopInfo(threadnum)
return self.other(packet)
def interrupt(self):
@@ -204,6 +209,9 @@
def setBreakpoint(self, packet):
raise self.UnexpectedPacketException()
+def threadStopInfo(self, threadnum):
+return ""
+
def other(self, packet):
# empty string means unsupported
return ""
Repository:
rL LLVM
https://reviews.llvm.org/D48868
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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` field, but does not implement a `jThreadsInfo` packet. Repository: rL LLVM https://reviews.llvm.org/D48868 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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=155291#toc
Repository:
rL LLVM
https://reviews.llvm.org/D48868
Files:
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
Index: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1523,7 +1523,6 @@
size_t
ProcessGDBRemote::UpdateThreadIDsFromStopReplyThreadsValue(std::string )
{
m_thread_ids.clear();
- m_thread_pcs.clear();
size_t comma_pos;
lldb::tid_t tid;
while ((comma_pos = value.find(',')) != std::string::npos) {
Index: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1523,7 +1523,6 @@
size_t
ProcessGDBRemote::UpdateThreadIDsFromStopReplyThreadsValue(std::string ) {
m_thread_ids.clear();
- m_thread_pcs.clear();
size_t comma_pos;
lldb::tid_t tid;
while ((comma_pos = value.find(',')) != std::string::npos) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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 land. > > looks good. > > > https://reviews.llvm.org/D48868 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1523,7 +1523,6 @@
size_t
ProcessGDBRemote::UpdateThreadIDsFromStopReplyThreadsValue(std::string )
{
m_thread_ids.clear();
- m_thread_pcs.clear();
size_t comma_pos;
lldb::tid_t tid;
while ((comma_pos = value.find(',')) != std::string::npos) {
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1523,7 +1523,6 @@
size_t
ProcessGDBRemote::UpdateThreadIDsFromStopReplyThreadsValue(std::string ) {
m_thread_ids.clear();
- m_thread_pcs.clear();
size_t comma_pos;
lldb::tid_t tid;
while ((comma_pos = value.find(',')) != std::string::npos) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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 > unless we know it's causing problems. Does this cause problems in your > environment? No, it did not cause any problems in our environment. But it is redundant as UpdateThreadPCsFromStopReplyThreadsValue(), which gets called immediately after clearing m_thread_pcs in UpdateThreadIDList(), will anyway clear the old m_thread_pcs (line 1545 without my changes) before populating them from the thread-pcs: received in the stop reply packet. https://reviews.llvm.org/D48868 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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. UpdateThreadPCsFromStopReplyThreadsValue() which we call if we did get a thread-pcs: in this stop reply packet, clears m_thread_pcs so the only reason to have this also in UpdateThreadIDList() is if we had a stub that was inconsistent. I agree that the m_thread_pcs.clear() in UpdateThreadIDsFromStopReplyThreadsValue() is wrong, and your patch here is good. 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 unless we know it's causing problems. Does this cause problems in your environment? https://reviews.llvm.org/D48868 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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 present, we call > UpdateThreadIDsFromStopReplyThreadsValue which clears m_thread_pcs again (bug > #2) and then adds any threads listed to the m_thread_ids. Yes, we shouldn't be clearing the m_thread_pcs in UpdateThreadIDsFromStopReplyThreadsValue() otherwise we would loose the thread PCs populated from stop reply thread-pcs: list just before the UpdateThreadIDsFromStopReplyThreadsValue() call. The other m_thread_pcs.clear() in UpdateThreadIDList() is redundant as we are anyway clearing m_thread_pcs in UpdateThreadPCsFromStopReplyThreadsValue(). https://reviews.llvm.org/D48868 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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 populating it from thread_pcs: seems reasonable.) https://reviews.llvm.org/D48868 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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 implemented, we fall back to looking for the thread-pcs: and threads: key-value pairs in the stop packet that we received. We clear m_thread_pcs, then look if thread-pcs: is present (that's one bug) to add all the thread pc values. After that, we look for the threads: list and if it is present, we call UpdateThreadIDsFromStopReplyThreadsValue which clears m_thread_pcs again (bug #2) and then adds any threads listed to the m_thread_ids. I could believe there are problems here if we're talking to a gdb-remote that doesn't support jThreadsInfo but does provide threads: or thread-pcs: maybe? It'd be interesting to see what the packet log for a stop packet is showing, and what exactly the gdb-remote stub is. https://reviews.llvm.org/D48868 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet
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:
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1523,7 +1523,6 @@
size_t
ProcessGDBRemote::UpdateThreadIDsFromStopReplyThreadsValue(std::string )
{
m_thread_ids.clear();
- m_thread_pcs.clear();
size_t comma_pos;
lldb::tid_t tid;
while ((comma_pos = value.find(',')) != std::string::npos) {
@@ -1598,7 +1597,6 @@
StringExtractorGDBRemote _info = m_stop_packet_stack[i];
const std::string _info_str = stop_info.GetStringRef();
-m_thread_pcs.clear();
const size_t thread_pcs_pos = stop_info_str.find(";thread-pcs:");
if (thread_pcs_pos != std::string::npos) {
const size_t start = thread_pcs_pos + strlen(";thread-pcs:");
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1523,7 +1523,6 @@
size_t
ProcessGDBRemote::UpdateThreadIDsFromStopReplyThreadsValue(std::string ) {
m_thread_ids.clear();
- m_thread_pcs.clear();
size_t comma_pos;
lldb::tid_t tid;
while ((comma_pos = value.find(',')) != std::string::npos) {
@@ -1598,7 +1597,6 @@
StringExtractorGDBRemote _info = m_stop_packet_stack[i];
const std::string _info_str = stop_info.GetStringRef();
-m_thread_pcs.clear();
const size_t thread_pcs_pos = stop_info_str.find(";thread-pcs:");
if (thread_pcs_pos != std::string::npos) {
const size_t start = thread_pcs_pos + strlen(";thread-pcs:");
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
