> On Feb 22, 2017, at 12:41 PM, Mark Lam <[email protected]> wrote:
>
>>
>> On Feb 22, 2017, at 12:35 PM, Filip Pizlo <[email protected]
>> <mailto:[email protected]>> wrote:
>>
>>>
>>> On Feb 22, 2017, at 12:33 PM, Mark Lam <[email protected]
>>> <mailto:[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.
>
> With my proposal, I’m emitting different inline asm now after the int3 trap
> because I’m embedding line number and file strings. 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.
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?
>
>>
>>>
>>>> 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?
>
> Because I would like to preserve the register values that were used in the
> comparison that failed the assertion.
That doesn't change anything. You can create a WTFFail that is written in
assembly and first saves all registers, and restores them prior to trapping.
-Filip
>
> Mark
>
>>
>>> 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
>>>>>>>>> <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
>>>>>>> <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
>>>>>> <https://lists.webkit.org/mailman/listinfo/webkit-dev>
_______________________________________________
webkit-dev mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-dev
