> On Feb 22, 2017, at 12:33 PM, Mark Lam <[email protected]> wrote:
>
>
>> On Feb 22, 2017, at 12:16 PM, Filip Pizlo <[email protected]
>> <mailto:[email protected]>> wrote:
>>
>>
>>> On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <[email protected]
>>> <mailto:[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.
>
> 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).
This never works for me. I tested it locally. LLVM will even coalesce similar
inline assembly.
>
>> 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).
Why not do the preserve/restore inside the WTFReport call?
> 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 <[email protected]
>>>> <mailto:[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]
>>>>> <mailto:[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]
>>>>>> <mailto:[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]
>>>>>>> <mailto:[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] <mailto:[email protected]>
>>>>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> webkit-dev mailing list
>>>>> [email protected] <mailto:[email protected]>
>>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>>>
>>>> _______________________________________________
>>>> webkit-dev mailing list
>>>> [email protected] <mailto:[email protected]>
>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>>
>>
>
_______________________________________________
webkit-dev mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-dev
