<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:46 PM, Filip Pizlo &lt;<a href="mailto:fpizlo@apple.com" class="">fpizlo@apple.com</a>&gt; wrote:</div><br class="Apple-interchange-newline"><div class=""><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><div class=""><br class="Apple-interchange-newline">On Feb 22, 2017, at 12:41 PM, Mark Lam &lt;<a href="mailto:mark.lam@apple.com" class="">mark.lam@apple.com</a>&gt; wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" class=""><div class=""><br class="Apple-interchange-newline">On Feb 22, 2017, at 12:35 PM, Filip Pizlo &lt;<a href="mailto:fpizlo@apple.com" class="">fpizlo@apple.com</a>&gt; wrote:</div><br class="Apple-interchange-newline"><div class=""><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class=""><br class="Apple-interchange-newline">On Feb 22, 2017, at 12:33 PM, Mark Lam &lt;<a href="mailto:mark.lam@apple.com" class="">mark.lam@apple.com</a>&gt; wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Feb 22, 2017, at 12:16 PM, Filip Pizlo &lt;<a href="mailto:fpizlo@apple.com" class="">fpizlo@apple.com</a>&gt; 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 &lt;<a href="mailto:ggaren@apple.com" class="">ggaren@apple.com</a>&gt; 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. &nbsp;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 class=""><br class=""></div><div class="">Correction: they are not coalesced. &nbsp;I was mistaken about that. &nbsp;The fact that we turn them into inline asm (for emitting the int3) means the compiler cannot optimize it away or coalesce it. &nbsp;The compiler does move it to the end of the emitted code for the function though because we end the CRASH() macro with&nbsp;__builtin_unreachable().</div><div class=""><br class=""></div><div class="">Hence, each int3 can be correlated back to the RELEASE_ASSERT that triggered it (with some extended disassembly work).</div></div></div></div></blockquote><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class=""></div><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;">This never works for me. &nbsp;I tested it locally. &nbsp;LLVM will even coalesce similar inline assembly.</div></div></blockquote><div class=""><br class=""></div><div class="">With my proposal, I’m emitting different inline asm now after the int3 trap because I’m embedding line number and file strings. &nbsp;Hence, even if the compiler is smart enough to compare inline asm code blobs, it will find them to be different, and hence, it doesn’t make sense to coalesce.</div></div></div></blockquote><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""></div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">Are you claiming that LLVM does not currently now coalesce RELEASE_ASSERTS, or that it will not coalesce them anymore after you make some change?</div></div></blockquote><div><br class=""></div><div>Here, I’m claiming that it will not coalesce after I make some changes.</div><br class=""><blockquote type="cite" class=""><div class=""><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><div class=""><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class=""><blockquote type="cite" class=""><div class=""><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class=""><div class="" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div class=""><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. &nbsp;Sure, you get less register state, but at least you know where you crashed. &nbsp;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 class=""><br class=""></div><div class="">I would like to point out that we might be able to get the best of both worlds. &nbsp;Here’s how we can do it:</div><div class=""><br class=""></div><div class=""><div class="" style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);"><span class="" style="font-variant-ligatures: no-common-ligatures;">define RELEASE_ASSERT(assertion) do { \</span></div><div class="" style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);"><span class="" style="font-variant-ligatures: no-common-ligatures;">&nbsp; &nbsp; if (UNLIKELY(!(assertion))) { \</span></div><div class="" style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);"><span class="" style="font-variant-ligatures: no-common-ligatures;">&nbsp; &nbsp; &nbsp; &nbsp; preserveRegisterState(); \</span></div><div class="" style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);"><span class="" style="font-variant-ligatures: no-common-ligatures;">&nbsp; &nbsp; &nbsp; &nbsp;&nbsp;WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, #assertion); \</span></div><div class="" style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);"><span class="" style="font-variant-ligatures: no-common-ligatures;">&nbsp; &nbsp; &nbsp; &nbsp; restoreRegisterState(); \</span></div><div class="" style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);"><span class="" style="font-variant-ligatures: no-common-ligatures;">&nbsp; &nbsp; &nbsp; &nbsp; CRASH(); \</span></div><div class="" style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);">&nbsp; &nbsp; } \</div><div class=""><span class="" style="font-variant-ligatures: no-common-ligatures;"><br class=""></span></div></div><div class="">preserveRegisterState() and&nbsp;restoreRegisterState() will carefully push and pop registers onto / off the stack (like how the JIT probe works).</div></div></div></div></blockquote><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class=""></div><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Why not do the preserve/restore inside the WTFReport call?</div></div></blockquote><div class=""><br class=""></div><div class="">Because I would like to preserve the register values that were used in the comparison that failed the assertion.</div></div></div></blockquote><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""></div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">That doesn't change anything. &nbsp;You can create a WTFFail that is written in assembly and first saves all registers, and restores them prior to trapping.</div></div></blockquote><div><br class=""></div><div>A meaningful call here requires passing __FILE__, __LINE__,&nbsp;WTF_PRETTY_FUNCTION, and #assertion as arguments. &nbsp;Hence, this will necessarily perturb register state at the call site. &nbsp;The compiler is also free to load the reporting function into a register to make the call. &nbsp;My approach of preserving regs before any code the compiler emits to make the call guarantees that we have the register immediately after the assertion compare.</div><div><br class=""></div><div>Mark</div><br class=""><blockquote type="cite" class=""><div class=""><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""></div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class="">-Filip</div><div style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br class=""></div><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><div class=""><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class=""><br class=""></div>Mark</div><div class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class=""><blockquote type="cite" class=""><div class=""><br class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" class="" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><div class=""><div class="" style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div class=""><div class="">This allows us to get a log message on the terminal when we’re running manually.</div><div class=""><br class=""></div><div class="">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. &nbsp;This is redundant but provides an easy place to find this info (i.e. after the int3 instruction).</div><div class=""><br class=""></div><div class=""><div class="" style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);"><span class="" style="font-variant-ligatures: no-common-ligatures;">#define WTFBreakpointTrap() do { \</span></div><div class="" style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);"><span class="" style="font-variant-ligatures: no-common-ligatures;">&nbsp; &nbsp; &nbsp; &nbsp; __asm__ volatile (</span><span class="" style="font-variant-ligatures: no-common-ligatures; color: rgb(209, 47, 27);">"int3"</span><span class="" style="font-variant-ligatures: no-common-ligatures;">); \</span></div><div class="" style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);"><span class="" style="font-variant-ligatures: no-common-ligatures;">&nbsp; &nbsp; &nbsp; &nbsp; __asm__ volatile(<span class="Apple-converted-space">&nbsp;</span></span><span class="" style="font-variant-ligatures: no-common-ligatures; color: rgb(209, 47, 27);">""</span><span class="" style="font-variant-ligatures: no-common-ligatures;"><span class="Apple-converted-space">&nbsp;</span>:&nbsp; :<span class="Apple-converted-space">&nbsp;</span></span><span class="" style="font-variant-ligatures: no-common-ligatures; color: rgb(209, 47, 27);">"r"</span><span class="" style="font-variant-ligatures: no-common-ligatures;">(__FILE__),<span class="Apple-converted-space">&nbsp;</span></span><span class="" style="font-variant-ligatures: no-common-ligatures; color: rgb(209, 47, 27);">"r"</span><span class="" style="font-variant-ligatures: no-common-ligatures;">(__LINE__),<span class="Apple-converted-space">&nbsp;</span></span><span class="" style="font-variant-ligatures: no-common-ligatures; color: rgb(209, 47, 27);">"r"</span><span class="" style="font-variant-ligatures: no-common-ligatures;">(WTF_PRETTY_FUNCTION)); \</span></div><div class="" style="margin: 0px; line-height: normal; font-family: Menlo; color: rgb(120, 73, 42);"><span class="" style="font-variant-ligatures: no-common-ligatures;">&nbsp; &nbsp; } while (false)</span></div><div class=""><span class="" style="font-variant-ligatures: no-common-ligatures;"><br class=""></span></div></div><div class="">We can easily get the line number this way. &nbsp;However, the line number is not very useful by itself when we have inlining. &nbsp;Hence, I also capture the __FILE__ and&nbsp;WTF_PRETTY_FUNCTION. &nbsp;However, I haven’t been able to figure out how to decode those from the otool disassembler yet.</div><div class=""><br class=""></div><div class="">The only downside of doing this extra work is that it increases the code size for each RELEASE_ASSERT site. &nbsp;This is probably insignificant in total.</div><div class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">Any thoughts on this alternative?</div><div class=""><br class=""></div><div class="">Mark</div><div class=""><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. &nbsp;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. &nbsp;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. &nbsp;I no longer use RELEASE_ASSERTS for those kinds of assertions, because if I do it then I will never know where I crashed. &nbsp;So, I use the explicit:<br class=""><br class="">if (!thing) {<br class="">&nbsp;&nbsp;dataLog(“…”);<br class="">&nbsp;&nbsp;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 &lt;<a href="mailto:fpizlo@apple.com" class="">fpizlo@apple.com</a>&gt; wrote:<br class=""><br class="">I disagree actually. &nbsp;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 &lt;<a href="mailto:mark.lam@apple.com" class="">mark.lam@apple.com</a>&gt; wrote:<br class=""><br class="">Oh yeah, I forgot about that. &nbsp;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. &nbsp;I’ll explore alternatives.<br class=""><br class=""><blockquote type="cite" class="">On Feb 21, 2017, at 5:54 PM, Saam barati &lt;<a href="mailto:sbarati@apple.com" class="">sbarati@apple.com</a>&gt; 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 &lt;<a href="mailto:mark.lam@apple.com" class="">mark.lam@apple.com</a>&gt; wrote:<br class=""><br class="">Is there a reason why RELEASE_ASSERT (and friends) does not call WTFReportAssertionFailure() to report where the assertion occur? &nbsp;Is this purely to save memory? &nbsp;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()? &nbsp;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=""><a href="https://lists.webkit.org/mailman/listinfo/webkit-dev" class="">https://lists.webkit.org/mailman/listinfo/webkit-dev</a><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=""><a href="https://lists.webkit.org/mailman/listinfo/webkit-dev" class="">https://lists.webkit.org/mailman/listinfo/webkit-dev</a><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=""><a href="https://lists.webkit.org/mailman/listinfo/webkit-dev" class="">https://lists.webkit.org/mailman/listinfo/webkit-dev</a></blockquote></blockquote></div></div></blockquote></div></div></div></blockquote></div></blockquote></div></div></blockquote></div></blockquote></div><br class=""></body></html>