* Questions on commit 6c95b8df7fef5273da71c34775918c554aae0ea8 @ 2014-09-20 18:39 Doug Evans 2014-09-26 16:51 ` Pedro Alves 0 siblings, 1 reply; 6+ messages in thread From: Doug Evans @ 2014-09-20 18:39 UTC (permalink / raw) To: gdb-patches, Pedro Alves, Stan Shebs [Ugh, bad To address on first try.] Hi. While looking into removing lwp_list from gdb (akin to what I did for gdbserver) I found that several bits of target code are calling init_thread_list (grep init_thread_list *.c). Maybe there's the odd case where target code would need to do this, but normally when should target code *ever* do this? [ side notes: - having to deal with target code doing common operations (for lack of a better phrase) is something I have to fix in another context (prologue skipping), so it's now a bit of a pet peeve of mine - it's likely there are more such cases (e.g. init_wait_for_inferior), but for the nonce I'm picking init_thread_list as a basis for discussion - these issues are not specific to or introduced by the commit specified in the subject line, it's just a concrete base from which to phrase my questions ] To try to assist you with getting a handle on my confusion, consider remote.c:extended_remote_create_inferior_1 from gdb 6.8: /* Clean up from the last time we were running. */ init_thread_list (); init_wait_for_inferior (); Now look at what's there today: if (!have_inferiors ()) { /* Clean up from the last time we ran, before we mark the target running again. This will mark breakpoints uninserted, and get_offsets may insert breakpoints. */ init_thread_list (); init_wait_for_inferior (); } I think(!) there may be multiple ways to look at all of this as being wrong, so pick your favorite, but here's one way: What does it matter whether there are other inferiors as to whether remote.c:extended_remote_create_inferior has to "clean up from the last time we were running"? Obviously we can't call init_thread_list if there are other inferiors, but why are we calling init_thread_list here at all? Why isn't this state being managed by gdb itself (inf*.c or some such)? I can imagine one can look at this as just being still a work in progress on the way to proper multi-target support. It's stil a bit odd though to have taken this step this way, so I'm hoping for some clarification. Another related question I have is: Why does remote-sim.c:gdbsim_create_inferior call init_wait_for_inferior unconditionally whereas the above code conditions the call on !have_inferiors()? Maybe it's a simple oversight, but I think (emphasis on THINK, I could certainly be missing something) we need to take a step back and ask why this code is there at all. Putting this code in target routines gives us a lot of flexibility, but the cost is more mental load (for lack of a better phrase) and more touch points when trying to fix/improve core gdb, and I'm getting the impression that the pendulum has swung too far towards putting general housekeeping operations in target code. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Questions on commit 6c95b8df7fef5273da71c34775918c554aae0ea8 2014-09-20 18:39 Questions on commit 6c95b8df7fef5273da71c34775918c554aae0ea8 Doug Evans @ 2014-09-26 16:51 ` Pedro Alves 2014-09-30 19:50 ` Doug Evans 0 siblings, 1 reply; 6+ messages in thread From: Pedro Alves @ 2014-09-26 16:51 UTC (permalink / raw) To: Doug Evans, gdb-patches, Stan Shebs On 09/20/2014 07:39 PM, Doug Evans wrote: > [Ugh, bad To address on first try.] > > Hi. > > While looking into removing lwp_list from gdb (akin to what I did for > gdbserver) I found that several bits of target code are calling > init_thread_list > (grep init_thread_list *.c). > Maybe there's the odd case where target code would need to do this, > but normally when should target code *ever* do this? > To try to assist you with getting a handle on my confusion, consider > remote.c:extended_remote_create_inferior_1 from gdb 6.8: > > /* Clean up from the last time we were running. */ > init_thread_list (); > init_wait_for_inferior (); > > Now look at what's there today: > > if (!have_inferiors ()) > { > /* Clean up from the last time we ran, before we mark the target > running again. This will mark breakpoints uninserted, and > get_offsets may insert breakpoints. */ > init_thread_list (); > init_wait_for_inferior (); > } > > I think(!) there may be multiple ways to look at all of this as being > wrong, so pick your favorite, but here's one way: What does it matter > whether there are other inferiors as to whether > remote.c:extended_remote_create_inferior has to "clean up from the > last time we were running"? > > Obviously we can't call init_thread_list if there are other inferiors, > but why are we calling init_thread_list here at all? Why isn't this > state being managed by gdb itself (inf*.c or some such)? I can > imagine one can look at this as just being still a work in progress on > the way to proper multi-target support. It's stil a bit odd though to > have taken this step this way, so I'm hoping for some clarification. Really not sure what sort of answer you're looking for. I don't really recall history on that level of detail, but, before 6c95b8df, the only kind of multi-process GDB supported, was the model used by DICOS. On that platform, we didn't use "run" at all, only "attach" and all programs shared the program space. Well, before 6c95b8df, GDB didn't even have the concept of a program space, but it didn't matter; even now that it does, a single program space is the model that best fits. So when we got to making GDB's multi-process fit for Linux, which was the point of that patch, we must have stumbled on that init_thread_list call in remote.c:extended_remote_create_inferior_1, as soon as we did two "run"s in a row. So the change in that patch just looks like the conservative change. The comment change you show wasn't done by that patch. A trivial git blame 6c95b8df^ points at: commit 45280a5259f209ba74ed8255674a3fd345307a55 Author: Daniel Jacobowitz <drow@false.org> AuthorDate: Thu May 8 16:08:10 2008 +0000 * remote.c (extended_remote_create_inferior_1): Clean up before marking the target running. I didn't dig deeper than that, but git blame further will probably point out something ancient. > Another related question I have is: Why does remote-sim.c:gdbsim_create_inferior > call init_wait_for_inferior unconditionally whereas the above code > conditions the call on !have_inferiors()? Most probably because nobody has ever tried making remote sim work with multiple processes at the same time, so nobody ever stumbled on that. > Maybe it's a simple > oversight, but I think (emphasis on THINK, I could certainly be > missing something) we need to take a step back and ask why this code > is there at all. Putting this code in target routines gives us a lot > of flexibility, but the cost is more mental load (for lack of a better > phrase) and more touch points when trying to fix/improve core gdb, and > I'm getting the impression that the pendulum has swung too far towards > putting general housekeeping operations in target code. Huh? I think you're getting this backwards. You make it sound like we've been adding more of this stuff in target code ("putting"), while instead these are _ancient_ code that over the years we've been cleaning up. Why not just git blame on that one? It shows: 40b92220 (Jim Kingdon 1993-09-17 17:27:43 +0000 467) 8501c742 (Stu Grossman 1996-08-13 00:01:37 +0000 468) gdbsim_kill (); 40b92220 (Jim Kingdon 1993-09-17 17:27:43 +0000 469) remove_breakpoints (); ec25d19b (Steve Chamberlain 1993-01-03 22:36:04 +0000 470) init_wait_for_inferior (); ec25d19b (Steve Chamberlain 1993-01-03 22:36:04 +0000 471) Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Questions on commit 6c95b8df7fef5273da71c34775918c554aae0ea8 2014-09-26 16:51 ` Pedro Alves @ 2014-09-30 19:50 ` Doug Evans 2014-10-03 17:14 ` Pedro Alves 0 siblings, 1 reply; 6+ messages in thread From: Doug Evans @ 2014-09-30 19:50 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Stan Shebs Pedro Alves writes: > On 09/20/2014 07:39 PM, Doug Evans wrote: > > [Ugh, bad To address on first try.] > > > > Hi. > > > > While looking into removing lwp_list from gdb (akin to what I did for > > gdbserver) I found that several bits of target code are calling > > init_thread_list > > (grep init_thread_list *.c). > > Maybe there's the odd case where target code would need to do this, > > but normally when should target code *ever* do this? > > > To try to assist you with getting a handle on my confusion, consider > > remote.c:extended_remote_create_inferior_1 from gdb 6.8: > > > > /* Clean up from the last time we were running. */ > > init_thread_list (); > > init_wait_for_inferior (); > > > > Now look at what's there today: > > > > if (!have_inferiors ()) > > { > > /* Clean up from the last time we ran, before we mark the target > > running again. This will mark breakpoints uninserted, and > > get_offsets may insert breakpoints. */ > > init_thread_list (); > > init_wait_for_inferior (); > > } > > > > > I think(!) there may be multiple ways to look at all of this as being > > wrong, so pick your favorite, but here's one way: What does it matter > > whether there are other inferiors as to whether > > remote.c:extended_remote_create_inferior has to "clean up from the > > last time we were running"? > > > > Obviously we can't call init_thread_list if there are other inferiors, > > but why are we calling init_thread_list here at all? Why isn't this > > state being managed by gdb itself (inf*.c or some such)? I can > > imagine one can look at this as just being still a work in progress on > > the way to proper multi-target support. It's stil a bit odd though to > > have taken this step this way, so I'm hoping for some clarification. > > Really not sure what sort of answer you're looking for. The most succinct way of expressing what I'm looking for that I can think of is that I want to understand this code, and I don't. There are several instances of the !have_inferiors() check, I picked this one because it seemed like as good a choice as any. > I don't really recall history on that level of detail, > but, before 6c95b8df, the only kind of multi-process GDB supported, > was the model used by DICOS. On that platform, we didn't use > "run" at all, only "attach" and all programs shared the program > space. Well, before 6c95b8df, GDB didn't even have the > concept of a program space, but it didn't matter; even now that > it does, a single program space is the model that best fits. > > So when we got to making GDB's multi-process fit for Linux, > which was the point of that patch, we must have stumbled on that > init_thread_list call in remote.c:extended_remote_create_inferior_1, > as soon as we did two "run"s in a row. So the change in that > patch just looks like the conservative change. If this is just a temp hack to play it safe, then that explains it. ["this" being the wrapping of the calls to init_thread_list and init_wait_for_inferior with "if (!have_inferiors ())"] I read what I could from the thread when the patch was submitted and I couldn't find anything. I don't know this code well enough to be comfortable with any assumptions. It's too bad FIXMEs are frowned on in gdb-land. I think this is a good site for one. Assuming this is just a work-in-progress, the code really is confusing. For example, what if have_inferiors() returns TRUE? That means that we won't "Clean up from the last time we ran, ...". The reader is left with questions like "Then what? Is the system left in a broken state because we haven't cleaned up? Or is the cleanup done elsewhere? If so, where? Or do things just not need to be cleaned up in this case? If so, why?" OTOH, if a FIXME isn't appropriate here, and the code is ok as-is (I'm not making a value judgement here, I still have to understand the code first), I'd still like to understand it so I can add a comment to help the next reader (which if it's me a year from now I may well have forgotten this conversation :-)). > The comment change you show wasn't done by that patch. > A trivial git blame 6c95b8df^ points at: > > commit 45280a5259f209ba74ed8255674a3fd345307a55 > Author: Daniel Jacobowitz <drow@false.org> > AuthorDate: Thu May 8 16:08:10 2008 +0000 > > * remote.c (extended_remote_create_inferior_1): Clean up > before marking the target running. > > I didn't dig deeper than that, but git blame further will > probably point out something ancient. Yeah, I went back a bit in the history, but it didn't help me understand the code that's there today. > > Another related question I have is: Why does remote-sim.c:gdbsim_create_inferior > > call init_wait_for_inferior unconditionally whereas the above code > > conditions the call on !have_inferiors()? > > Most probably because nobody has ever tried making remote sim work > with multiple processes at the same time, so nobody ever stumbled > on that. > > > Maybe it's a simple > > oversight, but I think (emphasis on THINK, I could certainly be > > missing something) we need to take a step back and ask why this code > > is there at all. Putting this code in target routines gives us a lot > > of flexibility, but the cost is more mental load (for lack of a better > > phrase) and more touch points when trying to fix/improve core gdb, and > > I'm getting the impression that the pendulum has swung too far towards > > putting general housekeeping operations in target code. > > Huh? I think you're getting this backwards. You make it sound like > we've been adding more of this stuff in target code ("putting"), > while instead these are _ancient_ code that over the years we've > been cleaning up. Well, yes we have been adding non-target-specific code to target files. E.g., I don't see how the trustability of compilers to emit good data for prologues varies by target, and yet we're doing different things in amd64_skip_prologue vs arm_skip_prologue w.r.t. gcc vs clang. I could certainly be wrong here, but I don't think so. Can gcc get this right for ARM enough that we accept the risk of the odd bug whereas with AMD64 we've chosen to not accept this risk? Here's a case where some cleanup is needed, but this is relatively freshly added code. [From one perspective *-tdep files are different than target files like remote.c. But it's still a case of balancing target-specific and non-target-specific code.] Plus, pending an understanding of the !have_inferiors() check above (and elsewhere), it's not clear to me we have always been cleaning up. I can't know until I understand why things are the way they are. And if we haven't always been cleaning up maybe more awareness is needed. OTOH, maybe we have, so I'm asking. So in addition to understanding the !have_inferiors() checks, I want to bring this issue up to help raise awareness. Not with anyone in particular but in general. I only put you and Stan in the To line because I think you guys can shed light on the use of !have_inferiors(). > Why not just git blame on that one? It shows: > > 40b92220 (Jim Kingdon 1993-09-17 17:27:43 +0000 467) > 8501c742 (Stu Grossman 1996-08-13 00:01:37 +0000 468) gdbsim_kill (); > 40b92220 (Jim Kingdon 1993-09-17 17:27:43 +0000 469) remove_breakpoints (); > ec25d19b (Steve Chamberlain 1993-01-03 22:36:04 +0000 470) init_wait_for_inferior (); > ec25d19b (Steve Chamberlain 1993-01-03 22:36:04 +0000 471) Yeah. Alas this data didn't help with my question. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Questions on commit 6c95b8df7fef5273da71c34775918c554aae0ea8 2014-09-30 19:50 ` Doug Evans @ 2014-10-03 17:14 ` Pedro Alves 2014-10-19 22:57 ` Doug Evans 0 siblings, 1 reply; 6+ messages in thread From: Pedro Alves @ 2014-10-03 17:14 UTC (permalink / raw) To: Doug Evans; +Cc: gdb-patches, Stan Shebs On 09/30/2014 08:50 PM, Doug Evans wrote: > Pedro Alves writes: > > On 09/20/2014 07:39 PM, Doug Evans wrote: > > > [Ugh, bad To address on first try.] > > > > > > Hi. > > > > > > While looking into removing lwp_list from gdb (akin to what I did for > > > gdbserver) I found that several bits of target code are calling > > > init_thread_list > > > (grep init_thread_list *.c). > > > Maybe there's the odd case where target code would need to do this, > > > but normally when should target code *ever* do this? > > > > > To try to assist you with getting a handle on my confusion, consider > > > remote.c:extended_remote_create_inferior_1 from gdb 6.8: > > > > > > /* Clean up from the last time we were running. */ > > > init_thread_list (); > > > init_wait_for_inferior (); > > > > > > Now look at what's there today: > > > > > > if (!have_inferiors ()) > > > { > > > /* Clean up from the last time we ran, before we mark the target > > > running again. This will mark breakpoints uninserted, and > > > get_offsets may insert breakpoints. */ > > > init_thread_list (); > > > init_wait_for_inferior (); > > > } > > > > > > > > I think(!) there may be multiple ways to look at all of this as being > > > wrong, so pick your favorite, but here's one way: What does it matter > > > whether there are other inferiors as to whether > > > remote.c:extended_remote_create_inferior has to "clean up from the > > > last time we were running"? > > > > > > Obviously we can't call init_thread_list if there are other inferiors, > > > but why are we calling init_thread_list here at all? Why isn't this > > > state being managed by gdb itself (inf*.c or some such)? I can > > > imagine one can look at this as just being still a work in progress on > > > the way to proper multi-target support. It's stil a bit odd though to > > > have taken this step this way, so I'm hoping for some clarification. > > > > Really not sure what sort of answer you're looking for. > > The most succinct way of expressing what I'm looking for that I can > think of is that I want to understand this code, and I don't. > There are several instances of the !have_inferiors() check, I picked > this one because it seemed like as good a choice as any. init_thread_list does two things: - wipes all threads. - resets the thread id counter back to 0, so the next thread is gdb thread number 1. The main point of calling init_thread_list if there are no live inferiors, is to reset the thread id counter. That is so that with "run; kill; run", you end up with the main thread being gdb thread number 1, in both runs. Now, if there are other live inferiors, then we shouldn't wipe their threads. And as gdb thread ids are global currently, not private per-inferior/process, then we obviously shouldn't reset the thread id counter either. The reason this isn't managed by the core itself, is I believe that nobody has tried to do that yet. > > > Maybe it's a simple > > > oversight, but I think (emphasis on THINK, I could certainly be > > > missing something) we need to take a step back and ask why this code > > > is there at all. Putting this code in target routines gives us a lot > > > of flexibility, but the cost is more mental load (for lack of a better > > > phrase) and more touch points when trying to fix/improve core gdb, and > > > I'm getting the impression that the pendulum has swung too far towards > > > putting general housekeeping operations in target code. > > > > Huh? I think you're getting this backwards. You make it sound like > > we've been adding more of this stuff in target code ("putting"), > > while instead these are _ancient_ code that over the years we've > > been cleaning up. > > Well, yes we have been adding non-target-specific code to target files. > E.g., I don't see how the trustability of compilers to emit good > data for prologues varies by target, and yet we're doing different things > in amd64_skip_prologue vs arm_skip_prologue w.r.t. gcc vs clang. Well, on the compiler side, prologue emission is tied to the backend. Certainly it's easy to picture that different backends on the compiler side vary in quality. For example, I recall that on GCC, there was a long term conversion of prologue emission from text to rtl. A quick google search finds: https://gcc.gnu.org/ml/gcc/2011-06/msg00222.html and https://gcc.gnu.org/ml/gcc-patches/2011-07/msg00685.html for example. I don't know whether something like that affects the cases in question or not. I'm just saying that I could see there being a reason. > I could certainly be wrong here, but I don't think so. > Can gcc get this right for ARM enough that we accept the risk of the > odd bug whereas with AMD64 we've chosen to not accept this risk? > Here's a case where some cleanup is needed, but this is relatively > freshly added code. > [From one perspective *-tdep files are different than > target files like remote.c. But it's still a case of balancing > target-specific and non-target-specific code.] > > Plus, pending an understanding of the !have_inferiors() check above (and > elsewhere), it's not clear to me we have always been cleaning up. I honestly fail to see the relation and I don't think it's useful to try to generalize like that. Each case is a different case. If you find patterns that can be factored out, and find ways to clean things up, then please go for it. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Questions on commit 6c95b8df7fef5273da71c34775918c554aae0ea8 2014-10-03 17:14 ` Pedro Alves @ 2014-10-19 22:57 ` Doug Evans 2014-10-24 16:54 ` Pedro Alves 0 siblings, 1 reply; 6+ messages in thread From: Doug Evans @ 2014-10-19 22:57 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, Stan Shebs Pedro Alves <palves@redhat.com> writes: >> The most succinct way of expressing what I'm looking for that I can >> think of is that I want to understand this code, and I don't. >> There are several instances of the !have_inferiors() check, I picked >> this one because it seemed like as good a choice as any. > > init_thread_list does two things: > > - wipes all threads. > - resets the thread id counter back to 0, so the next > thread is gdb thread number 1. > > The main point of calling init_thread_list if there are > no live inferiors, is to reset the thread id counter. > That is so that with "run; kill; run", you end up with > the main thread being gdb thread number 1, in both > runs. That part I can understand. > Now, if there are other live inferiors, then we shouldn't wipe > their threads. And as gdb thread ids are global currently, > not private per-inferior/process, then we obviously shouldn't > reset the thread id counter either. This I can grok too. However, what if some code is not properly clearing a thread from the list? (which may involve just tagging it as exited and leaving gc'ing it until later) Such a bug will be hidden in the !have_inferiors() case because we take a sledgehammer to the list. To the reader it's not clear whether wiping the list is necessary, or just follows along for the ride, so to speak, and is essentially a no-op, because we're using init_thread_list to reset thread numbering. I would expect the attached patch to be a no-op. I don't have an opinion on committing it. I do have an opinion that the current code is confusing, and this is one way to help clear it up. 2014-10-19 Doug Evans <xdje42@gmail.com> * thread.c (reset_thread_numbering): New function. (init_thread_list): Call it. * gdbthread.h (reset_thread_numbering): Declare. * fork-child.c (fork_inferior): Call reset_thread_numbering instead of init_thread_list. * infcmd.c (kill_command): Ditto. (detach_command): Ditto. * remote.c (extended_remote_create_inferior): Ditto. diff --git a/gdb/fork-child.c b/gdb/fork-child.c index 4c316b1..149eb22 100644 --- a/gdb/fork-child.c +++ b/gdb/fork-child.c @@ -379,7 +379,10 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env, environ = save_our_env; if (!have_inferiors ()) - init_thread_list (); + { + /* Reset thread numbering to begin back at 1. */ + reset_thread_numbering (); + } inf = current_inferior (); diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index fb47bae..4b3bf7f 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -261,6 +261,11 @@ struct thread_info struct btrace_thread_info btrace; }; +/* Reset thread numbering so that they begin at 1 again. + This can only be called when it is known that there are no current + non-exited threads. A typical test is !have_inferiors(). */ +extern void reset_thread_numbering (void); + /* Create an empty thread list, or empty the existing one. */ extern void init_thread_list (void); diff --git a/gdb/infcmd.c b/gdb/infcmd.c index 4415b31..0d06996 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -2399,7 +2399,8 @@ kill_command (char *arg, int from_tty) with their threads. */ if (!have_inferiors ()) { - init_thread_list (); /* Destroy thread info. */ + /* Reset thread numbering to begin back at 1. */ + reset_thread_numbering (); /* Killing off the inferior can leave us with a core file. If so, print the state we are left in. */ @@ -2787,10 +2788,11 @@ detach_command (char *args, int from_tty) if (!gdbarch_has_global_solist (target_gdbarch ())) no_shared_libraries (NULL, from_tty); - /* If we still have inferiors to debug, then don't mess with their - threads. */ if (!have_inferiors ()) - init_thread_list (); + { + /* No more inferiors, reset thread numbering back to 1. */ + reset_thread_numbering (); + } if (deprecated_detach_hook) deprecated_detach_hook (); diff --git a/gdb/remote.c b/gdb/remote.c index 20f2988..6c43da8 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -8035,10 +8035,12 @@ extended_remote_create_inferior (struct target_ops *ops, if (!have_inferiors ()) { + /* Reset thread numbering to begin back at 1. */ + reset_thread_numbering (); + /* Clean up from the last time we ran, before we mark the target running again. This will mark breakpoints uninserted, and get_offsets may insert breakpoints. */ - init_thread_list (); init_wait_for_inferior (); } diff --git a/gdb/thread.c b/gdb/thread.c index 5c94264..789ab7e 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -195,15 +195,30 @@ free_thread (struct thread_info *tp) xfree (tp); } +/* See gdbthread.h. */ + void -init_thread_list (void) +reset_thread_numbering (void) { - struct thread_info *tp, *tpnext; + struct thread_info *tp; + + /* While IWBN to assert thread_list is NULL, there could still be + threads on it (e.g., if you detach from the current inferior). + Instead verify all threads on the list have exited. They will be + garbage-collected as we're able to. */ + for (tp = thread_list; tp != NULL; tp = tp->next) + gdb_assert (tp->state == THREAD_EXITED); highest_thread_num = 0; - if (!thread_list) - return; + /* Take the opportunity to update this since we know it's zero. */ + threads_executing = 0; +} + +void +init_thread_list (void) +{ + struct thread_info *tp, *tpnext; for (tp = thread_list; tp; tp = tpnext) { @@ -212,7 +227,8 @@ init_thread_list (void) } thread_list = NULL; - threads_executing = 0; + + reset_thread_numbering (); } /* Allocate a new thread with target id PTID and add it to the thread ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Questions on commit 6c95b8df7fef5273da71c34775918c554aae0ea8 2014-10-19 22:57 ` Doug Evans @ 2014-10-24 16:54 ` Pedro Alves 0 siblings, 0 replies; 6+ messages in thread From: Pedro Alves @ 2014-10-24 16:54 UTC (permalink / raw) To: Doug Evans; +Cc: gdb-patches, Stan Shebs On 10/19/2014 11:56 PM, Doug Evans wrote: > However, what if some code is not properly clearing a thread > from the list? (which may involve just tagging it as exited and leaving > gc'ing it until later) Such a bug will be hidden in the > !have_inferiors() case because we take a sledgehammer to the list. > To the reader it's not clear whether wiping the list is necessary, > or just follows along for the ride, so to speak, and is essentially > a no-op, because we're using init_thread_list to reset thread numbering. > > I would expect the attached patch to be a no-op. > I don't have an opinion on committing it. > I do have an opinion that the current code is confusing, > and this is one way to help clear it up. > > 2014-10-19 Doug Evans <xdje42@gmail.com> > > * thread.c (reset_thread_numbering): New function. > (init_thread_list): Call it. > * gdbthread.h (reset_thread_numbering): Declare. > * fork-child.c (fork_inferior): Call reset_thread_numbering instead of > init_thread_list. > * infcmd.c (kill_command): Ditto. > (detach_command): Ditto. > * remote.c (extended_remote_create_inferior): Ditto. > > diff --git a/gdb/fork-child.c b/gdb/fork-child.c > index 4c316b1..149eb22 100644 > --- a/gdb/fork-child.c > +++ b/gdb/fork-child.c > @@ -379,7 +379,10 @@ fork_inferior (char *exec_file_arg, char *allargs, char **env, > environ = save_our_env; > > if (!have_inferiors ()) > - init_thread_list (); > + { > + /* Reset thread numbering to begin back at 1. */ > + reset_thread_numbering (); > + } > > inf = current_inferior (); > > diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h > index fb47bae..4b3bf7f 100644 > --- a/gdb/gdbthread.h > +++ b/gdb/gdbthread.h > @@ -261,6 +261,11 @@ struct thread_info > struct btrace_thread_info btrace; > }; > > +/* Reset thread numbering so that they begin at 1 again. > + This can only be called when it is known that there are no current > + non-exited threads. A typical test is !have_inferiors(). */ > +extern void reset_thread_numbering (void); > + > /* Create an empty thread list, or empty the existing one. */ > extern void init_thread_list (void); > > diff --git a/gdb/infcmd.c b/gdb/infcmd.c > index 4415b31..0d06996 100644 > --- a/gdb/infcmd.c > +++ b/gdb/infcmd.c > @@ -2399,7 +2399,8 @@ kill_command (char *arg, int from_tty) > with their threads. */ > if (!have_inferiors ()) > { > - init_thread_list (); /* Destroy thread info. */ > + /* Reset thread numbering to begin back at 1. */ > + reset_thread_numbering (); > > /* Killing off the inferior can leave us with a core file. If > so, print the state we are left in. */ > @@ -2787,10 +2788,11 @@ detach_command (char *args, int from_tty) > if (!gdbarch_has_global_solist (target_gdbarch ())) > no_shared_libraries (NULL, from_tty); > > - /* If we still have inferiors to debug, then don't mess with their > - threads. */ > if (!have_inferiors ()) > - init_thread_list (); > + { > + /* No more inferiors, reset thread numbering back to 1. */ > + reset_thread_numbering (); > + } > > if (deprecated_detach_hook) > deprecated_detach_hook (); > diff --git a/gdb/remote.c b/gdb/remote.c > index 20f2988..6c43da8 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -8035,10 +8035,12 @@ extended_remote_create_inferior (struct target_ops *ops, > > if (!have_inferiors ()) > { > + /* Reset thread numbering to begin back at 1. */ > + reset_thread_numbering (); IMO this function name is clear enough that we wouldn't need to add that same comment to all callers. > + > /* Clean up from the last time we ran, before we mark the target > running again. This will mark breakpoints uninserted, and > get_offsets may insert breakpoints. */ > - init_thread_list (); > init_wait_for_inferior (); > } > > diff --git a/gdb/thread.c b/gdb/thread.c > index 5c94264..789ab7e 100644 > --- a/gdb/thread.c > +++ b/gdb/thread.c > @@ -195,15 +195,30 @@ free_thread (struct thread_info *tp) > xfree (tp); > } > > +/* See gdbthread.h. */ > + > void > -init_thread_list (void) > +reset_thread_numbering (void) > { > - struct thread_info *tp, *tpnext; > + struct thread_info *tp; > + > + /* While IWBN to assert thread_list is NULL, there could still be > + threads on it (e.g., if you detach from the current inferior). Not sure I understand this detach reference. Are you seeing exited threads after a detach from the current inferior? What are the steps you used? detach_inferior and exit_inferior are called with inferior_ptid pointing at null_ptid so that thread can be deleted. > + Instead verify all threads on the list have exited. They will be > + garbage-collected as we're able to. */ > + for (tp = thread_list; tp != NULL; tp = tp->next) > + gdb_assert (tp->state == THREAD_EXITED); Hmm, this doesn't make that much sense to me. Either we have an empty list, or we can't reset the thread numbers. Otherwise, the next thread we add might reuse the GDB ID of one of those exited threads, if it also reuses the target side ID (which can easily happen when the target fakes thread ids, e.g., remote.c:magic_null_ptid). Thinking about this further, I think the cases that may legitimately leave an exited thread in the list are related to thread's reference count having been incremented, either because a make_cleanup_restore_current_thread cleanup is in the cleanup chain, or the user did "thread apply all some-command-that-wipes-threads" -- detach/kill/etc., but also could also be the remote connection closing abruptly while running any command. do_restore_current_thread_cleanup won't restore the dead thread as selected thread, but I think the thread will still be in the list. This then suggest to me that what we need is a function that tries to garbage collect exited threads, and then call that without checking have_inferiors() at specific points We could start with the current places we call init_thread_list, and look for somewhere more central in a following step. prune_threads is close, but I don't think there's need to query anything off of the target. Resetting the thread numbering would still have to only be done under have_inferiors(), or, perhaps even clearer, only if the thread list is empty after the purging. IOW, say that the thread numbering always starts at 1 if we're adding a thread to an empty thread list. With this in mind, it might be simpler to instead reset to the thread numbering back to 1 in new_thread. WDYT? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-10-24 16:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-20 18:39 Questions on commit 6c95b8df7fef5273da71c34775918c554aae0ea8 Doug Evans 2014-09-26 16:51 ` Pedro Alves 2014-09-30 19:50 ` Doug Evans 2014-10-03 17:14 ` Pedro Alves 2014-10-19 22:57 ` Doug Evans 2014-10-24 16:54 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).