[Lldb-commits] [PATCH] D70022: [lldb] [Process/NetBSD] Improve threading support

[Michał Górny via Phabricator via lldb-commits]

mgorny marked 7 inline comments as done.
mgorny added a comment.

I've made the changes locally, I'll test and reupload when I finish updating the whole batch.



================
Comment at: lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp:241
   case TRAP_BRKPT:
-    for (const auto &thread : m_threads) {
-      static_cast<NativeThreadNetBSD &>(*thread).SetStoppedByBreakpoint();
-      FixupBreakpointPCAsNeeded(static_cast<NativeThreadNetBSD &>(*thread));
+    if (thread) {
+      thread->SetStoppedByBreakpoint();
----------------
krytarowski wrote:
> This shall be always true unless there is a kernel issue.
> But this check is fine, I would just add a comment.
We already log the issue above.


================
Comment at: lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp:248
   case TRAP_TRACE:
-    for (const auto &thread : m_threads)
-      static_cast<NativeThreadNetBSD &>(*thread).SetStoppedByTrace();
+    if (thread)
+      thread->SetStoppedByTrace();
----------------
krytarowski wrote:
> Same here. Isn't there a fixup for PC?
> 
> It's not needed in x86 and can be delayed into future.
I dunno and I don't want to proactively add something I can't test right now.


================
Comment at: lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp:268
+    ptrace_state_t pst;
+    Status error = PtraceWrapper(PT_GET_PROCESS_STATE, GetID(), &pst,
+                                 sizeof(pst));
----------------
krytarowski wrote:
> Maybe `GetID()` -> `pid`? Same later.
Indeed, good catch.


================
Comment at: lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp:314
 
     // If a breakpoint was hit, report it
     uint32_t bp_index = LLDB_INVALID_INDEX32;
----------------
Actually, I've decided to remove this block for now rather than adding dummy APIs to make it not crash. We will have to update it to match watchpoint logic later on anyway, so we may as well copy and modify the watchpoint logic instead.


================
Comment at: lldb/source/Plugins/Process/NetBSD/NativeProcessNetBSD.cpp:790
+void NativeProcessNetBSD::RemoveThread(lldb::tid_t thread_id) {
+  Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_THREAD));
+  LLDB_LOG(log, "pid {0} removing thread with tid {1}", GetID(), thread_id);
----------------
krytarowski wrote:
> I would add an assert `thread_id > 0`.
Added here and to `AddThread()` as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70022/new/

https://reviews.llvm.org/D70022