Bug 155992 - Implement logging for RELEASE_BASSERT_WITH_MESSAGE() in BAssert.h
Summary: Implement logging for RELEASE_BASSERT_WITH_MESSAGE() in BAssert.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on: 155409
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-29 16:06 PDT by David Kilzer (:ddkilzer)
Modified: 2016-06-12 06:08 PDT (History)
4 users (show)

See Also:


Attachments
Patch v1 (6.18 KB, patch)
2016-03-31 03:40 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (6.29 KB, patch)
2016-04-03 11:00 PDT, David Kilzer (:ddkilzer)
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2016-03-29 16:06:00 PDT
For Bug 155409, I added RELEASE_BASSERT_WITH_MESSAGE() in BAssert.h, but it doesn't actually log anything.

This bug is to implement logging for RELEASE_BASSERT_WITH_MESSAGE() in BAssert.h.
Comment 1 David Kilzer (:ddkilzer) 2016-03-31 03:40:25 PDT
Created attachment 275273 [details]
Patch v1
Comment 2 WebKit Commit Bot 2016-03-31 03:41:12 PDT
Attachment 275273 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/BAssert.h:79:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Geoffrey Garen 2016-03-31 09:37:04 PDT
Comment on attachment 275273 [details]
Patch v1

r=me

I guess this is OK.

Do we need this? It looks like we'll only use it for soft linking failures. Are we suffering from soft linking failures we need to diagnose?

A problem with heavy and complex ASSERT macros is that they destroy the CPU context at the point of assertion failure, so they make crash reports much less useful.

So, I guess I can say OK to RELEASE_BASSERT_WITH_MESSAGE for soft linking because it's only a small part of the project, but this is an idiom I'd like to avoid using very much.
Comment 4 David Kilzer (:ddkilzer) 2016-04-03 11:00:28 PDT
Created attachment 275505 [details]
Patch v2
Comment 5 David Kilzer (:ddkilzer) 2016-04-03 11:01:33 PDT
(In reply to comment #3)
> Comment on attachment 275273 [details]
> Patch v1
> 
> r=me
> 
> I guess this is OK.
> 
> Do we need this? It looks like we'll only use it for soft linking failures.
> Are we suffering from soft linking failures we need to diagnose?
> 
> A problem with heavy and complex ASSERT macros is that they destroy the CPU
> context at the point of assertion failure, so they make crash reports much
> less useful.
> 
> So, I guess I can say OK to RELEASE_BASSERT_WITH_MESSAGE for soft linking
> because it's only a small part of the project, but this is an idiom I'd like
> to avoid using very much.

Patch v2 only does the extra logging in Debug builds, so Release builds won't destroy CPU context.
Comment 6 WebKit Commit Bot 2016-04-03 11:01:37 PDT
Attachment 275505 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/BAssert.h:82:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Geoffrey Garen 2016-06-09 17:20:26 PDT
Comment on attachment 275505 [details]
Patch v2

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

r=me

> Source/bmalloc/bmalloc/BAssert.h:72
> +#define BMALLOC_LOGGING_PREFIX "#BM: "

Might as well throw in the full "bmalloc:" here if we can.
Comment 8 David Kilzer (:ddkilzer) 2016-06-10 13:45:37 PDT
Comment on attachment 275505 [details]
Patch v2

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

>> Source/bmalloc/bmalloc/BAssert.h:72
>> +#define BMALLOC_LOGGING_PREFIX "#BM: "
> 
> Might as well throw in the full "bmalloc:" here if we can.

I'll change this.  I was mimicking what was done in Source/WTF/wtf/Assertions.h:

#define WTF_LOGGING_PREFIX "#WK: "
Comment 9 David Kilzer (:ddkilzer) 2016-06-11 04:45:31 PDT
Committed r201969: <http://trac.webkit.org/changeset/201969>