Bug 237572 - [libpas] Report more actionable crash in pas_enumerator
Summary: [libpas] Report more actionable crash in pas_enumerator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-07 18:21 PST by Yusuke Suzuki
Modified: 2022-03-11 16:00 PST (History)
3 users (show)

See Also:


Attachments
Patch (34.94 KB, patch)
2022-03-07 18:34 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2022-03-07 18:21:06 PST
[libpas] Report more actionable crash in pas_enumerator
Comment 1 Yusuke Suzuki 2022-03-07 18:34:50 PST
Created attachment 454060 [details]
Patch
Comment 2 Mark Lam 2022-03-07 18:52:20 PST
Comment on attachment 454060 [details]
Patch

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

> Source/bmalloc/libpas/src/libpas/pas_utils.c:60
> +    pas_panic("%s:%d: %s: assertion %s failed.\n", filename, line, function, expression);

It looks like pas_panic will trample on the incoming args:
```
void pas_panic(const char* format, ...)
{
    static const bool fast_panic = false;
    if (!fast_panic) {
       ... 
        pas_vlog(format, arg_list);
    }
    __builtin_trap();
}
```

Can you also change pas_panic to not call __builtin_trap() directly, but call a NEVER_INLINE pas_trap(const char* filename, int line, const char* function, const char* expression) (similar to WTFCrashWithInfoImpl()).  This way we can at least see the line number in crash logs as well.  I think this will be useful.
Comment 3 Mark Lam 2022-03-07 20:54:05 PST
Comment on attachment 454060 [details]
Patch

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

>> Source/bmalloc/libpas/src/libpas/pas_utils.c:60
>> +    pas_panic("%s:%d: %s: assertion %s failed.\n", filename, line, function, expression);
> 
> It looks like pas_panic will trample on the incoming args:
> ```
> void pas_panic(const char* format, ...)
> {
>     static const bool fast_panic = false;
>     if (!fast_panic) {
>        ... 
>         pas_vlog(format, arg_list);
>     }
>     __builtin_trap();
> }
> ```
> 
> Can you also change pas_panic to not call __builtin_trap() directly, but call a NEVER_INLINE pas_trap(const char* filename, int line, const char* function, const char* expression) (similar to WTFCrashWithInfoImpl()).  This way we can at least see the line number in crash logs as well.  I think this will be useful.

Never mind.  Yusuke reminded me offline that the caller of pas_assertion_failed_no_inline will be on the stack trace in the crash log.  So, the needed information is captured after all.
Comment 4 EWS 2022-03-08 12:49:42 PST
Committed r291007 (248183@main): <https://commits.webkit.org/248183@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454060 [details].
Comment 5 Radar WebKit Bug Importer 2022-03-08 12:50:18 PST
<rdar://problem/89984164>
Comment 6 Yusuke Suzuki 2022-03-11 16:00:22 PST
Committed r291199 (248354@trunk): <https://commits.webkit.org/248354@trunk>