<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Feb 22, 2017, at 12:16 PM, Filip Pizlo <<a href="mailto:fpizlo@apple.com" class="">fpizlo@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class=""><br class=""><blockquote type="cite" class="">On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <<a href="mailto:ggaren@apple.com" class="">ggaren@apple.com</a>> wrote:<br class=""><br class="">I’ve lost countless hours to investigating CrashTracers that would have been easy to solve if I had access to register state.<br class=""></blockquote><br class="">The current RELEASE_ASSERT means that every assertion in what the compiler thinks is a function (i.e. some function and everything inlined into it) is coalesced into a single trap site. I’d like to understand how you use the register state if you don’t even know which assertion you are at.<br class=""></div></div></blockquote><div><br class=""></div><div>Correction: they are not coalesced. I was mistaken about that. The fact that we turn them into inline asm (for emitting the int3) means the compiler cannot optimize it away or coalesce it. The compiler does move it to the end of the emitted code for the function though because we end the CRASH() macro with __builtin_unreachable().</div><div><br class=""></div><div>Hence, each int3 can be correlated back to the RELEASE_ASSERT that triggered it (with some extended disassembly work).</div><br class=""><blockquote type="cite" class=""><div class=""><div class="">I believe that if you do want to analyze register state, then switching back to calling some function that prints out diagnostic information is strictly better. Sure, you get less register state, but at least you know where you crashed. Knowing where you crashed is much more important than knowing the register state, since the register state is not useful if you don’t know where you crashed.<br class=""><br class=""></div></div></blockquote><div><br class=""></div><div>I would like to point out that we might be able to get the best of both worlds. Here’s how we can do it:</div><div><br class=""></div><div><div style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);" class=""><span style="font-variant-ligatures: no-common-ligatures" class="">define RELEASE_ASSERT(assertion) do { \</span></div><div style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""> if (UNLIKELY(!(assertion))) { \</span></div><div style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""> preserveRegisterState(); \</span></div><div style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);" class=""><span style="font-variant-ligatures: no-common-ligatures;" class=""> WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \</span></div><div style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);" class=""><span style="font-variant-ligatures: no-common-ligatures;" class=""> restoreRegisterState(); \</span></div><div style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""> CRASH(); \</span></div><div style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);" class=""> } \</div><div class=""><span style="font-variant-ligatures: no-common-ligatures" class=""><br class=""></span></div></div><div>preserveRegisterState() and restoreRegisterState() will carefully push and pop registers onto / off the stack (like how the JIT probe works).</div><div>This allows us to get a log message on the terminal when we’re running manually.</div><div><br class=""></div><div>In addition, we can capture some additional information about the assertion site by forcing the compiler to emit code to capture the code location info after the trapping instruction. This is redundant but provides an easy place to find this info (i.e. after the int3 instruction).</div><div><br class=""></div><div><div style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);" class=""><span style="font-variant-ligatures: no-common-ligatures" class="">#define WTFBreakpointTrap() do { \</span></div><div style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""> __asm__ volatile (</span><span style="font-variant-ligatures: no-common-ligatures; color: #d12f1b" class="">"int3"</span><span style="font-variant-ligatures: no-common-ligatures" class="">); \</span></div><div style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""> __asm__ volatile( </span><span style="font-variant-ligatures: no-common-ligatures; color: #d12f1b" class="">""</span><span style="font-variant-ligatures: no-common-ligatures" class=""> : : </span><span style="font-variant-ligatures: no-common-ligatures; color: #d12f1b" class="">"r"</span><span style="font-variant-ligatures: no-common-ligatures" class="">(__FILE__), </span><span style="font-variant-ligatures: no-common-ligatures; color: #d12f1b" class="">"r"</span><span style="font-variant-ligatures: no-common-ligatures" class="">(__LINE__), </span><span style="font-variant-ligatures: no-common-ligatures; color: #d12f1b" class="">"r"</span><span style="font-variant-ligatures: no-common-ligatures" class="">(WTF_PRETTY_FUNCTION)); \</span></div><div style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);" class=""><span style="font-variant-ligatures: no-common-ligatures" class=""> } while (false)</span></div><div class=""><span style="font-variant-ligatures: no-common-ligatures" class=""><br class=""></span></div></div><div>We can easily get the line number this way. However, the line number is not very useful by itself when we have inlining. Hence, I also capture the __FILE__ and WTF_PRETTY_FUNCTION. However, I haven’t been able to figure out how to decode those from the otool disassembler yet.</div><div><br class=""></div><div>The only downside of doing this extra work is that it increases the code size for each RELEASE_ASSERT site. This is probably insignificant in total.</div><div><br class=""></div><div>Performance-wise, it should be neutral-ish because the __builtin_unreachable() in the CRASH() macro + the UNLIKELY() macro would tell the compiler to put this in a slow path away from the main code path.</div><div><br class=""></div><div>Any thoughts on this alternative?</div><div><br class=""></div><div>Mark</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><blockquote type="cite" class=""><br class="">I also want the freedom to add RELEASE_ASSERT without ruining performance due to bad register allocation or making the code too large to inline. For example, hot paths in WTF::Vector use RELEASE_ASSERT.<br class=""></blockquote><br class="">Do we have data about the performance benefits of the current RELEASE_ASSERT implementation?<br class=""><br class=""><blockquote type="cite" class=""><br class="">Is some compromise solution possible?<br class=""><br class="">Some options:<br class=""><br class="">(1) Add a variant of RELEASE_ASSERT that takes a string and logs.<br class=""></blockquote><br class="">The point of C++ assert macros is that I don’t have to add a custom string. I want a RELEASE_ASSERT macro that automatically stringifies the expression and uses that as the string.<br class=""><br class="">If I had a choice between a RELEASE_ASSERT that can accurate report where it crashed but sometimes trashes the register state, and a RELEASE_ASSERT that always gives me the register state but cannot tell me which assert in the function it’s coming from, then I would always choose the one that can tell me where it crashed. That’s much more important, and the register state is not useful without that information.<br class=""><br class=""><blockquote type="cite" class=""><br class="">(2) Change RELEASE_ASSERT to do the normal debug ASSERT thing in Debug builds. (There’s not much need to preserve register state in debug builds.)<br class=""></blockquote><br class="">That would be nice, but doesn’t make RELEASE_ASSERT useful for debugging issues where timing is important. I no longer use RELEASE_ASSERTS for those kinds of assertions, because if I do it then I will never know where I crashed. So, I use the explicit:<br class=""><br class="">if (!thing) {<br class=""> dataLog(“…”);<br class=""> RELEASE_ASSERT_NOT_REACHED();<br class="">}<br class=""><br class="">-Filip<br class=""><br class=""><br class=""><blockquote type="cite" class=""><br class="">Geoff<br class=""><br class=""><blockquote type="cite" class="">On Feb 22, 2017, at 11:09 AM, Filip Pizlo <<a href="mailto:fpizlo@apple.com" class="">fpizlo@apple.com</a>> wrote:<br class=""><br class="">I disagree actually. I've lost countless hours to converting this:<br class=""><br class="">RELEASE_ASSERT(blah)<br class=""><br class="">into this:<br class=""><br class="">if (!blah) {<br class=""> dataLog("Reason why I crashed");<br class=""> RELEASE_ASSERT_NOT_REACHED();<br class="">}<br class=""><br class="">Look in the code - you'll find lots of stuff like this.<br class=""><br class="">I don't think analyzing register state at crashes is more important than keeping our code sane.<br class=""><br class="">-Filip<br class=""><br class=""><br class=""><blockquote type="cite" class="">On Feb 21, 2017, at 5:56 PM, Mark Lam <<a href="mailto:mark.lam@apple.com" class="">mark.lam@apple.com</a>> wrote:<br class=""><br class="">Oh yeah, I forgot about that. I think the register state is more important for crash analysis, especially if we can make sure that the compiler does not aggregate the int3s. I’ll explore alternatives.<br class=""><br class=""><blockquote type="cite" class="">On Feb 21, 2017, at 5:54 PM, Saam barati <<a href="mailto:sbarati@apple.com" class="">sbarati@apple.com</a>> wrote:<br class=""><br class="">I thought the main point of moving to SIGTRAP was to preserve register state?<br class=""><br class="">That said, there are probably places where we care more about the message than the registers.<br class=""><br class="">- Saam<br class=""><br class=""><blockquote type="cite" class="">On Feb 21, 2017, at 5:43 PM, Mark Lam <<a href="mailto:mark.lam@apple.com" class="">mark.lam@apple.com</a>> wrote:<br class=""><br class="">Is there a reason why RELEASE_ASSERT (and friends) does not call WTFReportAssertionFailure() to report where the assertion occur? Is this purely to save memory? svn blame tells me that it has been this way since the introduction of RELEASE_ASSERT in r140577 many years ago.<br class=""><br class="">Would anyone object to adding a call to WTFReportAssertionFailure() in RELEASE_ASSERT() like we do for ASSERT()? One of the upside (side-effect) of adding this call is that it appears to stop the compiler from aggregating all the RELEASE_ASSERTS into a single code location, and this will help with post-mortem crash debugging.<br class=""><br class="">Any thoughts?<br class=""><br class="">Mark<br class=""><br class="">_______________________________________________<br class="">webkit-dev mailing list<br class=""><a href="mailto:webkit-dev@lists.webkit.org" class="">webkit-dev@lists.webkit.org</a><br class="">https://lists.webkit.org/mailman/listinfo/webkit-dev<br class=""></blockquote><br class=""></blockquote><br class="">_______________________________________________<br class="">webkit-dev mailing list<br class=""><a href="mailto:webkit-dev@lists.webkit.org" class="">webkit-dev@lists.webkit.org</a><br class="">https://lists.webkit.org/mailman/listinfo/webkit-dev<br class=""></blockquote><br class="">_______________________________________________<br class="">webkit-dev mailing list<br class=""><a href="mailto:webkit-dev@lists.webkit.org" class="">webkit-dev@lists.webkit.org</a><br class="">https://lists.webkit.org/mailman/listinfo/webkit-dev<br class=""></blockquote><br class=""></blockquote><br class=""></div></div></blockquote></div><br class=""></body></html>