Bug 75605

Summary: WTF logging functions should call vprintf_stderr_common only once per line
Product: WebKit Reporter: Mark Rowe (bdash) <mrowe>
Component: Web Template FrameworkAssignee: Mark Rowe (bdash) <mrowe>
Status: RESOLVED FIXED    
Severity: Normal CC: ossy, webkit.review.bot, zherczeg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 75606    
Attachments:
Description Flags
Patch v1 mitz: review+, webkit-ews: commit-queue-

Description Mark Rowe (bdash) 2012-01-04 23:59:34 PST
Currently several of the WTF logging functions make multiple calls to vprintf_stderr_common to output a single line of text. This results in strangely formatted output if vprintf_stderr_common is retargeted to an output device such as syslog that is message-oriented rather than stream-oriented.
Comment 1 Mark Rowe (bdash) 2012-01-05 00:10:23 PST
Created attachment 121227 [details]
Patch v1
Comment 2 WebKit Review Bot 2012-01-05 00:12:45 PST
Attachment 121227 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/Assertions.cpp:130:  vprintf_stderr_with_prefix is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/Assertions.cpp:139:  vprintf_stderr_with_trailing_line is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 mitz 2012-01-05 00:21:42 PST
Comment on attachment 121227 [details]
Patch v1

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

> Source/JavaScriptCore/wtf/Assertions.cpp:134
> +    strcat(formatWithPrefix.get(), format);

This is effectively strlen()ing prefix again. You could have done this a little more efficiently.
Comment 4 Early Warning System Bot 2012-01-05 00:23:06 PST
Comment on attachment 121227 [details]
Patch v1

Attachment 121227 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11128164
Comment 5 Mark Rowe (bdash) 2012-01-05 01:16:56 PST
Landed in r104124.
Comment 6 Csaba Osztrogonác 2012-01-05 02:40:20 PST
Comment on attachment 121227 [details]
Patch v1

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

> Source/JavaScriptCore/wtf/Assertions.cpp:128
> +#if COMPILER(GCC)
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wformat-nonliteral"
> +#endif

This change is absolutely incorrect, because push introduced in gcc 4.6.
We can't expect that everyone use this gcc.

We should use this pragma inside a similar guard:
#if defined(__GNUC__) && (__GNUC__ * 100 + __GNUC_MINOR__ >= 406)
...
#endif
Comment 7 Csaba Osztrogonác 2012-01-05 02:42:02 PST
GCC_VERSION_AT_LEAST(4, 6, 0) is better.
Comment 8 Mark Rowe (bdash) 2012-01-05 02:42:47 PST
That would probably prevent it from being picked up by clang.
Comment 9 Mark Rowe (bdash) 2012-01-05 02:47:21 PST
I'd guess that something like the following would do the trick:

#if COMPILER(CLANG) || (COMPILER(GCC) && GCC_VERSION_AT_LEAST(4, 6, 0))

I have no way to test it with GCC though.
Comment 10 Zoltan Herczeg 2012-01-05 03:08:26 PST
(In reply to comment #9)
> I'd guess that something like the following would do the trick:
> 
> #if COMPILER(CLANG) || (COMPILER(GCC) && GCC_VERSION_AT_LEAST(4, 6, 0))
> 
> I have no way to test it with GCC though.

I have already landed http://trac.webkit.org/changeset/104134. Shall I add the CLANG test?
Comment 11 Mark Rowe (bdash) 2012-01-05 03:17:49 PST
Given that your change broke all of the Mac builds, please do.
Comment 12 Zoltan Herczeg 2012-01-05 04:19:55 PST
(In reply to comment #11)
> Given that your change broke all of the Mac builds, please do.

Sorry for that. Buildfix landed.