dca8e34d8b272526f84887c971036780b1ad2c19: Bug 1586452: Separate removal and destruction of Breakpoints into two methods. r=jonco
Jim Blandy <jimb@mozilla.com> - Wed, 23 Oct 2019 19:49:55 +0000 - rev 563967
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Bug 1586452: Separate removal and destruction of Breakpoints into two methods. r=jonco Separate `Breakpoint::destroy` into two functions, `Breakpoint::remove`, which takes care of cleaning up empty sites, and `Breakpoint::delete_`, which only removes the breakpoint from its linked lists and frees its storage. Call `Breakpoint::remove` from sites that passed `True` for `destroy`'s `mayDestroySite` argument, and `Breakpoint::delete_` for those that passed `False`. The prior code had a `Breakpoint::destroy` method that took a `mayDestroySite` flag, which indicated whether the BreakpointSite should also be cleaned up, if the outgoing breakpoint was the last one left. Given that we never intend to leave empty BreakpointSites lying around, it was unclear why this option was needed. And indeed, it was passed as `True` (yes, please destroy the site if empty) from all call sites except one, in `js::wasm::DebugState::clearBreakpointsIn`. This function is used for both deleting individual breakpoints and removing all breakpoints set in a particular `wasm::Instance`, so it has two nested loops, the outer visiting all sites in the instance, and the inner visiting each site's breakpoints. Since `DebugState` stores `WasmBreakpointSites` in a hash table, allowing `Breakpoint::destroy` to remove empty `BreakpointSite`s would invalidate the outer loop's iteration, a classic bug. Instead, the inner loop forbade `Breakpoint::destroy` from removing the site, and cleaned it up itself, if empty, at the bottom of the outer loop body. Later patches in this series want a cleaner separation between the high-level operation of removing a single breakpoint, with whatever followup that entails, versus the low-level of operation of simply freeing the structure at hand. Differential Revision: https://phabricator.services.mozilla.com/D49857
df4696b00c376826a45b750bcb37c9cbdf0fdcd5: Bug 1586452: SetBreakpointMatcher: success path should be main line. r=jonco
Jim Blandy <jimb@mozilla.com> - Wed, 23 Oct 2019 19:49:43 +0000 - rev 563966
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Bug 1586452: SetBreakpointMatcher: success path should be main line. r=jonco No effect on behavior. It's SpiderMonkey's convention that the success path should be the top-level path through a function, and that failure should be handled by early returns. This patch makes SetBreakpointMatcher's methods conform to that convention. Differential Revision: https://phabricator.services.mozilla.com/D49856
b49320ce9db53381350d9ed548d951b9d6db2a7c: Bug 1586452: Manage JIT traps when we create and remove breakpoint sites. r=jonco
Jim Blandy <jimb@mozilla.com> - Wed, 23 Oct 2019 19:49:35 +0000 - rev 563965
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Bug 1586452: Manage JIT traps when we create and remove breakpoint sites. r=jonco `Debugger` objects used to have an `enabled` property, which controlled (among other things) whether breakpoints would fire. This meant that it was possible to have a BreakpointSite that had breakpoints set in it, but yet should never trigger. Thus, whether or not the JIT code itself should be patched for the breakpoint depended on both 1) whether breakpoints were set, and 2) whether any of those breakpoints' owning Debuggers were enabled. To manage this, each BreakpointSite had not only a linked list of inserted breakpoints, but an 'enabled count' of how many of those breakpoints were in enabled Debuggers. Enabling/disabling a Debugger walked all its breakpoints and adjusted their sites' counts. When the enabled count was non-zero, the JIT code needed traps inserted; when it was zero, the trap sites could be filled with no-ops. The `enabled` property seemed like an innocent idea, but it turns out to have introduced all sorts of hair like this throughout the Debugger (for example, also making it harder for the GC to figure out what could be removed without observable effect), and then it turned out that the server never really wanted to toggle them on and off anyway. So we removed 'enabled' in bug 1564168. But this means that a BreakpointSite's enabled count should be non-zero if and only if its breakpoint list is non-empty; the count is redundant and can be removed. Further, since an empty BreakpointSite should be removed altogether, any inserted BreakpointSite can be assumed to have a non-zero enabled count. In other words, JIT code should have traps inserted if and only if a BreakpointSite is present for that code location. This means we can tie JIT code trap insertion and removal to BreakpointSite insertion and removal. Since BreakpointSite is an abstract base class, all the code that inserts and removes sites knows which concrete class it's creating, so we know statically how to go about patching the code. This means BreakpointSite no longer needs a 'recompile' pure virtual method. Its implementations get merged into the code that's inserting and removing BreakpointSite concrete instances. Differential Revision: https://phabricator.services.mozilla.com/D49855
7fc4169017bc5c1a0a97559e6f6382c25c32c6bb: Bug 1586452: Make `js::BreakpointSite::remove` the pure virtual method, and hoist `removeIfEmpty' into the base class. r=jonco
Jim Blandy <jimb@mozilla.com> - Wed, 23 Oct 2019 19:49:28 +0000 - rev 563964
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Bug 1586452: Make `js::BreakpointSite::remove` the pure virtual method, and hoist `removeIfEmpty' into the base class. r=jonco The way a breakpoint site is removed depends on the concrete subclass, but deciding whether the site is empty or not is common to all BreakpointSites. There's no reason to repeat the condition in every implementation. Differential Revision: https://phabricator.services.mozilla.com/D49854
4d3fe0b933978a05af738c8a8729fe1877cd0db2: Bug 1586452: Remove class js::WasmBreakpoint; use plain js::Breakpoint instead. r=jonco
Jim Blandy <jimb@mozilla.com> - Wed, 23 Oct 2019 19:49:21 +0000 - rev 563963
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Bug 1586452: Remove class js::WasmBreakpoint; use plain js::Breakpoint instead. r=jonco All the information we need specific to Wasm breakpoints is available in the WasmBreakpointSite now, so there's no need for two different kinds of breakpoints any more. Code that used to obtain the wasm instance from the breakpoint now consults the site instead. BreakpointSite::allocSize confusingly provided the size of the site's *breakpoints*, not the site itself; but now it's no longer needed at all, and can be removed. Differential Revision: https://phabricator.services.mozilla.com/D49853
d0d79b88fd494ae87cbea8ca5f7a6ed364dca3f5: Bug 1586452: Let WasmBreakpointSite hold a WasmInstanceObject*, not a wasm::Instance*. r=jonco
Jim Blandy <jimb@mozilla.com> - Wed, 23 Oct 2019 19:49:08 +0000 - rev 563962
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Bug 1586452: Let WasmBreakpointSite hold a WasmInstanceObject*, not a wasm::Instance*. r=jonco WasmInstanceObjects and wasm::Instances are 1:1, so this should have no effect. But the next patch removes WasmBreakpoint altogether, and being able to get exactly the same information from the site makes that patch more obviously correct. Why not make them both hold wasm::Instance*? The IsMarkedUnbarriered checks in DebugAPI::markIteratively and Debugger::traceForMovingGC would really like an object pointer they can write to, which of course a wasm::Instance will not provide. Even though using WasmInstanceObject* in both spots requires more tracing calls, it's a shorter patch overall. (Those checks are deleted in the end, anyway.) Differential Revision: https://phabricator.services.mozilla.com/D49852
0a1e6f5aa6c309230cc045c711e4783bbb65cbb3: Bug 1586452: Make BreakpointSite constructor and destructor protected. r=jonco
Jim Blandy <jimb@mozilla.com> - Wed, 23 Oct 2019 19:49:01 +0000 - rev 563961
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Bug 1586452: Make BreakpointSite constructor and destructor protected. r=jonco As an abstract base class, BreakpointSite can't be constructed except as a base class. And since BreakpointSites are owned by implementation-specific structures like js::DebugScript (for JS breakpoints) or js::wasm::DebugState (for Wasm), only implementation-specific code should be destructing them. Differential Revision: https://phabricator.services.mozilla.com/D49851
1acb660e5fc4caa75392989ed97f9640f3182e2a: Bug 1586452: Use tighter type JSBreakpointSite where possible. r=jonco
Jim Blandy <jimb@mozilla.com> - Wed, 23 Oct 2019 19:48:54 +0000 - rev 563960
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Bug 1586452: Use tighter type JSBreakpointSite where possible. r=jonco Much of the code in js/src/debugger/DebugScript.{cpp,h} deals only with breakpoints set in JSScripts, which must be located at js::JSBreakpointSites, not just any kind of js::BreakpointSite. Using the looser type seems to have misled people into thinking they need to deal with more cases than they actually do, resulting in unnecessary dynamic checks and confusing `if` conditions. This patch changes js::DebugScript::breakpoints into an array of js::JSBreakpointSite*, not just js::BreakpointSite*, and then propagates the consequences of that through the code. Differential Revision: https://phabricator.services.mozilla.com/D49850
0a3621d900c8fd0f92f997b87877db90e36cf5f4: Bug 1590714 - Remove unused 'mozbuild_state_path'. r=chmanchester
Marco Castelluccio <mcastelluccio@mozilla.com> - Wed, 23 Oct 2019 19:37:41 +0000 - rev 563959
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Bug 1590714 - Remove unused 'mozbuild_state_path'. r=chmanchester Differential Revision: https://phabricator.services.mozilla.com/D50203
1708688569b40d0b8ba09f7a15d81fbacbb5cbf3: Bug 1580462 - Store iframe's FeaturePolicy in browsingContext to inherit cross origin document. r=baku,farre
Thomas Nguyen <tnguyen@mozilla.com> - Wed, 23 Oct 2019 19:39:00 +0000 - rev 563958
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Bug 1580462 - Store iframe's FeaturePolicy in browsingContext to inherit cross origin document. r=baku,farre Differential Revision: https://phabricator.services.mozilla.com/D48825
c1b6dd7e51db3fb114fd103418c6758dde74d78a: Bug 1589783 - Update sccache in-tree to the latest version. r=nalexander
Chris Manchester <cmanchester@mozilla.com> - Tue, 22 Oct 2019 16:40:34 +0000 - rev 563957
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Bug 1589783 - Update sccache in-tree to the latest version. r=nalexander Differential Revision: https://phabricator.services.mozilla.com/D49801
886758646be8a581f05eb621199fcff99e8236c2: Backed out changeset e2448f988af9 (bug 1590344) for causing python failures on configure/lint.py.
Cosmin Sabou <csabou@mozilla.com> - Wed, 23 Oct 2019 22:33:00 +0300 - rev 563956
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Backed out changeset e2448f988af9 (bug 1590344) for causing python failures on configure/lint.py.
5b359730e56868b617335ef3c21b769e971cdbe8: Bug 1590475 - Add batch decoding support for RemoteDataDecoder. r=mjf
Jean-Yves Avenard <jyavenard@mozilla.com> - Wed, 23 Oct 2019 18:20:57 +0000 - rev 563955
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Bug 1590475 - Add batch decoding support for RemoteDataDecoder. r=mjf Differential Revision: https://phabricator.services.mozilla.com/D50087
e2448f988af9e39a6957d44446b203c24759ea8c: Bug 1590344 - Don't unset PYTHONDONTWRITEBYTECODE anymore; r=firefox-build-system-reviewers,mshal
Anthony Ramine <nox@nox.paris> - Tue, 22 Oct 2019 21:05:53 +0000 - rev 563954
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Bug 1590344 - Don't unset PYTHONDONTWRITEBYTECODE anymore; r=firefox-build-system-reviewers,mshal Differential Revision: https://phabricator.services.mozilla.com/D50041
3a628b702db64e569d68648112fa2bab1b75ed22: Bug 1583970 - Cast sizeof to ptrdiff_t to avoid `-16 / 4 = big`. r=tsmith
Jeff Gilbert <jgilbert@mozilla.com> - Wed, 23 Oct 2019 17:29:30 +0000 - rev 563953
Push 2218 by ffxbld-merge at Mon, 30 Dec 2019 12:35:14 +0000
Bug 1583970 - Cast sizeof to ptrdiff_t to avoid `-16 / 4 = big`. r=tsmith Differential Revision: https://phabricator.services.mozilla.com/D50149
(0) -300000 -100000 -30000 -10000 -3000 -1000 -300 -100 -15 +15 +100 +300 +1000 +3000 +10000 +30000 +100000 tip