Bug 174005 - DFG_ASSERT should allow stuffing registers before trapping.
Summary: DFG_ASSERT should allow stuffing registers before trapping.
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-29 19:17 PDT by Keith Miller
Modified: 2017-07-03 13:41 PDT (History)
14 users (show)

See Also:


Attachments
Patch (11.82 KB, patch)
2017-06-29 19:21 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (11.80 KB, patch)
2017-06-29 19:43 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (11.90 KB, patch)
2017-06-29 19:56 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (11.94 KB, patch)
2017-06-29 21:49 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (12.00 KB, patch)
2017-06-29 21:56 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (12.04 KB, patch)
2017-06-29 21:57 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (13.85 KB, patch)
2017-06-29 22:33 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (12.57 KB, patch)
2017-06-29 22:43 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2017-06-29 19:17:04 PDT
DFG_ASSERT should allow stuffing registers before trapping.
Comment 1 Keith Miller 2017-06-29 19:21:48 PDT
Created attachment 314207 [details]
Patch
Comment 2 Keith Miller 2017-06-29 19:43:25 PDT
Created attachment 314211 [details]
Patch
Comment 3 Keith Miller 2017-06-29 19:56:24 PDT
Created attachment 314214 [details]
Patch
Comment 4 Mark Lam 2017-06-29 20:56:30 PDT
Comment on attachment 314214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314214&action=review

> Source/JavaScriptCore/ChangeLog:12
> +        up to five registers right befor e crashing. The values stuffed

typo: /befor e/before/.

> Source/JavaScriptCore/dfg/DFGGraph.cpp:1455
> -    CRASH_WITH_SECURITY_IMPLICATION();
> +    WTFReportBacktrace();

I have a concern about removing CRASH_WITH_SECURITY_IMPLICATION() here.  I'm not certain, but I think CRASH_WITH_SECURITY_IMPLICATION() is used by the security folks for their special testing.  I see why you didn't move it into WAI_U_CRASH i.e. because CRASH_WITH_SECURITY_IMPLICATION() results in a function call which can wipe your registers.  We need to find a way to preserve this functionality so that the security folks can still use CRASH_WITH_SECURITY_IMPLICATION().

> Source/WTF/wtf/Assertions.h:472
> +ALWAYS_INLINE NO_RETURN_DUE_TO_CRASH void WAI_U_CRASH(Reason reason, A misc1, B misc2, C misc3, D misc4)

I think "WAI_U_CRASH" is a bit too cutesy (and very specific to contemporary American colloquialism).  Can we just call it CRASH_WITH_DETAILS instead?  That seems more clear for people who aren't so hip to the latest cultural jargon (like me).

> Source/WTF/wtf/Assertions.h:479
> +    CRASH();

Do something about the need for CRASH_WITH_SECURITY_IMPLICATION() here.

> Source/WTF/wtf/Assertions.h:534
> +template<typename Reason, typename A, typename B, typename C, typename D>
> +ALWAYS_INLINE NO_RETURN_DUE_TO_CRASH void WAI_U_CRASH(Reason reason, A misc1, B misc2, C misc3, D misc4)
> +{
> +    STUFF_REGISTER_FOR_CRASH("x16", reason);
> +    STUFF_REGISTER_FOR_CRASH("x17", misc1);
> +    STUFF_REGISTER_FOR_CRASH("x18", misc2);
> +    STUFF_REGISTER_FOR_CRASH("x19", misc3);
> +    STUFF_REGISTER_FOR_CRASH("x20", misc4);
> +    CRASH();
> +}

nit: Instead of duplicating the functions, how about just defining a set of registers like so:

#if CPU(X86_64) && OS(DARWIN)
#define CRASH_DETAIL_REGISTER1 "r11"
#define CRASH_DETAIL_REGISTER2 "r10"
...
#elif CPU(ARM64) && OS(DARWIN)
#define CRASH_DETAIL_REGISTER1 "x16"
#define CRASH_DETAIL_REGISTER2 "x17"
...
#endif

#if OS(DARWIN) && (CPU(X86_64) || CPU(ARM64))
template<typename Reason, typename A, typename B, typename C, typename D>
ALWAYS_INLINE NO_RETURN_DUE_TO_CRASH void CRASH_WITH_DETAILS(Reason reason, A misc1, B misc2, C misc3, D misc4)
{
    STUFF_REGISTER_FOR_CRASH(CRASH_DETAIL_REGISTER1, reason);
    STUFF_REGISTER_FOR_CRASH(CRASH_DETAIL_REGISTER1, misc1);
    ...
}
...
Comment 5 Mark Lam 2017-06-29 21:06:31 PDT
Comment on attachment 314214 [details]
Patch

Regarding the use of CRASH_WITH_SECURITY_IMPLICATION, I talked with Keith offline and we agree that we'll:
1. define a DFG_CRASH_WITH_SECURITY_IMPLICATION() if not already defined.  By default, it will be define to CRASH(). 
2. This DFG_ASSERT code will use DFG_CRASH_WITH_SECURITY_IMPLICATION() instead of CRASH() directly.

r=me with fixes.
Comment 6 Keith Miller 2017-06-29 21:48:26 PDT
Comment on attachment 314214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314214&action=review

>> Source/JavaScriptCore/ChangeLog:12
>> +        up to five registers right befor e crashing. The values stuffed
> 
> typo: /befor e/before/.

fixed.

>> Source/WTF/wtf/Assertions.h:472
>> +ALWAYS_INLINE NO_RETURN_DUE_TO_CRASH void WAI_U_CRASH(Reason reason, A misc1, B misc2, C misc3, D misc4)
> 
> I think "WAI_U_CRASH" is a bit too cutesy (and very specific to contemporary American colloquialism).  Can we just call it CRASH_WITH_DETAILS instead?  That seems more clear for people who aren't so hip to the latest cultural jargon (like me).

:( fair enough... I changed it to CRASH_WITH_INFO.

>> Source/WTF/wtf/Assertions.h:479
>> +    CRASH();
> 
> Do something about the need for CRASH_WITH_SECURITY_IMPLICATION() here.

I ended up going with an INLINE_CRASH_WITH_SECURITY_IMPLICATION that defaults to CRASH()

>> Source/WTF/wtf/Assertions.h:534
>> +}
> 
> nit: Instead of duplicating the functions, how about just defining a set of registers like so:
> 
> #if CPU(X86_64) && OS(DARWIN)
> #define CRASH_DETAIL_REGISTER1 "r11"
> #define CRASH_DETAIL_REGISTER2 "r10"
> ...
> #elif CPU(ARM64) && OS(DARWIN)
> #define CRASH_DETAIL_REGISTER1 "x16"
> #define CRASH_DETAIL_REGISTER2 "x17"
> ...
> #endif
> 
> #if OS(DARWIN) && (CPU(X86_64) || CPU(ARM64))
> template<typename Reason, typename A, typename B, typename C, typename D>
> ALWAYS_INLINE NO_RETURN_DUE_TO_CRASH void CRASH_WITH_DETAILS(Reason reason, A misc1, B misc2, C misc3, D misc4)
> {
>     STUFF_REGISTER_FOR_CRASH(CRASH_DETAIL_REGISTER1, reason);
>     STUFF_REGISTER_FOR_CRASH(CRASH_DETAIL_REGISTER1, misc1);
>     ...
> }
> ...

Nice! changed.
Comment 7 Keith Miller 2017-06-29 21:49:19 PDT
Created attachment 314229 [details]
Patch for landing
Comment 8 Keith Miller 2017-06-29 21:56:44 PDT
Created attachment 314230 [details]
Patch for landing
Comment 9 Keith Miller 2017-06-29 21:57:09 PDT
Created attachment 314231 [details]
Patch for landing
Comment 10 Saam Barati 2017-06-29 22:03:10 PDT
Comment on attachment 314229 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=314229&action=review

LGTM too

> Source/JavaScriptCore/dfg/DFGGraph.h:96
> +#define DFG_ASSERT(graph, node, assertion, ...) do {                    \

Why not just have this call logForCrash directly now. handleAssertionFailure seems slightly like a misnomer now since it doesn’t crash

> Source/WTF/wtf/Assertions.h:477
> +#define STUFF_REGISTER_FOR_CRASH(reg, value) __asm__ volatile ("movq %0, %%" reg : : "r" (static_cast<uint64_t>(value)) : reg)

Nit: we might want this to be bitwise_cast in the future so we can log doubles
Comment 11 Keith Miller 2017-06-29 22:09:19 PDT
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314229&action=review
> 
> LGTM too
> 
> > Source/JavaScriptCore/dfg/DFGGraph.h:96
> > +#define DFG_ASSERT(graph, node, assertion, ...) do {                    \
> 
> Why not just have this call logForCrash directly now. handleAssertionFailure
> seems slightly like a misnomer now since it doesn’t crash

I changed it to logAssertionFailure.

> 
> > Source/WTF/wtf/Assertions.h:477
> > +#define STUFF_REGISTER_FOR_CRASH(reg, value) __asm__ volatile ("movq %0, %%" reg : : "r" (static_cast<uint64_t>(value)) : reg)
> 
> Nit: we might want this to be bitwise_cast in the future so we can log
> doubles

bitwise_cast asserts you have the right size. I think it's easier to bitwise_cast your doubles/floats since those will be less common. I suppose I could add a static assert that the type of the value is integral so people don't forget to cast.
Comment 12 Saam Barati 2017-06-29 22:32:30 PDT
(In reply to Keith Miller from comment #11)
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=314229&action=review
> > 
> > LGTM too
> > 
> > > Source/JavaScriptCore/dfg/DFGGraph.h:96
> > > +#define DFG_ASSERT(graph, node, assertion, ...) do {                    \
> > 
> > Why not just have this call logForCrash directly now. handleAssertionFailure
> > seems slightly like a misnomer now since it doesn’t crash
> 
> I changed it to logAssertionFailure.
> 
> > 
> > > Source/WTF/wtf/Assertions.h:477
> > > +#define STUFF_REGISTER_FOR_CRASH(reg, value) __asm__ volatile ("movq %0, %%" reg : : "r" (static_cast<uint64_t>(value)) : reg)
> > 
> > Nit: we might want this to be bitwise_cast in the future so we can log
> > doubles
> 
> bitwise_cast asserts you have the right size. I think it's easier to
> bitwise_cast your doubles/floats since those will be less common. I suppose
> I could add a static assert that the type of the value is integral so people
> don't forget to cast.

I think we can probably do this in a later patch as needed.
Comment 13 Keith Miller 2017-06-29 22:33:46 PDT
Created attachment 314236 [details]
Patch for landing
Comment 14 Keith Miller 2017-06-29 22:43:12 PDT
Created attachment 314237 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2017-06-30 01:23:21 PDT
Comment on attachment 314237 [details]
Patch for landing

Clearing flags on attachment: 314237

Committed r218992: <http://trac.webkit.org/changeset/218992>
Comment 16 WebKit Commit Bot 2017-06-30 01:23:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Oliver Hunt 2017-06-30 08:42:35 PDT
Comment on attachment 314237 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=314237&action=review

> Source/WTF/wtf/Assertions.h:557
> +ALWAYS_INLINE NO_RETURN_DUE_TO_CRASH void CRASH_WITH_INFO(Types...)

webkit style says this should not be a pile of uppercase, even if it is replacing what used to be a macro (which i don't believe it is?)
Comment 18 JF Bastien 2017-06-30 08:43:25 PDT
Comment on attachment 314237 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=314237&action=review

This is super cool! :D

> Source/WTF/wtf/Assertions.h:481
> +// This ordering was chosen to be consistant with JSC's JIT asserts. We probably shouldn't change this ordering

"consistent"
Comment 19 Filip Pizlo 2017-06-30 09:22:47 PDT
Comment on attachment 314237 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=314237&action=review

> Source/WTF/wtf/Assertions.h:471
> +#ifndef INLINE_CRASH_WITH_SECURITY_IMPLICATION
> +// This is useful if you are going to stuff data into registers before crashing. Like the CRASH_WITH_INFO functions below...
> +#define INLINE_CRASH_WITH_SECURITY_IMPLICATION() CRASH()
> +#endif

Really don't like this change.  This likely means that clang will rage-coalesce all crashes to the same origin.  This means that we will get crash logs that claim for example that we crashed at vector overflow, even though it's a DFG_CRASH.

Can we revert this change to use a crash function call instead, and pass line/file info to that function to prevent rage-coalescing?
Comment 21 Matt Lewis 2017-06-30 11:38:04 PDT
Reverted r218992 for reason:

The patch broke the iOS device builds.

Committed r219006: <http://trac.webkit.org/changeset/219006>
Comment 22 Radar WebKit Bug Importer 2017-07-03 13:41:08 PDT
<rdar://problem/33112191>