Bugzilla – Full Text Bug Listing
| Summary: | [processes] Remote debugged process is not killed on debugger termination | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Ling Wang <ling.5.wang> | ||||||||
| Component: | cdt-debug-dsf-gdb | Assignee: | Pawel Piech <pawel.1.piech> | ||||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | aleherb+eclipse, cdtdoug, dd.general-inbox, fchouinard, marc.khouzam, pawel.1.piech | ||||||||
| Version: | 0 DD 1.1 | Flags: | cdtdoug:
iplog-
pawel.1.piech: review+ marc.khouzam: review? (Randy.Rohrbach) marc.khouzam: review? (ted) fchouinard: review+ aleherb+eclipse: review+ |
||||||||
| Target Milestone: | DD 1.1 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Ling Wang
Created attachment 116231 [details]
my fix
On termination, the DSF-GDB implementation just exit gdb without explicitly killing the debugged process. That works ok with local debug but not with remote debug.
My proposed fix is to send "kill" command on termination before "exit" command, as is done in CDT.
(In reply to comment #1) > Created an attachment (id=116231) [details] > my fix > My proposed fix is to send "kill" command on termination before "exit" command, > as is done in CDT. I don't have a problem with this, but I would like to understand better what is going on. What version of GDB are you using? Are you using gdbserver? When I try this with GDB/gdbserver 6.8, I see the inferior being killed automatically with the -gdb-exit command. The problem then is that the gdbserver remains active. This is a weird situation because we can re-launch DSF-GDB, but the gdbserver will then complain that the program is not running. I never really bothered with this because I got the feeling this functionality was not very consistent between GDB versions. But maybe we can discuss this a bit. I'm looking for what we really want to do here. The options I see are: 1- explicitly tell the gdbserver to exit when the DSF launch terminates (even though the user started it manually.) 2- leave the gdbserver running, since it still accepts connections. However, when the -exec-continue fails, issue an -exec-run to start the program again. Sadly, in some cases, this crashes GDB. Opinions? (In reply to comment #2) > What version of GDB are you using? Are you using gdbserver? I'm using gdb 6.4.90 for ARM target. Yes, I'm using gdbserver running on remote device. > > When I try this with GDB/gdbserver 6.8, I see the inferior being killed > automatically with the -gdb-exit command. > > The problem then is that the gdbserver remains active. > This is a weird situation because we can re-launch DSF-GDB, but the gdbserver > will then complain that the program is not running. > > I never really bothered with this because I got the feeling this functionality > was not very consistent between GDB versions. > > But maybe we can discuss this a bit. I'm looking for what we really want to do > here. The options I see are: > 1- explicitly tell the gdbserver to exit when the DSF launch terminates (even > though the user started it manually.) > 2- leave the gdbserver running, since it still accepts connections. However, > when the -exec-continue fails, issue an -exec-run to start the program again. > Sadly, in some cases, this crashes GDB. > > Opinions? I definitely prefer the option #1. First, the gdb & gdbserver behavior is not consistent between versions. Second and most important, having one gdbserver instance to server different GDB connections is too risky and error prone. We need to first make sure fresh GDB & GDBserver session works well:) > (In reply to comment #3) > (In reply to comment #2) > > What version of GDB are you using? Are you using gdbserver? > I'm using gdb 6.4.90 for ARM target. Yes, I'm using gdbserver running on remote > device. Wow... We are only using GDB 6.6 and later. However, if it works for you, I guess it doesn't hurt to use an older GDB. > > 1- explicitly tell the gdbserver to exit when the DSF launch terminates (even > > though the user started it manually.) > > 2- leave the gdbserver running, since it still accepts connections. However, > > when the -exec-continue fails, issue an -exec-run to start the program again. > > Sadly, in some cases, this crashes GDB. > > > > Opinions? > I definitely prefer the option #1. First, the gdb & gdbserver behavior is not > consistent between versions. Second and most important, having one gdbserver > instance to server different GDB connections is too risky and error prone. We > need to first make sure fresh GDB & GDBserver session works well:) Can you try this: instead of sending 'kill' as your patch did, send 'monitor exit' instead. This is what I would need to do for GDB 6.8. Let me know if it works for you. Thanks (In reply to comment #4) > Wow... We are only using GDB 6.6 and later. Ok, I think my "gdb --version" command gave me wrong data. The debian package info says it's 6.6, and my gdbserver is 6.6. > Can you try this: > instead of sending 'kill' as your patch did, send 'monitor exit' instead. > This is what I would need to do for GDB 6.8. Let me know if it works for you. > > Thanks > Hmm, the "monitor exit" command is not supported by my gdbserver. 990,838 &"monitor exit\n" 990,845 &"Target does not support this command.\n" BTW, FYI, in my env, without the "kill" command, the "exit" command will kill both gdb and gdbserver but leaving the inferior running. I'm surprised to see that in your case it kills gdb and inferior but leaves gdbserver running. Hi, Marc, is it ok to commit my fix ? Thanks. Created attachment 117232 [details]
Suggested fix
Try this patch instead. The problem you were having was probably caused by the fact that we were using
-target-select extended-remote
instead of
-target-select remote
even for a normal GDB server.
The problem with this patch is that it adds two methods to the API. I'll need to get approval. But let's first confirm it works for you.
BTW, I tested this suggested fix with 6.6, 6.7 6.8 and HEAD. Looks good on my side. (In reply to comment #7) Marc, I tried it and it does not work for me. From the MI tracing I can see this: ... 574,316 7-target-select remote 192.168.2.15:1234 ... 594,225 42monitor exit 594,227 &"monitor exit\n" 594,233 &"Target does not support this command.\n" 594,233 42^error,msg="Target does not support this command." 594,233 (gdb) 594,237 43-gdb-exit 594,244 43^exit (In reply to comment #9) > (In reply to comment #7) > Marc, I tried it and it does not work for me. From the MI tracing I can see > this: > ... > 574,316 7-target-select remote 192.168.2.15:1234 > ... > 594,225 42monitor exit > 594,227 &"monitor exit\n" > 594,233 &"Target does not support this command.\n" This new patch does not use "monitor exit". You probably have the old patch still applied. The only thing this new patch does is use remote instead of extended-remote. Let me know. (In reply to comment #10) Sorry I misunderstood you. Yes, the new patch works for me ! Thanks. I would like to commit this small fix. However, it adds two methods to the MI command file MITargetSelect.java while deprecating the two original ones (the deprecation is not necessary, but I felt it was more appropriate to prepare for a cleanup for 2.0). This is a change in the API after the API freeze so I need a vote. Please let me know if you approve. I've added all committers in the review list. Thanks +1 That's fine with me. Looks good to me. Oh, Marc, I think there's an error (a typo) in your patch for the case of serial connection:
public MITargetSelect(IDMContext ctx, String serialDevice, boolean extended) {
super(ctx, "-target-select extended-remote", ...
}
where the super() should be
super(ctx, "-target-select", ...
(In reply to comment #15) > where the super() should be > super(ctx, "-target-select", ... Woops! Thanks for catching this. Hi Marc, According to our ramp down policy you have plenty of approval to commit this change. Please go ahead. Created attachment 117776 [details]
Final fix
Committed this fix.
Fixed. Pawel, can you verify? I reviewed the changes, although I don't think they made it into RC1. Also I removed the NON-NLS comment from the strings that need to be externalized. See bug 252068. Marking verified. (In reply to comment #20) > I reviewed the changes, although I don't think they made it into RC1. How come the RC1 build was a day early? > Also I > removed the NON-NLS comment from the strings that need to be externalized. See > bug 252068. I saw that bug, but I didn't understand what it really meant... I guess we can talk about it in a future meeting. (In reply to comment #22) > (In reply to comment #20) > > I reviewed the changes, although I don't think they made it into RC1. > > How come the RC1 build was a day early? I'm sorry I should have communicated this. I asked Ted to kick off the build last night so we could test it today. But he didn't email the list as usual so you guys didn't know about it. In the next couple of weeks I'll be more clear about when we perform the candidate build. > I saw that bug, but I didn't understand what it really meant... I guess we can > talk about it in a future meeting. Except for internal error messages we have an obligation to externalize all user-facing strings. (In reply to comment #23) > > How come the RC1 build was a day early? > I'm sorry I should have communicated this. I asked Ted to kick off the build > last night so we could test it today. But he didn't email the list as usual so > you guys didn't know about it. In the next couple of weeks I'll be more clear > about when we perform the candidate build. No problem. DD 1.1 reelased! |