Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add variadic PAS_ASSERT support. #1537

Conversation

MenloDorian
Copy link

@MenloDorian MenloDorian commented Jun 15, 2022

b9a2b29

Add variadic PAS_ASSERT support.
https://bugs.webkit.org/show_bug.cgi?id=241626
rdar://95204263

Reviewed by Keith Miller.

This patch adds support for a variadic PAS_ASSERT, and uses it to gather
more debugging info for asserts in pas_local_view_cache_stop and
pas_segregated_heap_ensure_allocator_index.

From manual local disassembly of pas_local_view_cache_stop, I verified that
this patch only changes assertion fail reporting code in an out of line slow
path.  The change does perturb code size, and any changes in code may also
cause clang to arbitrarily rearrange the order of some control flow diamonds
(e.g. the cases in a switch statement).

If no extra data is provided to the variadic PAS_ASSERT, it will emit exactly
same code as it does previously when PAS_ASSERT was not variadic.  This gives
us confidence that any perturbations in code will only manifest in PAS_ASSERTs
that we choose to add extra info for reporting assertion failures.

Preliminary benchmark results appear to show that performance is neutral.

* Source/bmalloc/libpas/src/libpas/pas_local_view_cache.c:
(pas_local_view_cache_stop):
* Source/bmalloc/libpas/src/libpas/pas_segregated_heap.c:
(pas_segregated_heap_ensure_allocator_index):
* Source/bmalloc/libpas/src/libpas/pas_utils.c:
(pas_crash_with_info_impl1):
(pas_crash_with_info_impl2):
(pas_crash_with_info_impl3):
(pas_crash_with_info_impl4):
(pas_crash_with_info_impl5):
(pas_crash_with_info_impl6):
(pas_report_assertion_failed):
* Source/bmalloc/libpas/src/libpas/pas_utils.h:
(pas_assertion_failed):
(pas_assertion_failed_noreturn_silencer1):
(pas_assertion_failed_noreturn_silencer2):
(pas_assertion_failed_noreturn_silencer3):
(pas_assertion_failed_noreturn_silencer4):
(pas_assertion_failed_noreturn_silencer5):
(pas_assertion_failed_noreturn_silencer6):

Canonical link: https://commits.webkit.org/251567@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295562 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@MenloDorian MenloDorian self-assigned this Jun 15, 2022
Copy link
Contributor

@kmiller68 kmiller68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with a style comment

@@ -93,6 +93,11 @@ PAS_BEGIN_EXTERN_C;

#define PAS_RISCV __PAS_RISCV

#define PAS_PP_THIRD_ARG(a,b,c,...) c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think webKit style would put a space after each ,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Fixed.

Comment on lines 249 to 255
/* FIXME: Consider whether it makes sense to capture the filename, function, and expression
in crash data or not. Need to measure if there's a performance impact.
FIXME: Also consider converting PAS_ASSERT_WITH_DETAIL and PAS_ASSERT_WITH_EXTRA_DETAIL
to just use the variadic PAS_ASSERT. We currently leave these and the PAS_ASSERT with
no extra args unchanged to make sure we don't perturb performance (until we can measure
and confirm that using the variadic form won't impact performance).
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth when we did this in JSC for our RELEASE_ASSERTS we didn't see a perf impact. Probably due to the crashing side being considered UNLIKELY so the compiler puts that info off in oblivion. The only thing is that some crashes may not be merged but we never saw a perf impact if that happened.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. It's just that we've been told that libpas appears to be extra sensitive to this for reasons unclear to us. So, we should proceed with caution and let perf measurements do the talking.

@MenloDorian MenloDorian force-pushed the eng/variadic-PAS_ASSERT-support branch from 29fd9b8 to 042ca3c Compare June 15, 2022 16:35
@MenloDorian MenloDorian added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 15, 2022
@webkit-early-warning-system webkit-early-warning-system merged commit b9a2b29 into WebKit:main Jun 15, 2022
@webkit-early-warning-system
Copy link
Collaborator

Committed r295562 (251567@main): https://commits.webkit.org/251567@main

Reviewed commits have been landed. Closing PR #1537 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 15, 2022
@MenloDorian MenloDorian deleted the eng/variadic-PAS_ASSERT-support branch June 15, 2022 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bmalloc For bugs in bmalloc
Projects
None yet
3 participants