[Lldb-commits] [PATCH] D93874: [process] fix exec support on Linux
Walter via lldb-commits
lldb-commits at lists.llvm.org
Thu Jan 21 16:28:09 PST 2021
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()) {
should_stop = current_plan->ShouldStop(event_ptr);
```
The interesting part of this call-stack starts
at Process::HandlePrivateEvent, which seems to be handling the Stop Event.
Where I think there's some inconsistent code is in
ThreadPlanBase::ShouldStop, because it tries to get the stop reason, which
fails for Linux, and then it processes eStopReasonExec a few lines below.
And see what it does:
```
case eStopReasonExec:
// If we crashed, discard thread plans and stop. Don't force the
// discard, however, since on rerun the target may clean up this
// exception and continue normally from there.
LLDB_LOGF(
log,
"Base plan discarding thread plans for thread tid = 0x%4.4" PRIx64
" (exec.)",
m_tid);
GetThread().DiscardThreadPlans(false);
return true;
```
It does discard the thread plans for that thread! This makes me believe
that it should be fine to delete the thread plans in the first place. I
wasn't able to figure out more, but I can dig deeper if you give me a few
pointers. In any case, this last code of block makes me believe that
deleting the thread plans or reseting the thread pointer in the base thread
plan might be fine.
Btw, this is a bunch of logs I got, which start at a breakpoint, then
there's a "continue", which triggers what I'm saying. The backtrace is below
(lldb) bt
* thread #1, name = 'runner', stop reason = instruction step into
* frame #0: 0x00007f1fe674838d libc.so.6`raise + 61
frame #1: 0x0000000000400a3c runner`main(argc=2,
argv=0x00007fff4b78fc08) at runner.cpp:20
frame #2: 0x00007f1fe6734555 libc.so.6`__libc_start_main + 245
frame #3: 0x0000000000400919 runner`_start + 41
(lldb) thread plan list /////////////////////////////////////// See that
only the base plan is there
thread #1: tid = 0x2b72f1:
Active plan stack:
Element 0: Base thread plan.
Completed plan stack:
Element 0: Stepping one instruction past 0x00007f1fe6748387 stepping
into calls
Element 1: Stepping one instruction past 0x00007f1fe6748387 stepping
into calls
(lldb) c
lldb Process::Resume -- locking run lock
lldb Process::PrivateResume() m_stop_id = 3, public state:
stopped private state: stopped
lldb WillResume Thread #1 (0x0x7fcd30000da0): tid = 0x2b72f1,
pc = 0x7f1fe674838d, sp = 0x7fff4b78fad8, fp = 0x7fff4b78fb20, plan = 'base
plan', state = running, stop others = 0
lldb Resuming thread: 2b72f1 with state: running.
lldb 0x4e7020
Listener::Listener('gdb-remote.resume-packet-sent')
lldb 0x4e7020 Listener::StartListeningForEvents (broadcaster =
0x4787a8, mask = 0x00010000) acquired_mask = 0x00010000 for
gdb-remote.resume-packet-sent
lldb 0x4e7020 Listener::StartListeningForEvents (broadcaster =
0x478ff8, mask = 0x00000004) acquired_mask = 0x00000004 for
gdb-remote.resume-packet-sent
lldb 0x494ab0
Broadcaster("lldb.process.gdb-remote.async-broadcaster")::BroadcastEvent
(event_sp = {0x7fcd40007cd0 Event: broadcaster = 0x478ff8
(lldb.process.gdb-remote.async-broadcaster), type = 0x00000001 (async
thread continue), data = {"c"}}, unique =0) hijack = (nil)
lldb 0x494bf0
Listener('lldb.process.gdb-remote.async-listener')::AddEvent (event_sp =
{0x7fcd40007cd0})
lldb this = 0x00000000004E7020, timeout = 5000000 us for
gdb-remote.resume-packet-sent
b-remote.async> 0x494bf0 'lldb.process.gdb-remote.async-listener'
Listener::FindNextEventInternal(broadcaster=(nil),
broadcaster_names=(nil)[0], event_type_mask=0x00000000, remove=1) event
0x7fcd40007cd0
b-remote.async> Process::SetPrivateState (running)
b-remote.async> 0x483f30
Broadcaster("lldb.process.internal_state_broadcaster")::BroadcastEvent
(event_sp = {0x7fcd40005380 Event: broadcaster = 0x477dd0
(lldb.process.internal_state_broadcaster), type = 0x00000001, data = {
process = 0x477ce0 (pid = 2847473), state = running}}, unique =0) hijack =
(nil)
b-remote.async> 0x4841b0
Listener('lldb.process.internal_state_listener')::AddEvent (event_sp =
{0x7fcd40005380})
b-remote.async> 0x485cf0 Broadcaster("gdb-remote.client")::BroadcastEvent
(event_sp = {0x7fcd40005500 Event: broadcaster = 0x4787a8
(gdb-remote.client), type = 0x00010000, data = <NULL>}, unique =0) hijack =
(nil)
b-remote.async> 0x4e7020
Listener('gdb-remote.resume-packet-sent')::AddEvent (event_sp =
{0x7fcd40005500})
intern-state 0x4841b0 'lldb.process.internal_state_listener'
Listener::FindNextEventInternal(broadcaster=(nil),
broadcaster_names=(nil)[0], event_type_mask=0x00000000, remove=1) event
0x7fcd40005380
lldb 0x4e7020 'gdb-remote.resume-packet-sent'
Listener::FindNextEventInternal(broadcaster=(nil),
broadcaster_names=(nil)[0], event_type_mask=0x00000000, remove=1) event
0x7fcd40005500
intern-state Current Plan for thread 1(0x7fcd30000da0) (0x2b72f1,
running): base plan being asked whether we should report run.
lldb 0x4e7020 Listener::Clear('gdb-remote.resume-packet-sent')
intern-state Process::ShouldBroadcastEvent (0x7fcd40005380) => new
state: running, last broadcast state: running - YES
lldb 0x4e7020
Listener::~Listener('gdb-remote.resume-packet-sent')
intern-state Process::HandlePrivateEvent (pid = 2847473) broadcasting
new state running (old state stopped) to public
lldb Process thinks the process has resumed.
intern-state Process::HandlePrivateEvent updated m_iohandler_sync to 3
Process 2847473 resuming
intern-state 0x483df0 Broadcaster("lldb.process")::BroadcastEvent
(event_sp = {0x7fcd40005380 Event: broadcaster = 0x477d18 (lldb.process),
type = 0x00000001 (state-changed), data = { process = 0x477ce0 (pid =
2847473), state = running}}, unique =0) hijack = (nil)
(lldb) intern-state 0x358b00 Listener('lldb.Debugger')::AddEvent
(event_sp = {0x7fcd40005380})
intern-state timeout = <infinite>, event_sp)...
intern-state this = 0x00000000004841B0, timeout = <infinite> for
lldb.process.internal_state_listener
dbg.evt-handler 0x358b00 'lldb.Debugger'
Listener::FindNextEventInternal(broadcaster=(nil),
broadcaster_names=(nil)[0], event_type_mask=0x00000000, remove=1) event
0x7fcd40005380
dbg.evt-handler Process::SetPublicState (state = running, restarted = 0)
dbg.evt-handler this = 0x0000000000358B00, timeout = <infinite> for
lldb.Debugger
/////////////////////////////////////////////////////////////// This is
where we start handling the stop, see the ~Thread call below
b-remote.async> this = 0x00007FCD30000DA0, pid = 2847473, tid = 2847473
b-remote.async> 0x7fcd30000da0 Thread::~Thread(tid = 0x2b72f1)
b-remote.async> 0x00007FCD30000DD8 Broadcaster::~Broadcaster("lldb.thread")
b-remote.async> Process::SetPrivateState (stopped)
b-remote.async> Process::SetPrivateState (stopped) stop_id = 4
b-remote.async> 0x483f30
Broadcaster("lldb.process.internal_state_broadcaster")::BroadcastEvent
(event_sp = {0x7fcd3c0736f0 Event: broadcaster = 0x477dd0
(lldb.process.internal_state_broadcaster), type = 0x00000001, data = {
process = 0x477ce0 (pid = 2847473), state = stopped}}, unique =0) hijack =
(nil)
b-remote.async> 0x4841b0
Listener('lldb.process.internal_state_listener')::AddEvent (event_sp =
{0x7fcd3c0736f0})
b-remote.async> this = 0x0000000000494BF0, timeout = <infinite> for
lldb.process.gdb-remote.async-listener
intern-state 0x4841b0 'lldb.process.internal_state_listener'
Listener::FindNextEventInternal(broadcaster=(nil),
broadcaster_names=(nil)[0], event_type_mask=0x00000000, remove=1) event
0x7fcd3c0736f0
intern-state 0x7fcd302ac330
Listener::Listener('Communication::SyncronizeWithReadThread')
intern-state 0x7fcd302ac330 Listener::StartListeningForEvents
(broadcaster = 0x478228, mask = 0x00000020) acquired_mask = 0x00000020 for
Communication::SyncronizeWithReadThread
intern-state 0x7fcd302ac330
Listener::Clear('Communication::SyncronizeWithReadThread')
intern-state 0x7fcd302ac330
Listener::~Listener('Communication::SyncronizeWithReadThread')
intern-state 0x00007FCD302BC738 Broadcaster::Broadcaster("lldb.thread")
///////////////////////////////////////////////////// This is where the new
thread is created, but it points to the same tid
intern-state 0x7fcd302bc700 Thread::Thread(tid = 0x2b72f1)
intern-state 0x358b00 Listener::StartListeningForEvents (broadcaster =
0x7fcd302bc738, mask = 0x00000011) acquired_mask = 0x00000011 for
lldb.Debugger
intern-state this = 0x00007FCD302BC700, pid = 2847473, tid = 2847473
intern-state 0x7fcd302bc700: tid = 0x2b72f1: stop info = <NULL>
(stop_id = 4)
intern-state 0x7fcd302bc700: tid = 0x2b72f1: stop info = exec (stop_id
= 4)
intern-state
intern-state ThreadList::ShouldStop: 1 threads, 1 unsuspended threads
intern-state Thread::ShouldStop(0x7fcd302bc700) for tid = 0x2b72f1
0x2b72f1, pc = 0x00007f3b751ce140
intern-state ^^^^^^^^ Thread::ShouldStop Begin ^^^^^^^^
intern-state Plan stack initial state:
thread #1: tid = 0x2b72f1:
Active plan stack:
Element 0: Base thread plan.
intern-state Plan base plan explains stop, auto-continue 0.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash
backtrace.
Stack dump without symbol names (ensure you have llvm-symbolizer in your
PATH or set the environment var `LLVM_SYMBOLIZER_PATH` to point to it):
./bin/lldb[0x298aca]
./bin/lldb[0x298ca4]
./bin/lldb[0x296ca0]
./bin/lldb[0x29a606]
/usr/local/fbcode/platform007/lib/libpthread.so.0(+0x12b80)[0x7fcd53167b80]
/home/wallace/llvm-sand/build/Debug/Linux-x86_64/llvm/bin/../lib/liblldb.so.12(+0x47e5d96)[0x7fcd4c955d96]
/home/wallace/llvm-sand/build/Debug/Linux-x86_64/llvm/bin/../lib/liblldb.so.12(+0x47e595a)[0x7fcd4c95595a]
/home/wallace/llvm-sand/build/Debug/Linux-x86_64/llvm/bin/../lib/liblldb.so.12(+0x47cf23b)[0x7fcd4c93f23b]
/home/wallace/llvm-sand/build/Debug/Linux-x86_64/llvm/bin/../lib/liblldb.so.12(+0x47e006c)[0x7fcd4c95006c]
/home/wallace/llvm-sand/build/Debug/Linux-x86_64/llvm/bin/../lib/liblldb.so.12(+0x47253f5)[0x7fcd4c8953f5]
/home/wallace/llvm-sand/build/Debug/Linux-x86_64/llvm/bin/../lib/liblldb.so.12(+0x4720f01)[0x7fcd4c890f01]
/home/wallace/llvm-sand/build/Debug/Linux-x86_64/llvm/bin/../lib/liblldb.so.12(+0x472676e)[0x7fcd4c89676e]
/home/wallace/llvm-sand/build/Debug/Linux-x86_64/llvm/bin/../lib/liblldb.so.12(+0x47257ed)[0x7fcd4c8957ed]
/home/wallace/llvm-sand/build/Debug/Linux-x86_64/llvm/bin/../lib/liblldb.so.12(+0x45856ec)[0x7fcd4c6f56ec]
/usr/local/fbcode/platform007/lib/libpthread.so.0(+0x76ce)[0x7fcd5315c6ce]
/usr/local/fbcode/platform007/lib/libc.so.6(clone+0x3f)[0x7fcd46d6135f]
Segmentation fault (core dumped)
//// This is the callstack I got from another run, but they are the same
* thread #4, name = 'intern-state', stop reason = signal SIGSEGV: invalid
address (fault address: 0x218)
* frame #0: 0x00007f9983cccd96
liblldb.so.12`lldb_private::ThreadPlan::GetPrivateStopInfo(this=0x00007f99680014d0)
at ThreadPlan.h:521:24
frame #1: 0x00007f9983ccc95a
liblldb.so.12`lldb_private::ThreadPlanBase::ShouldStop(this=0x00007f99680014d0,
event_ptr=0x00007f997404c310) at ThreadPlanBase.cpp:78:29
frame #2: 0x00007f9983cb623b
liblldb.so.12`lldb_private::Thread::ShouldStop(this=0x00007f99682be2b0,
event_ptr=0x00007f997404c310) at Thread.cpp:871:35
frame #3: 0x00007f9983cc706c
liblldb.so.12`lldb_private::ThreadList::ShouldStop(this=0x0000000000477fa8,
event_ptr=0x00007f997404c310) at ThreadList.cpp:330:48
frame #4: 0x00007f9983c0c3f5
liblldb.so.12`lldb_private::Process::ShouldBroadcastEvent(this=0x0000000000477ce0,
event_ptr=0x00007f997404c310) at Process.cpp:3368:40
frame #5: 0x00007f9983c07f01
liblldb.so.12`lldb_private::Process::HandlePrivateEvent(this=0x0000000000477ce0,
event_sp=std::__shared_ptr<lldb_private::Event,
__gnu_cxx::_S_atomic>::element_type @ 0x00007f997404c310) at
Process.cpp:3593:33
frame #6: 0x00007f9983c0d76e
liblldb.so.12`lldb_private::Process::RunPrivateStateThread(this=0x0000000000477ce0,
is_secondary_thread=false) at Process.cpp:3787:7
frame #7: 0x00007f9983c0c7ed
liblldb.so.12`lldb_private::Process::PrivateStateThread(arg=0x0000000000494d80)
at Process.cpp:3685:25
frame #8: 0x00007f9983a6c6ec
liblldb.so.12`lldb_private::HostNativeThreadBase::ThreadCreateTrampoline(arg=0x00000000004982e0)
at HostNativeThreadBase.cpp:68:10
frame #9: 0x00007f998a4d36ce
libpthread.so.0`start_thread(arg=0x00007f996f7fe700) at
pthread_create.c:465:7
frame #10: 0x00007f997e0d835f libc.so.6`__clone at clone.S:95
(lldb) ^D
Il giorno gio 21 gen 2021 alle ore 14:37 Jim Ingham <jingham at apple.com> ha
scritto:
>
>
> > On Jan 21, 2021, at 2:33 PM, Jim Ingham <jingham at apple.com> wrote:
> >
> >
> >
> >> On Jan 21, 2021, at 12:51 PM, walter erquinigo via Phabricator <
> reviews at reviews.llvm.org> 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
> pointer
> >> - During the exec, the original thread is destroyed in
> ProcessGDBRemote::SetLastStopPacket, and later a new Thread pointer is
> created for the same tid. Notice that the base thread plan is still holding
> a reference to the previous pointer even though there's a new Thread
> pointer for the same tid. On Linux, exec preserves the tid. This step is
> where I think there's a wrong invariant.
> >> - Then, at some point, the base thread plan is asked its stop reason,
> and it'll try to use the destroyed pointer, thus crashing.
> >
> > Thanks for looking at this some more.
> >
> > Sadly, I think I'm still missing something. Here's how I think things
> should happen:
> >
> > 1) The program was stopped somewhere in the original program (before
> exec), and the user continued the process
> > a) WillResume is called and all the thread plans clear their cached
> thread pointers. So they will have to look them up by ID when next asked a
> question.
> > b) Then the process resumes.
> > 2) We get a stop packet with a reason of "exec" from the stub.
> > a) We process the stop packet, and get the new thread list
> > b) We start to build the new thread list
>
> Ack, that was confusing. Item b) should be "We start to build the new
> ThreadList". The object in a) is the thread list the stub tells about,
> either in the stop packet or through subsequent queries. The object in b)
> is the ThreadList in the Process that will describe the state at this
> particular stop point.
>
> > 1) From what you are saying, in the case of exec, regardless of
> whether the TID matches an extant one or not, we make a new Thread *, put
> it into the new ThreadList and delete the old Thread *.
> > c) Then we start figuring out what to do after the stop.
> > i) If the TID of the new thread is the same as any of the TID's in
> the old thread list (and thus of the ThreadPlanStack that was managing it)
> when the new thread gets asked what to do, it will route the question to
> the ThreadPlanStack for that TID.
> > ii) If this happens after the new thread list is in place,
> FindThreadByID will return the new, valid thread, and there should be no
> problem.
> >
> > So for this to fail as you described, somewhere between the time that we
> stopped for exec and the time we finalized the new thread list and
> discarded the old threads, somebody must have asked the thread plan stack
> for that TID a question that triggered it to cache the old thread's Thread
> *.
> >
> > That really shouldn't happen, precisely for the reason you see here.
> When we stop, we have to finish setting up the current ThreadList before we
> start asking questions of the ThreadPlanStack for threads, like ShouldStop
> or whatever. It makes no sense to ask the pre-stop thread list questions
> for the current stop, and we shouldn't do it.
> >
> > Do you know where we're calling ThreadPlan::GetThread while the thread
> list still has the old threads in it? I'd really like to stop that if we
> can. And if you can move those queries till after the new thread list is
> established, then we shouldn't need any of the alternatives you suggest.
> >
> > About the alternatives:
> >
> > First one: I'd rather not artificially clear out the ThreadPlanStack
> after exec. After all, if you knew that exec on Linux preserves the TID,
> you could write a thread plan that carried over the exec and did something
> interesting. Be a shame to prevent that for implementation reasons.
> >
> > Second one: Adding the IsValid check won't do any harm, but it
> implicitly lets something happen that I don't think should happen, so it
> doesn't feel right...
> >
> > Third one: If there's some good reason why you have to first check
> through the old thread list for stop reasons, then update the thread list,
> then ask again with no Resume in between, I think the cleanest solution is
> to announce that by explicitly clearing the ThreadPlans' cached thread
> pointer. So I think this one's the best option. But again, I'd like to
> understand why it was necessary.
> >
> > Jim
> >
> >
> >>
> >> I've found three possible fixes for this (I'm pretty sure that you can
> come up with more):
> >>
> >> - The first one is what this diff does, which is clearing all the
> thread plans once the gdbremote client knows there's an exec.
> >> - The second one is modifying the ThreadPlan::GetThread method, which
> currently looks like this
> >>
> >> if (m_thread)
> >> return *m_thread;
> >>
> >> ThreadSP thread_sp = m_process.GetThreadList().FindThreadByID(m_tid);
> >> m_thread = thread_sp.get();
> >> return *m_thread;
> >>
> >> Changing it into
> >>
> >> if (m_thread && m_thread->IsValid()) // IsValid will return false for a
> deleted thread
> >> return *m_thread;
> >>
> >> ThreadSP thread_sp = m_process.GetThreadList().FindThreadByID(m_tid);
> >> m_thread = thread_sp.get();
> >> return *m_thread;
> >>
> >> also works, as it will make the ThreadPlan grab the new thread pointer.
> >>
> >> - Another solution is to create a method ThreadPlan::PrepareForExec()
> which just clears the m_thread pointer in the the thread plan, and call it
> from ProcessGDBRemote::SetLastStopPacket, where we handle the exec.
> >>
> >> I prefer the first solution, as the thread plans are all cleared in
> preparation for the new process.
> >>
> >>
> >> Repository:
> >> rG LLVM Github Monorepo
> >>
> >> CHANGES SINCE LAST ACTION
> >> https://reviews.llvm.org/D93874/new/
> >>
> >> https://reviews.llvm.org/D93874
> >>
> >
>
>
--
- Walter ErquÃnigo Pezo
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20210121/e88e110c/attachment-0001.html>
More information about the lldb-commits
mailing list