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> 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 >>>>>> >>>>> >>>>> _______________________________________________ >>>>> webkit-dev mailing list >>>>> webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> >>>>> 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 >>> >> > > _______________________________________________ > 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
