Andrew Burgess c6b486755e gdb: parse pending breakpoint thread/task immediately ... The initial motivation for this commit was to allow thread or inferior specific breakpoints to only be inserted within the appropriate inferior's program-space. The benefit of this is that inferiors for which the breakpoint does not apply will no longer need to stop, and then resume, for such breakpoints. This commit does not make this change, but is a refactor to allow this to happen in a later commit. The problem we currently have is that when a thread-specific (or inferior-specific) breakpoint is created, the thread (or inferior) number is only parsed by calling find_condition_and_thread_for_sals. This function is only called for non-pending breakpoints, and requires that we know the locations at which the breakpoint will be placed (for expression checking in case the breakpoint is also conditional). A consequence of this is that by the time we figure out the breakpoint is thread-specific we have already looked up locations in all program spaces. This feels wasteful -- if we knew the thread-id earlier then we could reduce the work GDB does by only looking up locations within the program space for which the breakpoint applies. Another consequence of how find_condition_and_thread_for_sals is called is that pending breakpoints don't currently know they are thread-specific, nor even that they are conditional! Additionally, by delaying parsing the thread-id, pending breakpoints can be created for non-existent threads, this is different to how non-pending breakpoints are handled, so I can do this: $ gdb -q ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp Reading symbols from ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp... (gdb) break foo thread 99 Function "foo" not defined. Make breakpoint pending on future shared library load? (y or [n]) y Breakpoint 1 (foo thread 99) pending. (gdb) r Starting program: /tmp/gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". Error in re-setting breakpoint 1: Unknown thread 99. [Inferior 1 (process 3329749) exited normally] (gdb) GDB only checked the validity of 'thread 99' at the point the 'foo' location became non-pending. In contrast, if I try this: $ gdb -q ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp Reading symbols from ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp... (gdb) break main thread 99 Unknown thread 99. (gdb) GDB immediately checks if 'thread 99' exists. I think inconsistencies like this are confusing, and should be fixed if possible. In this commit the create_breakpoint function is updated so that the extra_string, which contains the thread, inferior, task, and/or condition information, is parsed immediately, even for pending breakpoints. Obviously, the condition still can't be validated until the breakpoint becomes non-pending, but the thread, inferior, and task information can be pulled from the extra-string, and can be validated early on, even for pending breakpoints. The -force-condition flag is also parsed as part of this early parsing change. There are a couple of benefits to doing this: 1. Printing of breakpoints is more consistent now. Consider creating a conditional breakpoint before this commit: (gdb) set breakpoint pending on (gdb) break pendingfunc if (0) Function "pendingfunc" not defined. Breakpoint 1 (pendingfunc if (0)) pending. (gdb) break main if (0) Breakpoint 2 at 0x401198: file /tmp/hello.c, line 18. (gdb) info breakpoints Num Type Disp Enb Address What 1 breakpoint keep y <PENDING> pendingfunc if (0) 2 breakpoint keep y 0x0000000000401198 in main at /tmp/hello.c:18 stop only if (0) (gdb) And after this commit: (gdb) set breakpoint pending on (gdb) break pendingfunc if (0) Function "pendingfunc" not defined. Breakpoint 1 (pendingfunc) pending. (gdb) break main if (0) Breakpoint 2 at 0x401198: file /home/andrew/tmp/hello.c, line 18. (gdb) info breakpoints Num Type Disp Enb Address What 1 breakpoint keep y <PENDING> pendingfunc stop only if (0) 2 breakpoint keep y 0x0000000000401198 in main at /home/andrew/tmp/hello.c:18 stop only if (0) (gdb) Notice that the display of the condition is now the same for the pending and non-pending breakpoints. The same is true for the thread, inferior, or task information in thread, inferior, or task specific breakpoints; this information is displayed on its own line rather than being part of the 'What' field. 2. We can check that the thread exists as soon as the pending breakpoint is created. Currently there is a weird difference between pending and non-pending breakpoints when creating a thread-specific breakpoint. A pending thread-specific breakpoint only checks its thread when it becomes non-pending, at which point the thread the breakpoint was intended for might have exited. Here's the behaviour before this commit: (gdb) set breakpoint pending on (gdb) break foo thread 2 Function "foo" not defined. Breakpoint 2 (foo thread 2) pending. (gdb) c Continuing. [Thread 0x7ffff7c56700 (LWP 2948835) exited] Error in re-setting breakpoint 2: Unknown thread 2. [Inferior 1 (process 2948832) exited normally] (gdb) Notice the 'Error in re-setting breakpoint 2: Unknown thread 2.' line, this was triggered when GDB tried to make the breakpoint non-pending, and GDB discovers that the thread no longer exists. Compare that to the behaviour after this commit: (gdb) set breakpoint pending on (gdb) break foo thread 2 Function "foo" not defined. Breakpoint 2 (foo) pending. (gdb) c Continuing. [Thread 0x7ffff7c56700 (LWP 2949243) exited] Thread-specific breakpoint 2 deleted - thread 2 no longer in the thread list. [Inferior 1 (process 2949240) exited normally] (gdb) Now the behaviour for pending breakpoints is identical to non-pending breakpoints, the thread specific breakpoint is removed as soon as the thread the breakpoint is associated with exits. There is an additional change; when the pending breakpoint is created prior to this patch we see this line: Breakpoint 2 (foo thread 2) pending. While after this patch we get this line: Breakpoint 2 (foo) pending. Notice that 'thread 2' has disappeared. This might look like a regression, but I don't think it is. That we said 'thread 2' before was just a consequence of the lazy parsing of the breakpoint specification, while with this patch GDB understands, and has parsed away the 'thread 2' bit of the spec. If folk think the old information was useful then this would be trivial to add back in code_breakpoint::say_where. As a result of this commit the breakpoints 'extra_string' field is now only used by bp_dprintf type breakpoints to hold the printf format and arguments. This string should always be empty for other breakpoint types. This allows some cleanup in print_breakpoint_location. In code_breakpoint::code_breakpoint I've changed an error case into an assert. This is because the error is now handled earlier in create_breakpoint. As a result we know that by this point, the extra_string will always be nullptr for anything other than a bp_dprintf style breakpoint. The find_condition_and_thread_for_sals function is now no longer needed, this was previously doing the delayed splitting of the extra string into thread, task, and condition, but this is now all done in create_breakpoint, so find_condition_and_thread_for_sals can be deleted, and the code that calls this in code_breakpoint::location_spec_to_sals can be removed. With this update this code would only ever be reached for bp_dprintf style breakpoints, and in these cases the extra_string should not contain anything other than format and args. The most interesting changes are all in create_breakpoint and in the new file break-cond-parse.c. We have a new block of code early on in create_breakpoint that is responsible for splitting the extra_string into its component parts by calling create_breakpoint_parse_arg_string a function in the new break-cond-parse.c file. This means that some of the later code can be simplified a little. The new break-cond-parse.c file implements the splitting up the extra_string and finding all the parts, as well as some self-tests for the new function. Finally, now we know all the breakpoint details, these can be stored within the breakpoint object if we end up creating a deferred breakpoint. Additionally, if we are creating a deferred bp_dprintf we can parse the extra_string to build the printf command. The implementation here aims to maintain backwards compatibility as much as possible, this means that: 1. We support abbreviations of 'thread', 'task', and 'inferior' in some places on the breakpoint line. The handling of abbreviations has (before this patch) been a little weird, so this works: (gdb) break *main th 1 And creates a breakpoint at '*main' for thread 1 only, while this does not work: (gdb) break main th 1 In this case GDB will try to find the symbol 'main th 1'. This weirdness exists before and after this patch. 2. The handling of '-force-condition' is odd, if this flag appears immediately after a condition then it will be treated as part of the condition, e.g.: (gdb) break main if 0 -force-condition No symbol "force" in current context. But we are fine with these alternatives: (gdb) break main if 0 thread 1 -force-condition (gdb) break main -force-condition if 0 Again, this is just a quirk of how the breakpoint line used to be parsed, but I've maintained this for backward compatibility. During review it was suggested that -force-condition should become an actual breakpoint flag (i.e. only valid after the 'break' command but before the function name), and I don't think that would be a terrible idea, however, that's not currently a trivial change, and I think should be done as a separate piece of work. For now, this patch just maintains the current behaviour. The implementation works by first splitting the breakpoint condition string (everything after the location specification) into a list of tokens, each token has a type and a value. (e.g. we have a THREAD token where the value is the thread-id string). The list of tokens is validated, and in some cases, tokens are merged. Then the values are extracted from the remaining token list. Consider this breakpoint command: (gdb) break main thread 1 if argc == 2 The condition string passed to create_breakpoint_parse_arg_string is going to be 'thread 1 if argc == 2', which is then split into the tokens: { THREAD: "1" } { CONDITION: "argc == 2" } The thread-id (1) and the condition string 'argc == 2' are extracted from these tokens and returns back to create_breakpoint. Now consider this breakpoint command: (gdb) break some_function if ( some_var == thread ) Here the user wants a breakpoint if 'some_var' is equal to the variable 'thread'. However, when this is initially parsed we will find these tokens: { CONDITION: "( some_var == " } { THREAD: ")" } This is a consequence of how we have to try and figure out the contents of the 'if' condition without actually parsing the expression; parsing the expression requires that we know the location in order to lookup the variables by name, and this can't be done for pending breakpoints (their location isn't known yet), and one of the points of this work is that we extract things like thread-id for pending breakpoints. And so, it is in this case that token merging takes place. We check if the value of a token appearing immediately after the CONDITION token looks valid. In this case, does ')' look like a valid thread-id. Clearly, in this case ')' does not, and so me merge the THREAD token into the condition token, giving: { CONDITION: "( some_var == thread )" } Which is what we want. I'm sure that we might still be able to come up with some edge cases where the parser makes the wrong choice. I think long term the best way to work around these would be to move the thread, inferior, task, and -force-condition flags to be "real" command options for the break command. I am looking into doing this, but can't guarantee if/when that work would be completed, so this patch should be reviewed assume that the work will never arrive (though I hope it will). Reviewed-By: Eli Zaretskii <eliz@gnu.org>		2024-09-07 21:48:35 +01:00
..
base
3explicit.c
body.h
break-ask.exp
break-asm-file0.S
break-asm-file1.S
break-asm-file.c
break-asm-file.exp
cp-completion-aliases.cc
cp-completion-aliases.exp
cp-replace-typedefs-ns-template.cc
cp-replace-typedefs-ns-template.exp
cpcompletion.exp
cpexplicit.cc
cpexplicit.exp
cpls2.cc
cpls-abi-tag.cc
cpls-abi-tag.exp
cpls-hyphen.cc
cpls-ops.cc
cpls-ops.exp
cpls.cc
errors.exp
explicit2.c
explicit.c
explicit.exp
keywords.c
keywords.exp
line-breakpoint-outside-function.c
line-breakpoint-outside-function.exp
linespec.exp
ls-dollar.cc
ls-dollar.exp
ls-errs.c
ls-errs.exp
lspec.cc
lspec.h
macro-relative.c
macro-relative.exp
skip-two.exp
thread.c
thread.exp