)]}' { "commit": "cc59d02b90cd34eb276f9609a2064011c7c4a001", "tree": "bf6405630a16b9d52a1b5edcc98c05ede8abf085", "parents": [ "bf616be99153b43c1077be9dbb7b081b4c080031" ], "author": { "name": "Andrew Burgess", "email": "aburgess@redhat.com", "time": "Wed Jan 31 11:18:34 2024 +0000" }, "committer": { "name": "Andrew Burgess", "email": "aburgess@redhat.com", "time": "Fri Jun 14 09:08:45 2024 +0100" }, "message": "gdbserver: update target description creation for x86/linux\n\nThis commit is part of a series which aims to share more of the target\ndescription creation between GDB and gdbserver for x86/Linux.\n\nAfter some refactoring earlier in this series the shared\nx86_linux_tdesc_for_tid function was added into nat/x86-linux-tdesc.c.\nHowever, this function still relies on amd64_linux_read_description\nand i386_linux_read_description which are implemented separately for\nboth gdbserver and GDB. Given that at their core, all these functions\ndo is:\n\n 1. take an xcr0 value as input,\n 2. mask out some feature bits,\n 3. look for a cached pre-generated target description and return it\n if found,\n 4. if no cached target description is found then call either\n amd64_create_target_description or\n i386_create_target_description to create a new target\n description, which is then added to the cache. Return the newly\n created target description.\n\nThe inner functions amd64_create_target_description and\ni386_create_target_description are already shared between GDB and\ngdbserver (in the gdb/arch/ directory), so the only thing that\nthe *_read_description functions really do is add the caching layer,\nand it feels like this really could be shared.\n\nHowever, we have a small problem.\n\nDespite using the same {amd64,i386}_create_target_description\nfunctions in both GDB and gdbserver to create the target descriptions,\non the gdbserver side we cache target descriptions based on a reduced\nset of xcr0 feature bits.\n\nWhat this means is that, in theory, different xcr0 values can map to\nthe same cache entry, which could result in the wrong target\ndescription being used.\n\nHowever, I\u0027m not sure if this can actually happen in reality. Within\ngdbserver we already split the target description cache based on i386,\namd64, and x32. I suspect within a given gdbserver session we\u0027ll only\nsee at most one target description for each of these.\n\nThe cache conflicting problem is caused by xcr0_to_tdesc_idx, which\nmaps an xcr0 value to a enum x86_linux_tdesc value, and there are only\n7 usable values in enum x86_linux_tdesc.\n\nIn contrast, on the GDB side there are 64, 32, and 16 cache slots for\ni386, amd64, and x32 respectively.\n\nOn the GDB side it is much more important to cache things correctly as\na single GDB session might connect to multiple different remote\ntargets, each of which might have slightly different x86\narchitectures.\n\nAnd so, if we want to merge the target description caching between GDB\nand gdbserver, then we need to first update gdbserver so that it\ncaches in the same way as GDB, that is, it needs to adopt a mechanism\nthat allows for the same number of cache slots of each of i386, amd64,\nand x32. In this way, when the caching is shared, GDB\u0027s behaviour\nwill not change.\n\nUnfortunately it\u0027s a little more complex than that due to the in\nprocess agent (IPA).\n\nWhen the IPA is in use, gdbserver sends a target description index to\nthe IPA, and the IPA uses this to find the correct target description\nto use, the IPA having first generated every possible target\ndescription.\n\nInterestingly, there is certainly a bug here which results from only\nhaving 7 values in enum x86_linux_tdesc. As multiple possible target\ndescriptions in gdbserver map to the same enum x86_linux_tdesc value,\nthen, when the enum x86_linux_tdesc value is sent to the IPA there is\nno way for gdbserver to know that the IPA will select the correct\ntarget description. This bug will get fixed by this commit.\n\n** START OF AN ASIDE **\n\nBack in the day I suspect this approach of sending a target\ndescription index made perfect sense. However since this commit:\n\n commit a8806230241d201f808d856eaae4d44088117b0c\n Date: Thu Dec 7 17:07:01 2017 +0000\n\n Initialize target description early in IPA\n\nI think that passing an index was probably a bad idea.\n\nWe used to pass the index, and then use that index to lookup which\ntarget description to instantiate and use, the target description was\nnot generated until the index arrived.\n\nHowever, the above commit fixed an issue where we can\u0027t call malloc()\nwithin (certain parts of) the IPA (apparently), so instead we now\npre-compute _every_ possible target description within the IPA. The\nindex is only used to lookup which of the (many) pre-computed target\ndescriptions to use.\n\nIt would (I think) have been easier all around if the IPA just\nself-inspected, figured out its own xcr0 value, and used that to\ncreate the one target description that is required. So long as the\nxcr0 to target description code is shared (at compile time) with\ngdbserver, then we can be sure that the IPA will derive the same\ntarget description as gdbserver, and we would avoid all this index\npassing business, which has made this commit so very, very painful.\n\nI did look at how a process might derive its own xcr0 value, but I\ndon\u0027t believe this is actually that simple, so for now I\u0027ve just\ndoubled down on the index passing approach.\n\nWhile reviewing earlier iterations of this patch there has been\ndiscussion about the possibility of removing the IPA from GDB. That\nwould certainly make all of the code touched in this patch much\nsimpler, but I don\u0027t really want to do that as part of this series.\n\n** END OF AN ASIDE **\n\nCurrently then for x86/linux, gdbserver sends a number between 0 and 7\nto the IPA, and the IPA uses this to create a target description.\n\nHowever, I am proposing that gdbserver should now create one of (up\nto) 64 different target descriptions for i386, so this 0 to 7 index\nisn\u0027t going to be good enough any more (amd64 and x32 have slightly\nfewer possible target descriptions, but still more than 8, so the\nproblem is the same).\n\nFor a while I wondered if I was going to have to try and find some\nbackward compatible solution for this mess. But after seeing how\nlightly the IPA is actually documented, I wonder if it is not the case\nthat there is a tight coupling between a version of gdbserver and a\nversion of the IPA? At least I\u0027m hoping so, and that\u0027s what I\u0027ve\nassumed in this commit.\n\nIn this commit I have thrown out the old IPA target description index\nnumbering scheme, and switched to a completely new numbering scheme.\nInstead of the index that is passed being arbitrary, the index is\ninstead calculated from the set of xcr0 features that are present on\nthe target. Within the IPA we can then reverse this logic to recreate\nthe xcr0 value based on the index, and from the xcr0 value we can\nchoose the correct target description.\n\nWith the gdbserver to IPA numbering scheme issue resolved I have then\nupdate the gdbserver versions of amd64_linux_read_description and\ni386_linux_read_description so that they cache target descriptions\nusing the same set of xcr0 features as GDB itself.\n\nAfter this gdbserver should now always come up with the same target\ndescription as GDB does on any x86/Linux target.\n\nThis commit does not introduce any new code sharing between GDB and\ngdbserver as previous commits in this series have done. Instead this\ncommit is all about bringing GDB and gdbserver into alignment\nfunctionally so that the next commit(s) can merge the GDB and\ngdbserver versions of these functions.\n\nNotes On The Implementation\n---------------------------\n\nPreviously, within gdbserver, target descriptions were cached within\narrays. These arrays were sized based on enum x86_linux_tdesc and\nxcr0_to_tdesc_idx returned the array (cache) index.\n\nNow we need different array lengths for each of i386, amd64, and x32.\nAnd the index to use within each array is calculated based on which\nxcr0 bits are set and valid for a particular target type.\n\nI really wanted to avoid having fixed array sizes, or having the set\nof relevant xcr0 bits encoded in multiple places.\n\nThe solution I came up with was to create a single data structure\nwhich would contain a list of xcr0 bits along with flags to indicate\nwhich of the i386, amd64, and x32 targets the bit is relevant for. By\nmaking the table constexpr, and adding some constexpr helper\nfunctions, it is possible to calculate the sizes for the cache arrays\nat compile time, as well as the bit masks needed to each target type.\n\nDuring review it was pointed out[1] that possibly the failure to check\nthe SSE and X87 bits for amd64/x32 targets might be an error, however,\nif this is the case then this is an issue that existed long before\nthis patch. I\u0027d really like to keep this patch focused on reworking\nthe existing code and try to avoid changing how target descriptions\nare actually created, mostly out of fear that I\u0027ll break something.\n\n[1] https://inbox.sourceware.org/gdb-patches/MN2PR11MB4566070607318EE7E669A5E28E1B2@MN2PR11MB4566.namprd11.prod.outlook.com\n\nApproved-By: John Baldwin \u003cjhb@FreeBSD.org\u003e\nApproved-By: Felix Willgerodt \u003cfelix.willgerodt@intel.com\u003e\n", "tree_diff": [ { "type": "modify", "old_id": "df5e6aca081fef9f1087350dd3b4aba3fe80069e", "old_mode": 33188, "old_path": "gdbserver/linux-amd64-ipa.cc", "new_id": "6f4ce6f116d1253b149e810bcfc7465d1bd03c1c", "new_mode": 33188, "new_path": "gdbserver/linux-amd64-ipa.cc" }, { "type": "modify", "old_id": "aa346fc9bc3f0f2d929d8947fe539e39a96bcb1f", "old_mode": 33188, "old_path": "gdbserver/linux-i386-ipa.cc", "new_id": "460f37e8389289439a04c8bdf9da3b1b2cbe6e02", "new_mode": 33188, "new_path": "gdbserver/linux-i386-ipa.cc" }, { "type": "modify", "old_id": "efb679793100fbd898a592fb2a9cad978df42f02", "old_mode": 33188, "old_path": "gdbserver/linux-x86-low.cc", "new_id": "99828a83ba9098faa07107ab517cfd8111941d7e", "new_mode": 33188, "new_path": "gdbserver/linux-x86-low.cc" }, { "type": "modify", "old_id": "af3735aa895df1a4033ff4fec4b4595950c9a534", "old_mode": 33188, "old_path": "gdbserver/linux-x86-tdesc.cc", "new_id": "f5636898d717194dc0a6665d1bc6ee9d34bf8ae3", "new_mode": 33188, "new_path": "gdbserver/linux-x86-tdesc.cc" }, { "type": "modify", "old_id": "576aaf5e1658205b9cc99b3337853aee0db9c69a", "old_mode": 33188, "old_path": "gdbserver/linux-x86-tdesc.h", "new_id": "c06af21da4890000d0e2c6147d72ace79582ca8c", "new_mode": 33188, "new_path": "gdbserver/linux-x86-tdesc.h" } ] }