Bug 194920 - Fix WTFLogVerbose variadic parameters forwarding
Summary: Fix WTFLogVerbose variadic parameters forwarding
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xabier Rodríguez Calvar
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-21 14:17 PST by Xabier Rodríguez Calvar
Modified: 2019-03-04 04:21 PST (History)
10 users (show)

See Also:


Attachments
Patch (2.97 KB, patch)
2019-02-21 14:25 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-highsierra (2.41 MB, application/zip)
2019-02-21 17:32 PST, Build Bot
no flags Details
Patch for landing (2.95 KB, patch)
2019-02-25 22:06 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xabier Rodríguez Calvar 2019-02-21 14:17:20 PST
Fix WTFLogVerbose variadic parameters forwarding
Comment 1 Xabier Rodríguez Calvar 2019-02-21 14:25:46 PST
Created attachment 362647 [details]
Patch
Comment 2 Build Bot 2019-02-21 17:32:31 PST
Comment on attachment 362647 [details]
Patch

Attachment 362647 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11238881

New failing tests:
storage/indexeddb/intversion-open-in-upgradeneeded.html
storage/indexeddb/intversion-open-in-upgradeneeded-private.html
storage/indexeddb/modern/blocked-open-db-requests-private.html
storage/indexeddb/odd-strings-private.html
Comment 3 Build Bot 2019-02-21 17:32:33 PST
Created attachment 362675 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 4 Alex Christensen 2019-02-22 10:28:21 PST
Comment on attachment 362647 [details]
Patch

r=me
I'm curious about what configuration led you to need to fix this bug.
Comment 5 WebKit Commit Bot 2019-02-24 23:38:08 PST
Comment on attachment 362647 [details]
Patch

Rejecting attachment 362647 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 362647, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=362647&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=194920&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 362647 from bug 194920.
Fetching: https://bugs.webkit.org/attachment.cgi?id=362647
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Alex Christensen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 2 diffs from patch file(s).
patching file Source/WTF/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WTF/wtf/Assertions.cpp
Hunk #1 FAILED at 380.
Hunk #2 succeeded at 454 (offset 50 lines).
Hunk #3 succeeded at 473 (offset 50 lines).
1 out of 3 hunks FAILED -- saving rejects to file Source/WTF/wtf/Assertions.cpp.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Alex Christensen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/11274586
Comment 6 Xabier Rodríguez Calvar 2019-02-25 07:13:39 PST
(In reply to Alex Christensen from comment #4)
> Comment on attachment 362647 [details]
> Patch
> 
> r=me
> I'm curious about what configuration led you to need to fix this bug.

Nothing too fancy, just:

@@ -458,7 +458,7 @@ WTF_EXPORT_PRIVATE NO_RETURN_DUE_TO_CRASH void WTFCrashWithSecurityImplication(v
 #if LOG_DISABLED
 #define LOG(channel, ...) ((void)0)
 #else
-#define LOG(channel, ...) WTFLog(&LOG_CHANNEL(channel), __VA_ARGS__)
+#define LOG(channel, ...) WTFLogVerbose(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, &LOG_CHANNEL(channel), __VA_ARGS__)
 #endif
 
 /* LOG_VERBOSE */

For what I'm debugging, it is easier for me to know where the prints are done. I thought of adding a compile switch to force this. Do you think it could be interesting?

PS: Thx for the review, will land tomorrow morning CET.
Comment 7 Alex Christensen 2019-02-25 11:31:51 PST
I don't think a compile switch to turn on all the logs would be good.  There are ways to enable and disable each logging channel at runtime, which can accomplish the same thing.
Comment 8 Xabier Rodríguez Calvar 2019-02-25 22:04:16 PST
(In reply to Alex Christensen from comment #7)
> I don't think a compile switch to turn on all the logs would be good.  There
> are ways to enable and disable each logging channel at runtime, which can
> accomplish the same thing.

No, I'm talking about a compile switch to do what I do in the diff I added here
Comment 9 Xabier Rodríguez Calvar 2019-02-25 22:06:56 PST
Created attachment 362962 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2019-02-25 22:44:12 PST
Comment on attachment 362962 [details]
Patch for landing

Clearing flags on attachment: 362962

Committed r242075: <https://trac.webkit.org/changeset/242075>
Comment 11 WebKit Commit Bot 2019-02-25 22:44:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-02-25 22:45:26 PST
<rdar://problem/48391464>
Comment 13 Truitt Savell 2019-03-01 16:27:42 PST
The changes in https://trac.webkit.org/changeset/242075/webkit in conjunction with the rollout in https://trac.webkit.org/changeset/242189/webkit has caused several storage/indexeddb/ tests to crash on Mac Debug WK1 and are slowing down EWS. Marking the tests as skip causes other tests to crash.  

The details of this were tracked in 
https://bugs.webkit.org/show_bug.cgi?id=195210 and https://bugs.webkit.org/show_bug.cgi?id=195141

Can you update your change to reflect the rollout? otherwise we will need to roll r242075 out as well to fix testing.
Comment 14 Xabier Rodríguez Calvar 2019-03-04 00:44:28 PST
(In reply to Truitt Savell from comment #13)
> Can you update your change to reflect the rollout? otherwise we will need to
> roll r242075 out as well to fix testing.

On it.
Comment 15 Xabier Rodríguez Calvar 2019-03-04 00:47:13 PST
Anyway, I don't follow why I need to fix this myself or my patch will be rolled out. The original implementation is wrong and it should be fixed as well. We can't just roll out a patch and leave things wrong, right?
Comment 16 Xabier Rodríguez Calvar 2019-03-04 04:21:11 PST
(In reply to Truitt Savell from comment #13)
> Can you update your change to reflect the rollout? otherwise we will need to
> roll r242075 out as well to fix testing.

As I commented on bug 195210, it looks like bots are green now thanks to a fix by Darin, who landed r242308.