Bug 174005 – DFG_ASSERT should allow stuffing registers before trapping.

• Status: REOPENED
• Component: New Bugs

[Keith Miller]

DFG_ASSERT should allow stuffing registers before trapping.

[Keith Miller]

Created attachment 314207 [details]
Patch

[Keith Miller]

Created attachment 314211 [details]
Patch

[Keith Miller]

Created attachment 314214 [details]
Patch

[Mark Lam]

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

[Mark Lam]

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.

[Keith Miller]

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.

[Keith Miller]

Created attachment 314229 [details]
Patch for landing

[Keith Miller]

Created attachment 314230 [details]
Patch for landing

[Keith Miller]

Created attachment 314231 [details]
Patch for landing

[Saam Barati]

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

[Keith Miller]

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

[Saam Barati]

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

[Keith Miller]

Created attachment 314236 [details]
Patch for landing

[Keith Miller]

Created attachment 314237 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 314237 [details]
Patch for landing

Clearing flags on attachment: 314237

Committed r218992: <http://trac.webkit.org/changeset/218992>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Oliver Hunt]

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

[JF Bastien]

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"

[Filip Pizlo]

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?

[Matt Lewis]

The revision is causing a break in all iOS Device Builds. 
https://build.webkit.org/builders/Apple%20iOS%2010%20Release%20%28Build%29/builds/2524

https://build.webkit.org/builders/Apple%20iOS%2010%20Release%20%28Build%29/builds/2524/steps/compile-webkit/logs/stdio

[Matt Lewis]

Reverted r218992 for reason:

The patch broke the iOS device builds.

Committed r219006: <http://trac.webkit.org/changeset/219006>

[Radar WebKit Bug Importer]

<rdar://problem/33112191>