> On Feb 22, 2017, at 12:16 PM, Filip Pizlo <[email protected]> wrote:
>
>
>> On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <[email protected]> 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.
When I disassemble JavaScriptCore, I often find a succession of int3s at the
bottom of a function. Does LLVM sometimes combine them and sometimes not?
For example, this is what the bottom of the
__ZN3JSC20AbstractModuleRecord18getModuleNamespaceEPNS_9ExecStateE looks like:
0000000000005c25 popq %r14
0000000000005c27 popq %r15
0000000000005c29 popq %rbp
0000000000005c2a retq
0000000000005c2b int3
0000000000005c2c int3
0000000000005c2d int3
0000000000005c2e int3
0000000000005c2f nop
- Saam
>
> 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 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
>>>>>> [email protected]
>>>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>>>>
>>>>
>>>> _______________________________________________
>>>> webkit-dev mailing list
>>>> [email protected]
>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>>
>>> _______________________________________________
>>> webkit-dev mailing list
>>> [email protected]
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>
>
> _______________________________________________
> webkit-dev mailing list
> [email protected]
> https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________
webkit-dev mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-dev
