[Lldb-commits] [PATCH] D93874: [process] fix exec support on Linux

Mon Jan 11 15:27:08 PST 2021

jingham added inline comments.

================
Comment at: lldb/test/API/functionalities/exec/TestExec.py:91-93
+        # Single step to create a thread plan. We have to make sure that after exec
+        # we clear all existing thread plans.
+        thread.StepInstruction(False)
----------------
wallace wrote:
> labath wrote:
> > A separate test case would be better, as (even with the comment) a random future modification of the test could negate this.
> Sure! I'll wait for @jingham to chime first
> Sure! I'll wait for @jingham to chime first

I'm a little confused about this crash.  

Before lldb lets the target proceed, it copies the thread list before proceeding into a temporary list.  Whenever you stop in the debugger, the first thing that happens is ProcessGDBRemote gets the ThreadID's from the stub and figures out which of those thread IDs still exist after the stop.  If the process plugin sees a ThreadID that it also saw before the stop, then it should copy the ThreadSP of the thread over from the "seen at previous stop" list to the current thread list.  At that point the Thread should be bona fide

Then all the ShouldStop type questions iterate over the current thread list, so if somebody asks the plans of the exec Thread from above which it found in the current thread list, ThreadPlan::GetThread will look it up by ID in the current thread list, and find and return that valid thread.

OTOH, if the thread ID didn't still exist, the process plugin should discard it as part of processing the stop, and nobody should be asking its thread plans questions since again you should go through the current thread list and it won't be on that list.  

I don't see under what circumstances you are getting a query to the thread plan for a bogus thread in this instance.

So I'm a little worried that just throwing away the threads when we know we've exec'ed is papering over some flaw lower down in the stack.  Can you give some more details on how exactly this is crashing, I don't have a Linux machine handy to try stuff out on?

Note, I'm also not at all clear what the StepInstruction in the test is for.  At that point in the test, the target will have stopped at your first breakpoint in main.cpp, which is fair ways (more than one instruction) before the exec in main.cpp.

When you stopped before the step, the thread you stopped on will have one ThreadPlan on its thread plan stack: the ThreadPlanBase.  That's the plan which handles all the "unexpected" stops - ones that aren't governed by a thread plan which is actually driving the thread For instance that handles hitting an unrelated breakpoint while doing a step-over, etc.  It is always present, and can't be popped.  

Doing a thread.StepInstruction at that point pushes the "StepOverInstruction" thread plan, runs the process till the StepOverInstruction thread plan says it is done, and then pops that thread plan.  So when you stop after the instruction step, the thread plan stack for this thread should be the same as it was before the instruction step.

If this step-i is necessary to trigger the bug, then again I'm worried we're working around some logic goof rather than finding and fixing the actual problem.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93874