)]}' { "commit": "c6b486755e020095710c7494d029577ca967a13a", "tree": "db8ca7276d4ec064c9381c00c77d53773ebfed64", "parents": [ "5a7cfbb424d27c6c27240b14e2572e837299402c" ], "author": { "name": "Andrew Burgess", "email": "aburgess@redhat.com", "time": "Thu Mar 30 19:21:22 2023 +0100" }, "committer": { "name": "Andrew Burgess", "email": "aburgess@redhat.com", "time": "Sat Sep 07 21:48:35 2024 +0100" }, "message": "gdb: parse pending breakpoint thread/task immediately\n\nThe initial motivation for this commit was to allow thread or inferior\nspecific breakpoints to only be inserted within the appropriate\ninferior\u0027s program-space. The benefit of this is that inferiors for\nwhich the breakpoint does not apply will no longer need to stop, and\nthen resume, for such breakpoints. This commit does not make this\nchange, but is a refactor to allow this to happen in a later commit.\n\nThe problem we currently have is that when a thread-specific (or\ninferior-specific) breakpoint is created, the thread (or inferior)\nnumber is only parsed by calling find_condition_and_thread_for_sals.\nThis function is only called for non-pending breakpoints, and requires\nthat we know the locations at which the breakpoint will be placed (for\nexpression checking in case the breakpoint is also conditional).\n\nA consequence of this is that by the time we figure out the breakpoint\nis thread-specific we have already looked up locations in all program\nspaces. This feels wasteful -- if we knew the thread-id earlier then\nwe could reduce the work GDB does by only looking up locations within\nthe program space for which the breakpoint applies.\n\nAnother consequence of how find_condition_and_thread_for_sals is\ncalled is that pending breakpoints don\u0027t currently know they are\nthread-specific, nor even that they are conditional! Additionally, by\ndelaying parsing the thread-id, pending breakpoints can be created for\nnon-existent threads, this is different to how non-pending\nbreakpoints are handled, so I can do this:\n\n $ gdb -q ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp\n Reading symbols from ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp...\n (gdb) break foo thread 99\n Function \"foo\" not defined.\n Make breakpoint pending on future shared library load? (y or [n]) y\n Breakpoint 1 (foo thread 99) pending.\n (gdb) r\n Starting program: /tmp/gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp\n [Thread debugging using libthread_db enabled]\n Using host libthread_db library \"/lib64/libthread_db.so.1\".\n Error in re-setting breakpoint 1: Unknown thread 99.\n [Inferior 1 (process 3329749) exited normally]\n (gdb)\n\nGDB only checked the validity of \u0027thread 99\u0027 at the point the \u0027foo\u0027\nlocation became non-pending. In contrast, if I try this:\n\n $ gdb -q ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp\n Reading symbols from ./gdb/testsuite/outputs/gdb.multi/pending-bp/pending-bp...\n (gdb) break main thread 99\n Unknown thread 99.\n (gdb)\n\nGDB immediately checks if \u0027thread 99\u0027 exists. I think inconsistencies\nlike this are confusing, and should be fixed if possible.\n\nIn this commit the create_breakpoint function is updated so that the\nextra_string, which contains the thread, inferior, task, and/or\ncondition information, is parsed immediately, even for pending\nbreakpoints.\n\nObviously, the condition still can\u0027t be validated until the breakpoint\nbecomes non-pending, but the thread, inferior, and task information\ncan be pulled from the extra-string, and can be validated early on,\neven for pending breakpoints. The -force-condition flag is also\nparsed as part of this early parsing change.\n\nThere are a couple of benefits to doing this:\n\n1. Printing of breakpoints is more consistent now. Consider creating\n a conditional breakpoint before this commit:\n\n (gdb) set breakpoint pending on\n (gdb) break pendingfunc if (0)\n Function \"pendingfunc\" not defined.\n Breakpoint 1 (pendingfunc if (0)) pending.\n (gdb) break main if (0)\n Breakpoint 2 at 0x401198: file /tmp/hello.c, line 18.\n (gdb) info breakpoints\n Num Type Disp Enb Address What\n 1 breakpoint keep y \u003cPENDING\u003e pendingfunc if (0)\n 2 breakpoint keep y 0x0000000000401198 in main at /tmp/hello.c:18\n stop only if (0)\n (gdb)\n\n And after this commit:\n\n (gdb) set breakpoint pending on\n (gdb) break pendingfunc if (0)\n Function \"pendingfunc\" not defined.\n Breakpoint 1 (pendingfunc) pending.\n (gdb) break main if (0)\n Breakpoint 2 at 0x401198: file /home/andrew/tmp/hello.c, line 18.\n (gdb) info breakpoints\n Num Type Disp Enb Address What\n 1 breakpoint keep y \u003cPENDING\u003e pendingfunc\n stop only if (0)\n 2 breakpoint keep y 0x0000000000401198 in main at /home/andrew/tmp/hello.c:18\n stop only if (0)\n (gdb)\n\n Notice that the display of the condition is now the same for the\n pending and non-pending breakpoints.\n\n The same is true for the thread, inferior, or task information in\n thread, inferior, or task specific breakpoints; this information is\n displayed on its own line rather than being part of the \u0027What\u0027\n field.\n\n2. We can check that the thread exists as soon as the pending\n breakpoint is created. Currently there is a weird difference\n between pending and non-pending breakpoints when creating a\n thread-specific breakpoint.\n\n A pending thread-specific breakpoint only checks its thread when it\n becomes non-pending, at which point the thread the breakpoint was\n intended for might have exited. Here\u0027s the behaviour before this\n commit:\n\n (gdb) set breakpoint pending on\n (gdb) break foo thread 2\n Function \"foo\" not defined.\n Breakpoint 2 (foo thread 2) pending.\n (gdb) c\n Continuing.\n [Thread 0x7ffff7c56700 (LWP 2948835) exited]\n Error in re-setting breakpoint 2: Unknown thread 2.\n [Inferior 1 (process 2948832) exited normally]\n (gdb)\n\n Notice the \u0027Error in re-setting breakpoint 2: Unknown thread 2.\u0027\n line, this was triggered when GDB tried to make the breakpoint\n non-pending, and GDB discovers that the thread no longer exists.\n\n Compare that to the behaviour after this commit:\n\n (gdb) set breakpoint pending on\n (gdb) break foo thread 2\n Function \"foo\" not defined.\n Breakpoint 2 (foo) pending.\n (gdb) c\n Continuing.\n [Thread 0x7ffff7c56700 (LWP 2949243) exited]\n Thread-specific breakpoint 2 deleted - thread 2 no longer in the thread list.\n [Inferior 1 (process 2949240) exited normally]\n (gdb)\n\n Now the behaviour for pending breakpoints is identical to\n non-pending breakpoints, the thread specific breakpoint is removed\n as soon as the thread the breakpoint is associated with exits.\n\n There is an additional change; when the pending breakpoint is\n created prior to this patch we see this line:\n\n Breakpoint 2 (foo thread 2) pending.\n\n While after this patch we get this line:\n\n Breakpoint 2 (foo) pending.\n\n Notice that \u0027thread 2\u0027 has disappeared. This might look like a\n regression, but I don\u0027t think it is. That we said \u0027thread 2\u0027\n before was just a consequence of the lazy parsing of the breakpoint\n specification, while with this patch GDB understands, and has\n parsed away the \u0027thread 2\u0027 bit of the spec. If folk think the old\n information was useful then this would be trivial to add back in\n code_breakpoint::say_where.\n\nAs a result of this commit the breakpoints \u0027extra_string\u0027 field is now\nonly used by bp_dprintf type breakpoints to hold the printf format and\narguments. This string should always be empty for other breakpoint\ntypes. This allows some cleanup in print_breakpoint_location.\n\nIn code_breakpoint::code_breakpoint I\u0027ve changed an error case into an\nassert. This is because the error is now handled earlier in\ncreate_breakpoint. As a result we know that by this point, the\nextra_string will always be nullptr for anything other than a\nbp_dprintf style breakpoint.\n\nThe find_condition_and_thread_for_sals function is now no longer\nneeded, this was previously doing the delayed splitting of the extra\nstring into thread, task, and condition, but this is now all done in\ncreate_breakpoint, so find_condition_and_thread_for_sals can be\ndeleted, and the code that calls this in\ncode_breakpoint::location_spec_to_sals can be removed. With this\nupdate this code would only ever be reached for bp_dprintf style\nbreakpoints, and in these cases the extra_string should not contain\nanything other than format and args.\n\nThe most interesting changes are all in create_breakpoint and in the\nnew file break-cond-parse.c. We have a new block of code early on in\ncreate_breakpoint that is responsible for splitting the extra_string\ninto its component parts by calling create_breakpoint_parse_arg_string\na function in the new break-cond-parse.c file. This means that some\nof the later code can be simplified a little.\n\nThe new break-cond-parse.c file implements the splitting up the\nextra_string and finding all the parts, as well as some self-tests for\nthe new function.\n\nFinally, now we know all the breakpoint details, these can be stored\nwithin the breakpoint object if we end up creating a deferred\nbreakpoint. Additionally, if we are creating a deferred bp_dprintf we\ncan parse the extra_string to build the printf command.\n\nThe implementation here aims to maintain backwards compatibility as\nmuch as possible, this means that:\n\n 1. We support abbreviations of \u0027thread\u0027, \u0027task\u0027, and \u0027inferior\u0027 in\n some places on the breakpoint line. The handling of abbreviations\n has (before this patch) been a little weird, so this works:\n\n (gdb) break *main th 1\n\n And creates a breakpoint at \u0027*main\u0027 for thread 1 only, while this\n does not work:\n\n (gdb) break main th 1\n\n In this case GDB will try to find the symbol \u0027main th 1\u0027. This\n weirdness exists before and after this patch.\n\n 2. The handling of \u0027-force-condition\u0027 is odd, if this flag appears\n immediately after a condition then it will be treated as part of the\n condition, e.g.:\n\n (gdb) break main if 0 -force-condition\n No symbol \"force\" in current context.\n\n But we are fine with these alternatives:\n\n (gdb) break main if 0 thread 1 -force-condition\n (gdb) break main -force-condition if 0\n\n Again, this is just a quirk of how the breakpoint line used to be\n parsed, but I\u0027ve maintained this for backward compatibility. During\n review it was suggested that -force-condition should become an\n actual breakpoint flag (i.e. only valid after the \u0027break\u0027 command\n but before the function name), and I don\u0027t think that would be a\n terrible idea, however, that\u0027s not currently a trivial change, and I\n think should be done as a separate piece of work. For now, this\n patch just maintains the current behaviour.\n\nThe implementation works by first splitting the breakpoint condition\nstring (everything after the location specification) into a list of\ntokens, each token has a type and a value. (e.g. we have a THREAD\ntoken where the value is the thread-id string). The list of tokens is\nvalidated, and in some cases, tokens are merged. Then the values are\nextracted from the remaining token list.\n\nConsider this breakpoint command:\n\n (gdb) break main thread 1 if argc \u003d\u003d 2\n\nThe condition string passed to create_breakpoint_parse_arg_string is\ngoing to be \u0027thread 1 if argc \u003d\u003d 2\u0027, which is then split into the\ntokens:\n\n { THREAD: \"1\" } { CONDITION: \"argc \u003d\u003d 2\" }\n\nThe thread-id (1) and the condition string \u0027argc \u003d\u003d 2\u0027 are extracted\nfrom these tokens and returns back to create_breakpoint.\n\nNow consider this breakpoint command:\n\n (gdb) break some_function if ( some_var \u003d\u003d thread )\n\nHere the user wants a breakpoint if \u0027some_var\u0027 is equal to the\nvariable \u0027thread\u0027. However, when this is initially parsed we will\nfind these tokens:\n\n { CONDITION: \"( some_var \u003d\u003d \" } { THREAD: \")\" }\n\nThis is a consequence of how we have to try and figure out the\ncontents of the \u0027if\u0027 condition without actually parsing the\nexpression; parsing the expression requires that we know the location\nin order to lookup the variables by name, and this can\u0027t be done for\npending breakpoints (their location isn\u0027t known yet), and one of the\npoints of this work is that we extract things like thread-id for\npending breakpoints.\n\nAnd so, it is in this case that token merging takes place. We check\nif the value of a token appearing immediately after the CONDITION\ntoken looks valid. In this case, does \u0027)\u0027 look like a valid\nthread-id. Clearly, in this case \u0027)\u0027 does not, and so me merge the\nTHREAD token into the condition token, giving:\n\n { CONDITION: \"( some_var \u003d\u003d thread )\" }\n\nWhich is what we want.\n\nI\u0027m sure that we might still be able to come up with some edge cases\nwhere the parser makes the wrong choice. I think long term the best\nway to work around these would be to move the thread, inferior, task,\nand -force-condition flags to be \"real\" command options for the break\ncommand. I am looking into doing this, but can\u0027t guarantee if/when\nthat work would be completed, so this patch should be reviewed assume\nthat the work will never arrive (though I hope it will).\n\nReviewed-By: Eli Zaretskii \u003celiz@gnu.org\u003e\n", "tree_diff": [ { "type": "modify", "old_id": "13f512f5cad95112a75b96cecd2e82719f69ffca", "old_mode": 33188, "old_path": "gdb/Makefile.in", "new_id": "bcf1ee45a70f9d4b526a319cd7879559771e1c46", "new_mode": 33188, "new_path": "gdb/Makefile.in" }, { "type": "modify", "old_id": "19e05af625a4856363618f30bbeb5c43e63c4f51", "old_mode": 33188, "old_path": "gdb/NEWS", "new_id": "3ee6a61b21006757e0546262db2f604ab1c9d04e", "new_mode": 33188, "new_path": "gdb/NEWS" }, { "type": "add", "old_id": "0000000000000000000000000000000000000000", "old_mode": 0, "old_path": "/dev/null", "new_id": "f5fe308a9236337dccc92f93435e79f9b9987846", "new_mode": 33188, "new_path": "gdb/break-cond-parse.c" }, { "type": "add", "old_id": "0000000000000000000000000000000000000000", "old_mode": 0, "old_path": "/dev/null", "new_id": "cbee70f4e9ed15d7b8340a2a598334febd7a28cc", "new_mode": 33188, "new_path": "gdb/break-cond-parse.h" }, { "type": "modify", "old_id": "eab3b937c13d3729d7ac1329b1818030ebd55b50", "old_mode": 33188, "old_path": "gdb/breakpoint.c", "new_id": "9e30dbc1a9356d028c951af847f28be89258cafd", "new_mode": 33188, "new_path": "gdb/breakpoint.c" }, { "type": "modify", "old_id": "6e588408c9d51706bf7a5f116c3c33e21be08734", "old_mode": 33188, "old_path": "gdb/testsuite/gdb.ada/tasks.exp", "new_id": "ecbfbd5522fb7d4450d7f985e9306f0d4fc37161", "new_mode": 33188, "new_path": "gdb/testsuite/gdb.ada/tasks.exp" }, { "type": "modify", "old_id": "65d19b3e7a9a48cd2362723bab4ed31bc8b3be51", "old_mode": 33188, "old_path": "gdb/testsuite/gdb.base/condbreak.exp", "new_id": "3b619e47bfcdaba5a1dbd1df8e9f1791f76c0e82", "new_mode": 33188, "new_path": "gdb/testsuite/gdb.base/condbreak.exp" }, { "type": "modify", "old_id": "5ee31d32389dcb6e6d2a6191d997cae2c72f1b8a", "old_mode": 33188, "old_path": "gdb/testsuite/gdb.base/pending.exp", "new_id": "833e084db82df838c99c8b1b277ecf825c2c0d7c", "new_mode": 33188, "new_path": "gdb/testsuite/gdb.base/pending.exp" }, { "type": "modify", "old_id": "60183e98e1e1e0bb0b1708c327fb97121d10836c", "old_mode": 33188, "old_path": "gdb/testsuite/gdb.linespec/explicit.exp", "new_id": "e8ae10a27dfa72ef289fa945b8543945888c8094", "new_mode": 33188, "new_path": "gdb/testsuite/gdb.linespec/explicit.exp" }, { "type": "modify", "old_id": "36a919c8be2f774c259cc003da0336f64768bb1c", "old_mode": 33188, "old_path": "gdb/testsuite/gdb.linespec/keywords.exp", "new_id": "d2596d2b3576b774148df72b480cb52f959a57d6", "new_mode": 33188, "new_path": "gdb/testsuite/gdb.linespec/keywords.exp" }, { "type": "modify", "old_id": "fd5684bd2b149bf9e8a731a19360d7ad49e09442", "old_mode": 33188, "old_path": "gdb/testsuite/gdb.mi/mi-dprintf-pending.exp", "new_id": "4cf6dec4b41439030826ae256dbc1e6b23bfa2b3", "new_mode": 33188, "new_path": "gdb/testsuite/gdb.mi/mi-dprintf-pending.exp" }, { "type": "modify", "old_id": "5cc451b0ecc1aa4bc709945cf34fa1a404647919", "old_mode": 33188, "old_path": "gdb/testsuite/gdb.multi/inferior-specific-bp.exp", "new_id": "46efe6f54bcf3ca56e2b1e1a9ad74f6867e979f9", "new_mode": 33188, "new_path": "gdb/testsuite/gdb.multi/inferior-specific-bp.exp" }, { "type": "add", "old_id": "0000000000000000000000000000000000000000", "old_mode": 0, "old_path": "/dev/null", "new_id": "15d1b9833dd4e0c3b72c0f2d920c4f11522a99b3", "new_mode": 33188, "new_path": "gdb/testsuite/gdb.threads/del-pending-thread-bp-lib.c" }, { "type": "add", "old_id": "0000000000000000000000000000000000000000", "old_mode": 0, "old_path": "/dev/null", "new_id": "6fc76dbf08c948fc45fdce5aff9b452e29de18c8", "new_mode": 33188, "new_path": "gdb/testsuite/gdb.threads/del-pending-thread-bp.c" }, { "type": "add", "old_id": "0000000000000000000000000000000000000000", "old_mode": 0, "old_path": "/dev/null", "new_id": "14a91a40d76e1fe0f772f9af04ec96ff38f80c59", "new_mode": 33188, "new_path": "gdb/testsuite/gdb.threads/del-pending-thread-bp.exp" } ] }