Closed Bug 914402 Opened 6 years ago Closed 5 years ago
GC: remove the conservative stack scanner and associated machinery
There are several excellent reasons why we should remove the dynamic analysis: 1) The last several issues the dynamic rooting analysis has found have been false positives -- missing skiproots on stack integers, etc. This will be a recurring problem. 2) GGC with gczeal can catch the same problems and in practice has already caught several things that the dynamic root analysis should have caught ages ago. GGC+zeal runs ~2x faster than the dynamic rooting analysis, so it can try twice as many test cases in the same around of time. 3) Removing the dynamic rooting analysis will allow us to remove the conservative stack scanner, killing a bunch of really ugly code. Arguments for keeping the analysis and counter-argument: 1) It catches rooting hazards on things we do not currently move. Reply: adding a configure flag that does move those things and then fuzzing against that configuration is trivial. 2) Root analysis crashes are easier to debug. Reply: It depends, but generally the difference is not significant. 3) GGC will re-use object data, potentially quite frequently when using gczeal, making crashes much harder to track down. Reply: Correct. This bug is about fixing that problem. The solution should be much less code and much less complexity than conservative scanning and the dynamic root analysis. I'd like to move forward with making zeal+ggc a viable option right now so that it can help us with landing exact rooting. We can rip out the rest when we remove the conservative stack scanner.
Spoke to Terrence over IRC and there is a chance this will remove the need for the --enable-root-analysis option.
Now that GC zeal mode 7 has landed and is actively finding bugs, I'm morphing this to be our bug for removing the conservative scanner, including the root analysis machinery.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8486758 - Flags: review?(sphink)
Comment on attachment 8486758 [details] [diff] [review] rip_out_conservative_stack_scanning-v0.diff Review of attachment 8486758 [details] [diff] [review]: ----------------------------------------------------------------- Sort of sad to see this go, since it's pretty cool to be able to query a random pointer and find out what it is. But I sure don't want to maintain it!
Attachment #8486758 - Flags: review?(sphink) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f12964fe26 Try run at: https://tbpl.mozilla.org/?tree=Try&rev=fb9db399ea10 It's as green as what's under it: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b7d2c160e1f0 This patch only removes unused code, so the important thing here is that it still builds on all platforms -- I would be very surprised if it caused bustage other than missing symbols.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.