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

[Jim Ingham via lldb-commits]

Excellent, thanks for persisting on this!

I think your second idea sounds less error prone than having to figure out whether the cache is trustworthy at a particular point.  Maybe even better than triggering off a stop event is to clear the caches before we do whatever might make the threads in the cache invalid.  That should only happen in Process::UpdateThreadList, so we could just move the current virtual UpdateThreadList method -> DoUpdateThreadList, and add a non-virtual UpdateThreadList that clears the cache and then calls UpdateThreadList.

There's also a non-virtual UpdateThreadListIfNeeded, and we could do it there before calling the virtual UpdateThreadList.  But then we'd have to ensure that the only way to call UpdateThreadList if through UpdateThreadListIfNeeded, which seems hard to ensure formally.  So I think just wrapping the virtual method to clear the cache before updating the thread list is the most robust way to do this.


Jim

> On Jan 22, 2021, at 8:53 AM, walter erquinigo via Phabricator <reviews at reviews.llvm.org> wrote:
> 
> wallace added a comment.
> 
> Jim, thanks for the pointers! I think we are getting close to the issue. After doing what you asked, I found out the following:
> 
> - I set up the state of the lldb debugging the program that will exec right before it execs
> - Then I do "continue"
> - ThreadPlan::WillResume is invoked, which effectively clears out the m_thread cache. After this, I would expect no calls to ThreadPlan::GetThread until the the new thread list is created
> - Then suddenly ThreadList::ShouldReportRun is invoked, which is handling the resume event. It eventually asks the current ThreadPlan ShouldReportRun, which invokes GetPreviousPlan() if m_run_vote is eVoteNoOpinion
> 
>  Vote ThreadPlan::ShouldReportRun(Event *event_ptr) {
>    if (m_run_vote == eVoteNoOpinion) {
>      ThreadPlan *prev_plan = GetPreviousPlan();
>      if (prev_plan)
>        return prev_plan->ShouldReportRun(event_ptr);
>    }
>    return m_run_vote;
>  }
> 
> This triggers ends up invoking GetThread()
> 
>  ThreadPlan *GetPreviousPlan() { return GetThread().GetPreviousPlan(this); }
> 
> Which causes the incorrect value to be cached.
> 
> - After this event is processed, the stop exec happens, but the ThreadPlanBase's m_thread cache is not null, which breaks the invariant that you mentioned
> 
>> Keep continuing in the inferior lldb if you have to till you see it get the stop packet that corresponds to the exec. At this point all the thread plans should have no cached threads, so they are going to have to look them up.
> 
> I think that the ShouldReportRun is unavoidable at that step, so I thought of a couple of solutions.
> 
> - One is to to modify GetPreviousPlan() to something like this
> 
>  ThreadPlan *GetPreviousPlan() { return GetThread(/*cache_thread*/ false).GetPreviousPlan(this); }
> 
> This allows traversing the thread plan stack without caching the thread. This might be fine because ShouldReportRun is merely looking for any thread plan in the stack with an m_run_vote other than eVoteNoOpinion. It doesn't use the thread for any other reason.
> 
> - Another solution is to force clear the m_thread caches in all the ThreadPlans as soon as there's a stop event.
> 
> What do you think?
> 
> 
> Repository:
>  rG LLVM Github Monorepo
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D93874/new/
> 
> https://reviews.llvm.org/D93874
>