)]}'
{
  "commit": "85eb08c5f05b2e7d89009ee0388e88feb0d85fe2",
  "tree": "b86dd5b0a94083238536e2f9ab5ffcdf2e0b3835",
  "parents": [
    "c6b486755e020095710c7494d029577ca967a13a"
  ],
  "author": {
    "name": "Andrew Burgess",
    "email": "aburgess@redhat.com",
    "time": "Tue Aug 22 12:25:54 2023 +0100"
  },
  "committer": {
    "name": "Andrew Burgess",
    "email": "aburgess@redhat.com",
    "time": "Sat Sep 07 21:48:35 2024 +0100"
  },
  "message": "gdb: don\u0027t set breakpoint::pspace in create_breakpoint\n\nI spotted this code within create_breakpoint:\n\n  if ((type_wanted !\u003d bp_breakpoint\n      \u0026\u0026 type_wanted !\u003d bp_hardware_breakpoint) || thread !\u003d -1)\n   b-\u003epspace \u003d current_program_space;\n\nthis code is only executed when creating a pending breakpoint, and\nsets the breakpoint::pspace member variable.\n\nThe above code gained the \u0027thread !\u003d -1\u0027 clause with this commit:\n\n  commit cc72b2a2da6d6372cbdb1d14639a5fce84e1a325\n  Date:   Fri Dec 23 17:06:16 2011 +0000\n\n              Introduce gdb.FinishBreakpoint in Python\n\nWhile the type_wanted checks were added with this commit:\n\n  commit f8eba3c61629b3c03ac1f33853eab4d8507adb9c\n  Date:   Tue Dec 6 18:54:43 2011 +0000\n\n      the \"ambiguous linespec\" series\n\nBefore this breakpoint::pspace was set unconditionally.\n\nIf we look at how breakpoint::pspace is used today, some breakpoint\ntypes specifically set this field, either in their constructors, or in\na wrapper function that calls the constructor.  So, the watchpoint\ntype and its sub-class set this variable, as does the catchpoint type,\nand all it\u0027s sub-classes.\n\nHowever, code_breakpoint doesn\u0027t specifically set this field within\nits constructor, though some sub-classes of\ncode_breakpoint (ada_catchpoint, exception_catchpoint,\ninternal_breakpoint, and momentary_breakpoint) do set this field.\n\nWhen I examine all the places that breakpoint::pspace is used, I\nbelieve that in every place where it is expected that this field is\nset, the breakpoint type will be one that specifically sets this\nfield.\n\nNext, I observe two problems with the existing code.\n\nFirst, the above code is only hit for pending breakpoints, there\u0027s no\nequivalent code for non-pending breakpoints.  This opens up the\npossibility of GDB entering non-consistent states; if a breakpoint is\nfirst created pending and then later gets a location, the pspace field\nwill be set, while if the breakpoint is immediately non-pending, then\nthe pspace field will never be set.\n\nSecond, if we look at how breakpoint::pspace is used in the function\nbreakpoint_program_space_exit, we see that when a program space is\nremoved, any breakpoint with breakpoint::pspace set to the removed\nprogram space, will be deleted.  This makes sense, but does mean we\nneed to ensure breakpoint::pspace is only set for breakpoints that\napply to a single program space.\n\nSo, if I create a pending dprintf breakpoint (type bp_dprintf) then\nthe breakpoint::pspace variable will be set even though the dprintf is\nnot really tied to that one program space.  As a result, when the\nmatching program space is removed the dprintf is incorrectly removed.\n\nAlso, if I create a thread specific breakpoint, then, thanks to the\n\u0027thread !\u003d -1\u0027 clause the wrong program space will be stored in\nbreakpoint::pspace (the current program space is always used, which\nmight not be the program space that corresponds to the selected\nthread), as a result, the thread specific breakpoint will be deleted\nwhen the matching program space is removed.\n\nIf we look at commit cc72b2a2da6d which added the \u0027thread !\u003d -1\u0027\nclause, we can see this change was entirely redundant, the\nbreakpoint::pspace is also set in bpfinishpy_init after\ncreate_breakpoint has been called.  As such, I think we can safely\ndrop the \u0027thread !\u003d -1\u0027 clause.\n\nFor the other problems, I\u0027m proposing to be pretty aggressive - I\u0027d\nlike to drop the breakpoint::pspace assignment completely from\ncreate_breakpoint.  Having looked at how this variable is used, I\nbelieve that it is already set elsewhere in all the cases that it is\nneeded.  Maybe this code was needed at one time, but I can\u0027t see how\nit\u0027s needed any more.\n\nThere\u0027s tests to expose the issues I\u0027ve spotted with this code, and\nthere\u0027s no regressions in testing.\n",
  "tree_diff": [
    {
      "type": "modify",
      "old_id": "9e30dbc1a9356d028c951af847f28be89258cafd",
      "old_mode": 33188,
      "old_path": "gdb/breakpoint.c",
      "new_id": "8860ec48fc0726c610a61bb306923a6d778c1ff2",
      "new_mode": 33188,
      "new_path": "gdb/breakpoint.c"
    },
    {
      "type": "modify",
      "old_id": "064165374fdc214d56725fe3d92567cc6bb5a599",
      "old_mode": 33188,
      "old_path": "gdb/breakpoint.h",
      "new_id": "77b754fc7dad683faa0dfc62cbcb52bf1d7666ea",
      "new_mode": 33188,
      "new_path": "gdb/breakpoint.h"
    },
    {
      "type": "add",
      "old_id": "0000000000000000000000000000000000000000",
      "old_mode": 0,
      "old_path": "/dev/null",
      "new_id": "1d0300078190d825f70c24c50dea0b4206ece8ec",
      "new_mode": 33188,
      "new_path": "gdb/testsuite/gdb.multi/pending-bp-del-inferior.c"
    },
    {
      "type": "add",
      "old_id": "0000000000000000000000000000000000000000",
      "old_mode": 0,
      "old_path": "/dev/null",
      "new_id": "5fcd1ef2e3952b07a256ee5498c5b0b813846ba6",
      "new_mode": 33188,
      "new_path": "gdb/testsuite/gdb.multi/pending-bp-del-inferior.exp"
    }
  ]
}