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

wallace updated this revision to Diff 319069. wallace added a comment. apply suggestion Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93874/new/ https://reviews.llvm.org/D93874 Files: lldb/include/lldb/Target/Process.h lldb/include/lldb/Target

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

wallace added inline comments. Comment at: lldb/test/API/functionalities/exec/TestExec.py:133-158 +exe = self.getBuildArtifact("a.out") +secondprog = self.getBuildArtifact("secondprog") + +# Create the target +target = self.dbg.CreateTarget(exe) +

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

jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. Yes, this is how I should have done it originally. Thanks! I had one suggestion for making the test more compact which you can do or not as you please. Other than the LGTM.

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

wallace updated this revision to Diff 318640. wallace added a comment. Herald added a subscriber: emaste. Updated based on @jingham's idea, and added an independent test for this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93874/new/ https://rev

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

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

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

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

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

The ThreadPlanStack always has one base thread plan. You can't get rid of that one, the system relies on its always being there. Despite it's name, DiscardThreadPlans doesn't actually discard all the thread plans, just all but the base thread plan... So I think that's neither here nor there..

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

I've tried to find a way to move the calls the way you mentioned, but it doesn't seem trivial. Some more information: - The invocation to the thread plan is done by Thread::ShouldStop, where it does ``` // We're starting from the base plan, so just let it decide; if (current_plan->IsBasePlan(

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

> On Jan 21, 2021, at 2:33 PM, Jim Ingham wrote: > > > >> On Jan 21, 2021, at 12:51 PM, walter erquinigo via Phabricator >> wrote: >> >> wallace added a comment. >> >> Sorry for returning late to this diff, but I have some additional >> information. This is what's happening: >> >> - Bef

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

> On Jan 21, 2021, at 12:51 PM, walter erquinigo via Phabricator > wrote: > > wallace added a comment. > > Sorry for returning late to this diff, but I have some additional > information. This is what's happening: > > - Before the exec, the base thread plan holds a reference to the thread

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

wallace added a comment. Sorry for returning late to this diff, but I have some additional information. This is what's happening: - Before the exec, the base thread plan holds a reference to the thread pointer - During the exec, the original thread is destroyed in ProcessGDBRemote::SetLastStopP

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

Thanks for looking into this further! The thing to figure out is who still has a reference to either the Thread * or to the ThreadPlanStack over the destruction of the thread. That shouldn't be allowed to happen. Jim > On Jan 11, 2021, at 10:01 PM, walter erquinigo via Phabricator > wrote:

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

wallace added a comment. I've done a lightweight test and it seems that the BaseThreadPlan is being asked for the stop reason when the exec happens, but it holds a reference to the thread whose destructor has been called, which causes the crash. On Darwin, as Greg said, the BaseThreadPlan is de

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

jingham added inline comments. Comment at: lldb/test/API/functionalities/exec/TestExec.py:93 +# we clear all existing thread plans. +thread.StepInstruction(False) + clayborg wrote: > On Darwin threads have unique identifiers and the thread ID befo

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

clayborg added inline comments. Comment at: lldb/test/API/functionalities/exec/TestExec.py:93 +# we clear all existing thread plans. +thread.StepInstruction(False) + On Darwin threads have unique identifiers and the thread ID before exec != threa

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

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)

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

clayborg added a comment. I still think we should add a Process::ProcessDidExec() as mentioned in the inline comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93874/new/ https://reviews.llvm.org/D93874 __

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

wallace added a comment. @jingham, friendly ping :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93874/new/ https://reviews.llvm.org/D93874 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http

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

wallace added inline comments. Comment at: lldb/test/API/functionalities/exec/TestExec.py:96 # Run and we should stop due to exec process.Continue() clayborg wrote: > Do we need this continue if we did the step above? How does this test still

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

clayborg added a comment. Walter and I identified this at work, definitely want Jim to chime in on this. Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2614-2616 m_thread_list_real.Clear(); m_thread_list.Clear(); +m_thread_plans.Clear();

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

wallace 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)

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

labath added a comment. The new version seems ok-ish to me, but I'm leaving it open as I'd like Jim to weigh in here. The immediate cause of the crash is related to the (dangling?) m_thread pointer inside the completed "step instruction" plan. Normally, this pointer clears itself when the threa

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

wallace updated this revision to Diff 314035. wallace added a comment. nit Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93874/new/ https://reviews.llvm.org/D93874 Files: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/test/AP

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

wallace updated this revision to Diff 313997. wallace added a comment. Updated the description of the diff. It was actually very easy to find a deterministic reproduction of the failure. And the diff is actually minimal now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION htt

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

labath added a comment. It seems Thread plans already have logic to handle "exec" events. It would be better to figure out why that logic is not sufficient instead of

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

wallace updated this revision to Diff 313904. wallace added a comment. improve test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93874/new/ https://reviews.llvm.org/D93874 Files: lldb/include/lldb/Target/Process.h lldb/source/Plugins/Process/

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

wallace created this revision. wallace added reviewers: clayborg, jingham. wallace requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. On Linux, unlike Darwin, after a process performs exec, the thread id doesn't change. This causes any living