)]}' { "commit": "2968b79fca38cf18e8eef360c36de7a6e3846d3c", "tree": "8554b75a28346a635b347669917446b9f602f519", "parents": [ "05ac6365e558a207fcf7fa39c3fc8c7b1d0601aa" ], "author": { "name": "Andrew Burgess", "email": "aburgess@redhat.com", "time": "Thu Feb 16 13:06:29 2023 +0000" }, "committer": { "name": "Andrew Burgess", "email": "aburgess@redhat.com", "time": "Tue Feb 28 10:56:28 2023 +0000" }, "message": "gdb: fix mi breakpoint-deleted notifications for thread-specific b/p\n\nBackground\n----------\n\nWhen a thread-specific breakpoint is deleted as a result of the\nspecific thread exiting the function remove_threaded_breakpoints is\ncalled which sets the disposition of the breakpoint to\ndisp_del_at_next_stop and sets the breakpoint number to 0. Setting\nthe breakpoint number to zero has the effect of hiding the breakpoint\nfrom the user. We also print a message indicating that the breakpoint\nhas been deleted.\n\nIt was brought to my attention during a review of another patch[1]\nthat setting a breakpoints number to zero will suppress the MI\nbreakpoint-deleted notification for that breakpoint, and indeed, this\ncan be seen to be true, in delete_breakpoint, if the breakpoint number\nis zero, then GDB will not notify the breakpoint_deleted observer.\n\nIt seems wrong that a user created, thread-specific breakpoint, will\nhave a \u003dbreakpoint-created notification, but will not have a\n\u003dbreakpoint-deleted notification. I suspect that this is a bug.\n\n[1] https://sourceware.org/pipermail/gdb-patches/2023-February/196560.html\n\nThe First Problem\n-----------------\n\nDuring my initial testing I wanted to see how GDB handled the\nbreakpoint after it\u0027s number was set to zero. To do this I created\nthe testcase gdb.threads/thread-bp-deleted.exp. This test creates a\nworker thread, which immediately exits. After the worker thread has\nexited the main thread spins in a loop.\n\nIn GDB I break once the worker thread has been created and place a\nthread-specific breakpoint, then use \u0027continue\u0026\u0027 to resume the\ninferior in non-stop mode. The worker thread then exits, but the main\nthread never stops - instead it sits in the spin. I then tried to use\n\u0027maint info breakpoints\u0027 to see what GDB thought of the\nthread-specific breakpoint.\n\nUnfortunately, GDB crashed like this:\n\n (gdb) continue\u0026\n Continuing.\n (gdb) [Thread 0x7ffff7c5d700 (LWP 1202458) exited]\n Thread-specific breakpoint 3 deleted - thread 2 no longer in the thread list.\n maint info breakpoints\n ... snip some output ...\n\n Fatal signal: Segmentation fault\n ----- Backtrace -----\n 0x5ffb62 gdb_internal_backtrace_1\n ../../src/gdb/bt-utils.c:122\n 0x5ffc05 _Z22gdb_internal_backtracev\n ../../src/gdb/bt-utils.c:168\n 0x89965e handle_fatal_signal\n ../../src/gdb/event-top.c:964\n 0x8997ca handle_sigsegv\n ../../src/gdb/event-top.c:1037\n 0x7f96f5971b1f ???\n /usr/src/debug/glibc-2.30-2-gd74461fa34/nptl/../sysdeps/unix/sysv/linux/x86_64/sigaction.c:0\n 0xe602b0 _Z15print_thread_idP11thread_info\n ../../src/gdb/thread.c:1439\n 0x5b3d05 print_one_breakpoint_location\n ../../src/gdb/breakpoint.c:6542\n 0x5b462e print_one_breakpoint\n ../../src/gdb/breakpoint.c:6702\n 0x5b5354 breakpoint_1\n ../../src/gdb/breakpoint.c:6924\n 0x5b58b8 maintenance_info_breakpoints\n ../../src/gdb/breakpoint.c:7009\n ... etc ...\n\nAs the thread-specific breakpoint is set to disp_del_at_next_stop, and\nGDB hasn\u0027t stopped yet, then the breakpoint still exists in the global\nbreakpoint list.\n\nThe breakpoint will not show in \u0027info breakpoints\u0027 as its number is\nzero, but it will show in \u0027maint info breakpoints\u0027.\n\nAs GDB prints the breakpoint, the thread-id for the breakpoint is\nprinted as part of the \u0027stop only in thread ...\u0027 line. Printing the\nthread-id involves calling find_thread_global_id to convert the global\nthread-id into a thread_info*. Then calling print_thread_id to\nconvert the thread_info* into a string.\n\nThe problem is that find_thread_global_id returns nullptr as the\nthread for the thread-specific breakpoint has exited. The\nprint_thread_id assumes it will be passed a non-nullptr. As a result\nGDB crashes.\n\nIn this commit I\u0027ve added an assert to print_thread_id (gdb/thread.c)\nto check that the pointed passed in is not nullptr. This assert would\nhave triggered in the above case before GDB crashed.\n\nMI Notifications: The Dangers Of Changing A Breakpoint\u0027s Number\n---------------------------------------------------------------\n\nCurrently the delete_breakpoint function doesn\u0027t trigger the\nbreakpoint_deleted observer for any breakpoint with the number zero.\n\nThere is a comment explaining why this is the case in the code; it\u0027s\nsomething about watchpoints. But I did consider just removing the \u0027is\nthe number zero\u0027 guard and always triggering the breakpoint_deleted\nobserver, figuring that I\u0027d then fix the watchpoint issue some other\nway.\n\nBut I realised this wasn\u0027t going to be good enough. When the MI\nnotification was delivered the number would be zero, so any frontend\nparsing the notifications would not be able to match\n\u003dbreakpoint-deleted notification to the earlier \u003dbreakpoint-created\nnotification.\n\nWhat this means is that, at the point the breakpoint_deleted observer\nis called, the breakpoint\u0027s number must be correct.\n\nMI Notifications: The Dangers Of Delaying Deletion\n--------------------------------------------------\n\nThe test I used to expose the above crash also brought another problem\nto my attention. In the above test we used \u0027continue\u0026\u0027 to resume,\nafter which a thread exited, but the inferior didn\u0027t stop. Recreating\nthe same test in the MI looks like this:\n\n -break-insert -p 2 main\n ^done,bkpt\u003d{number\u003d\"2\",type\u003d\"breakpoint\",disp\u003d\"keep\",...\u003csnip\u003e...}\n (gdb)\n -exec-continue\n ^running\n *running,thread-id\u003d\"all\"\n (gdb)\n ~\"[Thread 0x7ffff7c5d700 (LWP 987038) exited]\\n\"\n \u003dthread-exited,id\u003d\"2\",group-id\u003d\"i1\"\n ~\"Thread-specific breakpoint 2 deleted - thread 2 no longer in the thread list.\\n\"\n\nAt this point the we have a single thread left, which is still\nrunning:\n\n -thread-info\n ^done,threads\u003d[{id\u003d\"1\",target-id\u003d\"Thread 0x7ffff7c5eb80 (LWP 987035)\",name\u003d\"thread-bp-delet\",state\u003d\"running\",core\u003d\"4\"}],current-thread-id\u003d\"1\"\n (gdb)\n\nNotice that we got the \u003dthread-exited notification from GDB as soon as\nthe thread exited. We also saw the CLI line from GDB, the line\nexplaining that breakpoint 2 was deleted. But, as expected, we didn\u0027t\nsee the \u003dbreakpoint-deleted notification.\n\nI say \"as expected\" because the number was set to zero. But, even if\nthe number was not set to zero we still wouldn\u0027t see the\nnotification. The MI notification is driven by the breakpoint_deleted\nobserver, which is only called when we actually delete the breakpoint,\nwhich is only done the next time GDB stops.\n\nNow, maybe this is fine. The notification is delivered a little\nlate. But remember, by setting the number to zero the breakpoint will\nbe hidden from the user, for example, the breakpoint is removed from\nthe MI\u0027s -break-info command output.\n\nThis means that GDB is in a position where the breakpoint doesn\u0027t show\nup in the breakpoint table, but a \u003dbreakpoint-deleted notification has\nnot yet been sent out. This doesn\u0027t seem right to me.\n\nWhat this means is that, when the thread exits, we should immediately\nbe sending out the \u003dbreakpoint-deleted notification. We should not\nwait for GDB to next stop before sending the notification.\n\nThe Solution\n------------\n\nMy proposed solution is this; in remove_threaded_breakpoints, instead\nof setting the disposition to disp_del_at_next_stop and setting the\nnumber to zero, we now just call delete_breakpoint directly.\n\nThe notification will now be sent out immediately; as soon as the\nthread exits.\n\nAs the number has not changed when delete_breakpoint is called, the\nnotification will have the correct number.\n\nAnd as the breakpoint is immediately removed from the breakpoint list,\nwe no longer need to worry about \u0027maint info breakpoints\u0027 trying to\nprint the thread-id for an exited thread.\n\nMy only concern is that calling delete_breakpoint directly seems so\nobvious that I wonder why the original patch (that added\nremove_threaded_breakpoints) didn\u0027t take this approach. This code was\nadded in commit 49fa26b0411d, but the commit message offers no clues\nto why this approach was taken, and the original email thread offers\nno insights either[2]. There are no test regressions after making\nthis change, so I\u0027m hopeful that this is going to be fine.\n\n[2] https://sourceware.org/pipermail/gdb-patches/2013-September/106493.html\n\nThe Complication\n----------------\n\nOf course, it couldn\u0027t be that simple.\n\nThe script gdb.python/py-finish-breakpoint.exp had some regressions\nduring testing.\n\nThe problem was with the FinishBreakpoint.out_of_scope callback\nimplementation. This callback is supposed to trigger whenever the\nFinishBreakpoint goes out of scope; and this includes when the thread\nfor the breakpoint exits.\n\nThe problem I ran into is the Python FinishBreakpoint implementation.\nSpecifically, after this change I was loosing some of the out_of_scope\ncalls.\n\nThe problem is that the out_of_scope call (of which I\u0027m interested) is\ntriggered from the inferior_exit observer. Before my change the\nobservers were called in this order:\n\n thread_exit\n inferior_exit\n breakpoint_deleted\n\nThe inferior_exit would trigger the out_of_scope call.\n\nAfter my change the breakpoint_deleted notification (for\nthread-specific breakpoints) occurs earlier, as soon as the\nthread-exits, so now the order is:\n\n thread_exit\n breakpoint_deleted\n inferior_exit\n\nCurrently, after the breakpoint_deleted call the Python object\nassociated with the breakpoint is released, so, when we get to the\ninferior_exit observer, there\u0027s no longer a Python object to call the\nout_of_scope method on.\n\nMy solution is to follow the model for how bpfinishpy_pre_stop_hook\nand bpfinishpy_post_stop_hook are called, this is done from\ngdbpy_breakpoint_cond_says_stop in py-breakpoint.c.\n\nI\u0027ve now added a new bpfinishpy_pre_delete_hook\ngdbpy_breakpoint_deleted in py-breakpoint.c, and from this new hook\nfunction I check and where needed call the out_of_scope method.\n\nWith this fix in place I now see the\ngdb.python/py-finish-breakpoint.exp test fully passing again.\n\nTesting\n-------\n\nTested on x86-64/Linux with unix, native-gdbserver, and\nnative-extended-gdbserver boards.\n\nNew tests added to covers all the cases I\u0027ve discussed above.\n\nApproved-By: Pedro Alves \u003cpedro@palves.net\u003e\n", "tree_diff": [ { "type": "modify", "old_id": "36c53e5dae611b971825a8c7aa35ed947472f089", "old_mode": 33188, "old_path": "gdb/breakpoint.c", "new_id": "a42d26fd25ab71f551070f2f0716d9d3fee95bb3", "new_mode": 33188, "new_path": "gdb/breakpoint.c" }, { "type": "modify", "old_id": "ecf52a4637cd935ab521ec322aea8a42d5722a11", "old_mode": 33188, "old_path": "gdb/python/py-breakpoint.c", "new_id": "880f1b5c1e216e7560365686b0d2ec14abec52d1", "new_mode": 33188, "new_path": "gdb/python/py-breakpoint.c" }, { "type": "modify", "old_id": "159164e80093ccde5b346727d1900c6874c6fafa", "old_mode": 33188, "old_path": "gdb/python/py-finishbreakpoint.c", "new_id": "7122fa820f60c563ce813e90c204bbc63f96c403", "new_mode": 33188, "new_path": "gdb/python/py-finishbreakpoint.c" }, { "type": "modify", "old_id": "55bbe78a7a57d74a094b360c2e40a41c5495f137", "old_mode": 33188, "old_path": "gdb/python/python-internal.h", "new_id": "258f5c42537926efecff2dbc85b4af27147b0dcb", "new_mode": 33188, "new_path": "gdb/python/python-internal.h" }, { "type": "add", "old_id": "0000000000000000000000000000000000000000", "old_mode": 0, "old_path": "/dev/null", "new_id": "5ac23b4ab255acba2196e79eed9e7ebb27e3d691", "new_mode": 33188, "new_path": "gdb/testsuite/gdb.mi/mi-thread-bp-deleted.c" }, { "type": "add", "old_id": "0000000000000000000000000000000000000000", "old_mode": 0, "old_path": "/dev/null", "new_id": "0ebca9248015be19ee42ea0cd9a7da5ba27e5641", "new_mode": 33188, "new_path": "gdb/testsuite/gdb.mi/mi-thread-bp-deleted.exp" }, { "type": "add", "old_id": "0000000000000000000000000000000000000000", "old_mode": 0, "old_path": "/dev/null", "new_id": "5ac23b4ab255acba2196e79eed9e7ebb27e3d691", "new_mode": 33188, "new_path": "gdb/testsuite/gdb.threads/thread-bp-deleted.c" }, { "type": "add", "old_id": "0000000000000000000000000000000000000000", "old_mode": 0, "old_path": "/dev/null", "new_id": "019bdddee818b1562ca82d46ff2b1469b56cb4eb", "new_mode": 33188, "new_path": "gdb/testsuite/gdb.threads/thread-bp-deleted.exp" }, { "type": "modify", "old_id": "1a852f702069f4324b976d1813f93d7c41a9c5fc", "old_mode": 33188, "old_path": "gdb/thread.c", "new_id": "5b472150a627ab5a1dd72ec0fa8b8558a452eb03", "new_mode": 33188, "new_path": "gdb/thread.c" } ] }