Community
Participate
Working Groups
GDB does not issue any thread started or thread exited events through the MI protocol. Instead, the GDB debugger integration needs to rely on CLI interface notifications and on periodically issuing the -thread-list-ids commands. This functionality should be implemented in the GDBRunControl module which extends the standard MIRunControl. See bug 160038 (comments 26 and 27) for more details.
(In reply to comment #31) > (In reply to comment #30) > > - Why reset the cache after StartedDMEvent and ExitedDMEvent? I guess it > makes sense to clean up the cache for the thread that exited, but I can't think > of a reason for the other cases. > For thread exit, its okie to only clean the cache for the thread that exited. > But for creation we don't have a straight way today. We can discuss this. > I just figured that when a new thread is created there is no cached data in its context yet. So there should be no need to reset anything :-) One exception to this is the -thread-list-ids and -info-threads. Both of these commands are executed through the standard command cache now but they are specifically affected by threads being created/exiting. Perhaps we should add another command to the MICommandCache interface: reset(ICommand<?> cmd) to reset a specific command. Then we could still use the cache, but we could reset these two commands on every start/exit event. Then again, I may be making too big a deal out of this, since threads will only be created/destroyed while the target is running, and a stopped event will cause the cache to reset anyway. So maybe just reseting everything is fine. > While doing this I stumbled into another GDB related problem. > -thread-list-ids : Lists thread number assigned by GDB > info threads: displays > 1. the thread number assigned by GDB > 2. the target system's thread identifier (systag) > 3. the current stack frame summary for that thread > > Thread created and Exited event: > ~"[New Thread 1084554160 (LWP 6924)]\n" > ~"[Thread 1078250416 (LWP 6852) exited]\n" > These events don't report thread number assigned by GDB on a Linux system > > So we will have mismatch in context creation. For getExecutionContext() we are > using GDB assigned thread ID as we don't have info about system's thread id and > for events we don't have the info about GDB assigned thread id so we are using > system thread id. > > This will also be a problem when clearing cache corresponding to a context for > ThreadExited events. > > Veenu > This is definitely a problem. Without thread-id's the started/exited events are almost useless :-( What you could do is: - For started events, you could guess the thread-id. GDB assigns the thread-ids sequentially, so it shouldn't be too hard :-) - For exited events, you could match up the OS ID from the exited event, with results from "info threads", or from thread-create event. This is definitely error prone and complicated. The alternative is to just poll -thread-list-ids and only issue the ExitedDMEvent when the exited thread shows up there. In either case, you will not be able to solve this problem in GDBControl. So the DSFMIThreadExitEvent/DsfMIThreadCreateEvent should return INVALID_ID for getId(). The handling of these events and all the hacks should go into GDBRunControl.
> > (In reply to comment #30) > > > - Why reset the cache after StartedDMEvent and ExitedDMEvent? I guess it > makes sense to clean up the cache for the thread that exited, but I can't think > of a reason for the other cases. > > For thread exit, its okie to only clean the cache for the thread that exited. > > But for creation we don't have a straight way today. We can discuss this. > > > > I just figured that when a new thread is created there is no cached data in its > context yet. So there should be no need to reset anything :-) > > One exception to this is the -thread-list-ids and -info-threads. Both of these > commands are executed through the standard command cache now but they are > specifically affected by threads being created/exiting. > > Perhaps we should add another command to the MICommandCache interface: > reset(ICommand<?> cmd) to reset a specific command. Then we could still use > the cache, but we could reset these two commands on every start/exit event. > Then again, I may be making too big a deal out of this, since threads will only > be created/destroyed while the target is running, and a stopped event will > cause the cache to reset anyway. So maybe just reseting everything is fine. Yes exactly. I also thought about reset method just for a command. But you are right that at target stop and start cache will reset anyways. I was thinking that we are clearing whole cache in containerSuspendEvent, for the STEP. Can't STEP be made more efficient in ContainerSuspendEvent by only clearing out the context in action ie context for which other views are being updated. accompanied by resetting a command for thread creation/exit. Anyways, I feel it's good to have such a method. I am almost sure we will find a use case for it :) Veenu
(In reply to comment #2) > I was thinking that we are clearing whole cache in containerSuspendEvent, for > the STEP. Can't STEP be made more efficient in ContainerSuspendEvent by only > clearing out the context in action ie context for which other views are being > updated. > accompanied by resetting a command for thread creation/exit. I'm not sure if completely understand your question. But in any case, if a whole container is suspended, then we need to reset the cache for the whole container. If we were to reset just for the thread that was resumed, then the data about other threads may be incorrect.
(In reply to comment #3) > (In reply to comment #2) > > I was thinking that we are clearing whole cache in containerSuspendEvent, for > > the STEP. Can't STEP be made more efficient in ContainerSuspendEvent by only > > clearing out the context in action ie context for which other views are being > > updated. > > accompanied by resetting a command for thread creation/exit. > > I'm not sure if completely understand your question. > But in any case, if a whole container is suspended, then we need to reset the > cache for the whole container. If we were to reset just for the thread that > was resumed, then the data about other threads may be incorrect. > Forget it. I got a bit carried away :) Sorry!! In any case, we don't fetch the data until it is visible. Bad question.
Created attachment 77366 [details] Handling of thread create and exit event for GDB Hi Patch now handles the right thread id's for thread create event and thread exit event and the time gap problem in thread exit event. For exit event, I have removed the event generation from GDBEventProcessor class. Now it is done in GDBRunControl where we compare the execution DMCs and event is generated with the right thread id, if needed. Generating event here also takes care of the time gap between the thread exit event and actually when the thread exits. Veenu
(In reply to comment #5) I looked over the patch and I have only a couple of question :-) 1. Why create an access method MICommandCache.getCachedContext()? It seems to me that its an internal detail to the cache implementation, and even part of the returned type CommandInfo, is declared as package-private. 2. It seems like you went part way in implementing one method of generating thread-exited event: you added a the GdbProcesses.getThreadIdsMapping() method, but you don't use it anywhere. Was this an un-finished thought?
(In reply to comment #6) > 1. Why create an access method MICommandCache.getCachedContext()? It seems to > me that its an internal detail to the cache implementation, and even part of > the returned type CommandInfo, is declared as package-private. Had to do it for Junit TC. We will have to expose CommandInfo for other TCs. > > 2. It seems like you went part way in implementing one method of generating > thread-exited event: you added a the GdbProcesses.getThreadIdsMapping() > method, but you don't use it anywhere. Was this an un-finished thought? Its a finished thought but with some left overs which I forgot to clean. Its not needed. Thanx for noticing. Can you remove the lines (fMapThreadIds is not needed at all) or should I send another patch ? Do you think there is anything else that needs to be done for this bug ? Veenu
(In reply to comment #7) > (In reply to comment #6) > > 1. Why create an access method MICommandCache.getCachedContext()? It seems to > > me that its an internal detail to the cache implementation, and even part of > > the returned type CommandInfo, is declared as package-private. > Had to do it for Junit TC. We will have to expose CommandInfo for other TCs. This is probably not a good idea. I've seen a few places in Eclipse in the past where access methods were added in objects for purpose of testing. IMO, it's a rather dangerous path to take. I found the test that uses this function (from the other patch) and I think it would be better to just retrieve a value from the back end and make sure that it it's not the old value (i.e. that it didn't come from the cache). The less that the test depends on the internals of the service, the less likely its going to need to be changed in the future. > Can you remove the lines (fMapThreadIds is not needed at all) or should I send > another patch ? I was hoping that I could wait so you could check in the change yourself :-) > Do you think there is anything else that needs to be done for this bug ? I'm only a little worried that the user won't know that a thread has exited until he refreshes. But I guess that's just the limitation of current GDB.
(In reply to comment #8) > > > 1. Why create an access method MICommandCache.getCachedContext()? It seems to > > > me that its an internal detail to the cache implementation, and even part of > > > the returned type CommandInfo, is declared as package-private. > > Had to do it for Junit TC. We will have to expose CommandInfo for other TCs. > > This is probably not a good idea. I've seen a few places in Eclipse in the > past where access methods were added in objects for purpose of testing. IMO, > it's a rather dangerous path to take. > I found the test that uses this function (from the other patch) and I think it > would be better to just retrieve a value from the back end and make sure that > it it's not the old value (i.e. that it didn't come from the cache). The less > that the test depends on the internals of the service, the less likely its > going to need to be changed in the future. Point taken. Could be true. > > > Can you remove the lines (fMapThreadIds is not needed at all) or should I send > > another patch ? > I was hoping that I could wait so you could check in the change yourself :-) Haven't got the password yet :( > > Do you think there is anything else that needs to be done for this bug ? > I'm only a little worried that the user won't know that a thread has exited > until he refreshes. But I guess that's just the limitation of current GDB. In that case, Can't we have 2 different events ? ie create another event for handling actual exit of thread. Hence we will have one event for reporting when GDB raises an event and a new one can be raised when thread actually exits. This way we will have different handlers and the same event wont be called twice, if thats the concern Veenu
Created attachment 77640 [details] Thread exit event handling for GDB (+committed, +logged) Hi Pawel, I think there is no point in waiting for my account information. You can apply this patch. I have removed the extras and also this patch contains the changes you made for the terminate problem. I couldn't test it though. Veenu
Comment on attachment 77640 [details] Thread exit event handling for GDB (+committed, +logged) I agree... I committed the patch pretty much as is.
(In reply to comment #9) > > > Do you think there is anything else that needs to be done for this bug ? > > I'm only a little worried that the user won't know that a thread has exited > > until he refreshes. But I guess that's just the limitation of current GDB. > In that case, Can't we have 2 different events ? ie create another event for > handling actual exit of thread. > Hence we will have one event for reporting when GDB raises an event and a new > one can be raised when thread actually exits. This way we will have different > handlers and the same event wont be called twice, if thats the concern No I don't think it's a good idea to complicate the standard interface definition to work around bugs in a specific back end. If need be, the UI could be extended to have GDB-specific behavior and to use events declared in GDBRunControl. In this case I'm not sure if it would help any, it's really a GDB problem and that's probably where it would be easiest to address it. For this bug, as soon as you achieve satisfactory behavior, you can mark it as fixed. If new issues come up later, we'll open separate bugs for them.
Created attachment 77786 [details] GDB Event processing (+committed, +logged) Hi Pawel Please apply this class once more. In order to cleanse the code, I cleansed a valid statement too. Running Junit TCs revealed that :) Veenu
Comment on attachment 77786 [details] GDB Event processing (+committed, +logged) I applied the change.
Moving back to the DD inbox.
I believe this bug is complete. Not sure why it is still open. New =thread-created/exited events are part of non-stop but that work is tracked by the non-stop bugs. Pawel, I guess you can verify.
This was fixed in an old release, marking as closed.
