Mark, I know that you keep saying this. I remember you told me this even when I was sitting on a RELEASE_ASSERT that had gotten rage-coalesced.
Your reasoning sounds great, but this just isn't what happens in clang. __builtin_trap gets coalesced, as does inline asm. -Filip > On Feb 22, 2017, at 12:38 PM, Mark Lam <mark....@apple.com> wrote: > > For some context, we used to see aggregation of the CRASH() for > RELEASE_ASSERT() in the old days before we switched to using the int3 trap. > Back then we called a crash() function that never returns. As a result, the > C++ compiler was able to coalesce all the calls. With the int3 trap emitted > by inline asm, the C++ compiler has less ability to determine that the crash > sites have the same code (probably because it doesn’t bother comparing what’s > in the inline asm blobs). > > Mark > > >> On Feb 22, 2017, at 12:33 PM, Mark Lam <mark....@apple.com >> <mailto:mark....@apple.com>> wrote: >> >>> >>> On Feb 22, 2017, at 12:16 PM, Filip Pizlo <fpi...@apple.com >>> <mailto:fpi...@apple.com>> wrote: >>> >>> >>>> On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <gga...@apple.com >>>> <mailto:gga...@apple.com>> wrote: >>>> >>>> I’ve lost countless hours to investigating CrashTracers that would have >>>> been easy to solve if I had access to register state. >>> >>> 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. >> >> 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(). >> >> Hence, each int3 can be correlated back to the RELEASE_ASSERT that triggered >> it (with some extended disassembly work). >> >>> 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. >>> >> >> 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: >> >> define RELEASE_ASSERT(assertion) do { \ >> if (UNLIKELY(!(assertion))) { \ >> preserveRegisterState(); \ >> WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, >> #assertion); \ >> restoreRegisterState(); \ >> CRASH(); \ >> } \ >> >> preserveRegisterState() and restoreRegisterState() will carefully push and >> pop registers onto / off the stack (like how the JIT probe works). >> This allows us to get a log message on the terminal when we’re running >> manually. >> >> 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). >> >> #define WTFBreakpointTrap() do { \ >> __asm__ volatile ("int3"); \ >> __asm__ volatile( "" : : "r"(__FILE__), "r"(__LINE__), >> "r"(WTF_PRETTY_FUNCTION)); \ >> } while (false) >> >> 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. >> >> 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. >> >> 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. >> >> Any thoughts on this alternative? >> >> Mark >> >> >>>> >>>> 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. >>> >>> Do we have data about the performance benefits of the current >>> RELEASE_ASSERT implementation? >>> >>>> >>>> Is some compromise solution possible? >>>> >>>> Some options: >>>> >>>> (1) Add a variant of RELEASE_ASSERT that takes a string and logs. >>> >>> 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. >>> >>> 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. >>> >>>> >>>> (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.) >>> >>> 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: >>> >>> if (!thing) { >>> dataLog(“…”); >>> RELEASE_ASSERT_NOT_REACHED(); >>> } >>> >>> -Filip >>> >>> >>>> >>>> Geoff >>>> >>>>> On Feb 22, 2017, at 11:09 AM, Filip Pizlo <fpi...@apple.com >>>>> <mailto:fpi...@apple.com>> wrote: >>>>> >>>>> I disagree actually. I've lost countless hours to converting this: >>>>> >>>>> RELEASE_ASSERT(blah) >>>>> >>>>> into this: >>>>> >>>>> if (!blah) { >>>>> dataLog("Reason why I crashed"); >>>>> RELEASE_ASSERT_NOT_REACHED(); >>>>> } >>>>> >>>>> Look in the code - you'll find lots of stuff like this. >>>>> >>>>> I don't think analyzing register state at crashes is more important than >>>>> keeping our code sane. >>>>> >>>>> -Filip >>>>> >>>>> >>>>>> On Feb 21, 2017, at 5:56 PM, Mark Lam <mark....@apple.com >>>>>> <mailto:mark....@apple.com>> wrote: >>>>>> >>>>>> 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. >>>>>> >>>>>>> On Feb 21, 2017, at 5:54 PM, Saam barati <sbar...@apple.com >>>>>>> <mailto:sbar...@apple.com>> wrote: >>>>>>> >>>>>>> I thought the main point of moving to SIGTRAP was to preserve register >>>>>>> state? >>>>>>> >>>>>>> That said, there are probably places where we care more about the >>>>>>> message than the registers. >>>>>>> >>>>>>> - Saam >>>>>>> >>>>>>>> On Feb 21, 2017, at 5:43 PM, Mark Lam <mark....@apple.com >>>>>>>> <mailto:mark....@apple.com>> wrote: >>>>>>>> >>>>>>>> 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. >>>>>>>> >>>>>>>> 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. >>>>>>>> >>>>>>>> Any thoughts? >>>>>>>> >>>>>>>> Mark >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> webkit-dev mailing list >>>>>>>> webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> >>>>>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev >>>>>>>> <https://lists.webkit.org/mailman/listinfo/webkit-dev> >>>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> webkit-dev mailing list >>>>>> webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> >>>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev >>>>>> <https://lists.webkit.org/mailman/listinfo/webkit-dev> >>>>> >>>>> _______________________________________________ >>>>> webkit-dev mailing list >>>>> webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> >>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev >>>>> <https://lists.webkit.org/mailman/listinfo/webkit-dev> >>>> >>> >> >> _______________________________________________ >> webkit-dev mailing list >> webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> >> https://lists.webkit.org/mailman/listinfo/webkit-dev >> <https://lists.webkit.org/mailman/listinfo/webkit-dev>
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
