{"action":"created","issue":{"url":"https://api.github.com/repos/dolphin-emu/dolphin/issues/4198","repository_url":"https://api.github.com/repos/dolphin-emu/dolphin","labels_url":"https://api.github.com/repos/dolphin-emu/dolphin/issues/4198/labels{/name}","comments_url":"https://api.github.com/repos/dolphin-emu/dolphin/issues/4198/comments","events_url":"https://api.github.com/repos/dolphin-emu/dolphin/issues/4198/events","html_url":"https://github.com/dolphin-emu/dolphin/pull/4198","id":175646041,"number":4198,"title":"Fix the memChecks breaking a second time after hitting one and unpausing","user":{"login":"aldelaro5","id":8932978,"avatar_url":"https://avatars.githubusercontent.com/u/8932978?v=3","gravatar_id":"","url":"https://api.github.com/users/aldelaro5","html_url":"https://github.com/aldelaro5","followers_url":"https://api.github.com/users/aldelaro5/followers","following_url":"https://api.github.com/users/aldelaro5/following{/other_user}","gists_url":"https://api.github.com/users/aldelaro5/gists{/gist_id}","starred_url":"https://api.github.com/users/aldelaro5/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/aldelaro5/subscriptions","organizations_url":"https://api.github.com/users/aldelaro5/orgs","repos_url":"https://api.github.com/users/aldelaro5/repos","events_url":"https://api.github.com/users/aldelaro5/events{/privacy}","received_events_url":"https://api.github.com/users/aldelaro5/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2016-09-08T01:44:22Z","updated_at":"2016-09-08T02:14:00Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/dolphin-emu/dolphin/pulls/4198","html_url":"https://github.com/dolphin-emu/dolphin/pull/4198","diff_url":"https://github.com/dolphin-emu/dolphin/pull/4198.diff","patch_url":"https://github.com/dolphin-emu/dolphin/pull/4198.patch"},"body":"This is quite the pecular and weird issue so I will explain what went wrong here.\r\n\r\nWhen the CPU is unpaused and memchecks are enabled, it needs to perform a single step to prevent issues with the debugger control features (according to a comment in CPU.cpp).  The problem is this single step will perform the current instruction again and so, this includes doing another check with the memChecks list.  Naturally, this check will trigger a break if a matching one with appropriate flags has been found.\r\n\r\nSo consider the following scenario: the user adds a memCheck, eventually this memCheck would hit and after that, the user unpause the emulator.\r\n\r\nWhen unpausing, it will single step on the same instruction the memCheck hit earlier which means it would be the same load/store instruction used to hit the memCheck in the first place.  Because of this, it will pass and trigger a second break, but this break would happen just before unpausing which is enough time to break after one or two instructions.\r\n\r\nI propose this solution: simply disable (not remove) all memChecks before the step and enable them back after the step.  To do this, I needed a flag, so I wanted to use the break flag of the memChecks.....but then I foudn a pretty silly mistake: despite the code caring about setting that flag appropriately, it never was used to check whether to break or not :)\r\n\r\nSo I went ahead and changed the break condition.  MemChecks only triggers once now.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/dolphin-emu/dolphin/4198)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/dolphin-emu/dolphin/issues/comments/245474708","html_url":"https://github.com/dolphin-emu/dolphin/pull/4198#issuecomment-245474708","issue_url":"https://api.github.com/repos/dolphin-emu/dolphin/issues/4198","id":245474708,"user":{"login":"aldelaro5","id":8932978,"avatar_url":"https://avatars.githubusercontent.com/u/8932978?v=3","gravatar_id":"","url":"https://api.github.com/users/aldelaro5","html_url":"https://github.com/aldelaro5","followers_url":"https://api.github.com/users/aldelaro5/followers","following_url":"https://api.github.com/users/aldelaro5/following{/other_user}","gists_url":"https://api.github.com/users/aldelaro5/gists{/gist_id}","starred_url":"https://api.github.com/users/aldelaro5/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/aldelaro5/subscriptions","organizations_url":"https://api.github.com/users/aldelaro5/orgs","repos_url":"https://api.github.com/users/aldelaro5/repos","events_url":"https://api.github.com/users/aldelaro5/events{/privacy}","received_events_url":"https://api.github.com/users/aldelaro5/received_events","type":"User","site_admin":false},"created_at":"2016-09-08T02:14:00Z","updated_at":"2016-09-08T02:14:00Z","body":"\n\n\n\nReview status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.\n\n---\n\n*[Source/Core/Common/BreakPoints.cpp, line 229 \\[r1\\]](https://reviewable.io:443/reviews/dolphin-emu/dolphin/4198#-KR6UvHcXKx7lmE8gywg:-KR6ZVjvDbPCze4djVwY:b-896fix) ([raw file](https://github.com/dolphin-emu/dolphin/blob/3ab536dfce0b4d154c29d16d84e748a7ffc3b0f5/Source/Core/Common/BreakPoints.cpp#L229)):*\n<details><summary><i>Previously, lioncash (Mat M.) wrote…</i></summary>\n> `Break` doesn't need to be parenthesized\n</details>\nDone.\n\n---\n\n*[Source/Core/Common/BreakPoints.h, line 99 \\[r1\\]](https://reviewable.io:443/reviews/dolphin-emu/dolphin/4198#-KR6VCa0Aiikld6frvqy:-KR6ZWpDEM9cug2USbg8:bgxosqc) ([raw file](https://github.com/dolphin-emu/dolphin/blob/3ab536dfce0b4d154c29d16d84e748a7ffc3b0f5/Source/Core/Common/BreakPoints.h#L99)):*\n<details><summary><i>Previously, lioncash (Mat M.) wrote…</i></summary>\n> These functions can just be combined into a single function\n> \n> ```cpp\n> // In header\n> void EnableAllMemChecks(bool enable = true);\n> \n> // In cpp\n> void MemChecks::EnableAllMemChecks(bool enable)\n> {\n>   for (TMemCheck& mc : m_MemChecks)\n>   {\n>     mc.Break = enable;\n>   }\n> }\n> ```\n> \n> then if you want to provide the other function (if only to save on typing), you can implement that in terms of the enabling function.\n> \n> \n> ```cpp\n> // In header\n> void DisableAllMemChecks();\n> \n> // In cpp\n> void MemChecks::DisableAllMemChecks()\n> {\n>   EnableAllMemChecks(false);\n> }\n> ```\n</details>\nDone.  Thanks for that tip btw, it's more elegant.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/dolphin-emu/dolphin/4198)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"aldelaro5"}}