Quantcast

Why does RELEASE_ASSERT not have an error message?

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Why does RELEASE_ASSERT not have an error message?

Mark Lam
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
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Filip Pizlo
+1

-Filip

> On Feb 21, 2017, at 17:43, Mark Lam <[hidden email]> 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
> [hidden email]
> https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Saam barati-2
In reply to this post by Mark Lam
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 <[hidden email]> 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
> [hidden email]
> https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Mark Lam
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 <[hidden email]> 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 <[hidden email]> 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
>> [hidden email]
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Filip Pizlo
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
>>> [hidden email]
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>
>
> _______________________________________________
> webkit-dev mailing list
> [hidden email]
> https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

JF Bastien
Do we get the dataLog output in a crash report? It seems useful to have at a minimum some "reason" enum which ends up in a register, so the crash report is somewhat helpful, like we do with JIT code.

At that point the release assert might as well capture __LINE__ and other things in a register, so that crash reporter picks it up.

Not that the dataLog isn't useful -- it is! -- just not as useful if it isn't in crash reports.

On Wed, Feb 22, 2017 at 11:09 AM, Filip Pizlo <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
>>> [hidden email]
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>
>
> _______________________________________________
> webkit-dev mailing list
> [hidden email]
> https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Geoffrey Garen
In reply to this post by Filip Pizlo
I’ve lost countless hours to investigating CrashTracers that would have been easy to solve if I had access to register state.

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.

Is some compromise solution possible?

Some options:

(1) Add a variant of RELEASE_ASSERT that takes a string and logs.

(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.)

Geoff

> On Feb 22, 2017, at 11:09 AM, Filip Pizlo <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
>>>> [hidden email]
>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>>
>>
>> _______________________________________________
>> webkit-dev mailing list
>> [hidden email]
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
> _______________________________________________
> webkit-dev mailing list
> [hidden email]
> https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Saam barati-2
I like this idea. I think even doing what JF suggested could be really nice for suspected crashes. That way the register state would just tell us the reason.
If we do log, I think we should use WTFLogAlways instead of dataLog, because I think that does show up in some crash logs (but I could be wrong about this, I’m not 100% sure).

- Saam

> On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <[hidden email]> wrote:
>
> I’ve lost countless hours to investigating CrashTracers that would have been easy to solve if I had access to register state.
>
> 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.
>
> Is some compromise solution possible?
>
> Some options:
>
> (1) Add a variant of RELEASE_ASSERT that takes a string and logs.
>
> (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.)
>
> Geoff
>
>> On Feb 22, 2017, at 11:09 AM, Filip Pizlo <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
>>>>> [hidden email]
>>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>>>
>>>
>>> _______________________________________________
>>> webkit-dev mailing list
>>> [hidden email]
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>
>> _______________________________________________
>> webkit-dev mailing list
>> [hidden email]
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
> _______________________________________________
> webkit-dev mailing list
> [hidden email]
> https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Michael Catanzaro
In reply to this post by Geoffrey Garen
On Wed, 2017-02-22 at 11:58 -0800, Geoffrey Garen wrote:
> (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.)

This seems like a more desirable approach.

Michael
_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Filip Pizlo
In reply to this post by Geoffrey Garen

> On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <[hidden email]> 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.

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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
>>>>> [hidden email]
>>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>>>
>>>
>>> _______________________________________________
>>> webkit-dev mailing list
>>> [hidden email]
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>
>> _______________________________________________
>> webkit-dev mailing list
>> [hidden email]
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Saam barati-2

On Feb 22, 2017, at 12:16 PM, Filip Pizlo <[hidden email]> wrote:


On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Filip Pizlo

On Feb 22, 2017, at 12:23 PM, Saam barati <[hidden email]> wrote:


On Feb 22, 2017, at 12:16 PM, Filip Pizlo <[hidden email]> wrote:


On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <[hidden email]> 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?

Yeah.


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

I’m curious how many branches target those traps.

For example in the GC, I was often getting crashes that LLVM was convinced were vector overflow.  Turns out that the compiler loves to coalesce other traps with the one from the vector overflow assert, so if you assert for some random reason in a function that accesses vectors, all of our tooling will report with total confidence that you’re overflowing a vector.

That’s way worse than not having register state.

-Filip



- 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Mark Lam
In reply to this post by Filip Pizlo

On Feb 22, 2017, at 12:16 PM, Filip Pizlo <[hidden email]> wrote:


On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev




_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Filip Pizlo

On Feb 22, 2017, at 12:33 PM, Mark Lam <[hidden email]> wrote:


On Feb 22, 2017, at 12:16 PM, Filip Pizlo <[hidden email]> wrote:


On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev





_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Mark Lam
In reply to this post by Mark Lam
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 <[hidden email]> wrote:


On Feb 22, 2017, at 12:16 PM, Filip Pizlo <[hidden email]> wrote:


On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev



_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Mark Lam
In reply to this post by Filip Pizlo

On Feb 22, 2017, at 12:35 PM, Filip Pizlo <[hidden email]> wrote:


On Feb 22, 2017, at 12:33 PM, Mark Lam <[hidden email]> wrote:


On Feb 22, 2017, at 12:16 PM, Filip Pizlo <[hidden email]> wrote:


On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <[hidden email]> 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.



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.

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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Filip Pizlo
In reply to this post by Mark Lam
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 <[hidden email]> 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 <[hidden email]> wrote:


On Feb 22, 2017, at 12:16 PM, Filip Pizlo <[hidden email]> wrote:


On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev



_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev



_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Filip Pizlo
In reply to this post by Mark Lam

On Feb 22, 2017, at 12:41 PM, Mark Lam <[hidden email]> wrote:


On Feb 22, 2017, at 12:35 PM, Filip Pizlo <[hidden email]> wrote:


On Feb 22, 2017, at 12:33 PM, Mark Lam <[hidden email]> wrote:


On Feb 22, 2017, at 12:16 PM, Filip Pizlo <[hidden email]> wrote:


On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Mark Lam

On Feb 22, 2017, at 12:46 PM, Filip Pizlo <[hidden email]> wrote:


On Feb 22, 2017, at 12:41 PM, Mark Lam <[hidden email]> wrote:


On Feb 22, 2017, at 12:35 PM, Filip Pizlo <[hidden email]> wrote:


On Feb 22, 2017, at 12:33 PM, Mark Lam <[hidden email]> wrote:


On Feb 22, 2017, at 12:16 PM, Filip Pizlo <[hidden email]> wrote:


On Feb 22, 2017, at 11:58 AM, Geoffrey Garen <[hidden email]> 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?

Here, I’m claiming that it will not coalesce after I make some changes.





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.

A meaningful call here requires passing __FILE__, __LINE__, WTF_PRETTY_FUNCTION, and #assertion as arguments.  Hence, this will necessarily perturb register state at the call site.  The compiler is also free to load the reporting function into a register to make the call.  My approach of preserving regs before any code the compiler emits to make the call guarantees that we have the register immediately after the assertion compare.

Mark


-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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev

_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev


_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Why does RELEASE_ASSERT not have an error message?

Ryosuke Niwa-2
In reply to this post by Mark Lam
On Wed, Feb 22, 2017 at 12:41 PM, Mark Lam <[hidden email]> wrote:

>
> 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).
>

I'm afraid this would bloat the binary size too much. You can test but
I'd be surprised if this didn't negatively impact perf especially in
WebCore code.

- R. Niwa
_______________________________________________
webkit-dev mailing list
[hidden email]
https://lists.webkit.org/mailman/listinfo/webkit-dev
12
Loading...