WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
59830
Add WTFPrintf()
https://bugs.webkit.org/show_bug.cgi?id=59830
Summary
Add WTFPrintf()
Jeff Miller
Reported
2011-04-29 14:10:16 PDT
Add WTFPrintf()
Attachments
Add WTFPrintf()
(3.12 KB, patch)
2011-04-29 14:14 PDT
,
Jeff Miller
no flags
Details
Formatted Diff
Diff
Update patch to include PRINTF macro, based on feedback from Dan.
(3.61 KB, patch)
2011-04-29 15:45 PDT
,
Jeff Miller
ap
: review-
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Jeff Miller
Comment 1
2011-04-29 14:14:38 PDT
Created
attachment 91737
[details]
Add WTFPrintf()
Jeff Miller
Comment 2
2011-04-29 15:45:12 PDT
Created
attachment 91756
[details]
Update patch to include PRINTF macro, based on feedback from Dan.
Alexey Proskuryakov
Comment 3
2011-04-29 17:42:57 PDT
How is this better than a plain fprintf(stderr, ...)?
Jeff Miller
Comment 4
2011-04-29 17:54:14 PDT
(In reply to
comment #3
)
> How is this better than a plain fprintf(stderr, ...)?
That won't work on all platforms, which is why we have WTFLog() in the first place. See vprintf_stderr_common() in Assertions.cpp, which is where everything bottlenecks through.
Alexey Proskuryakov
Comment 5
2011-04-29 18:07:44 PDT
Thank you, I now see what the technical difference is. What is the use case for WTFPrint? When do we want to print from the current process to error log unconditionally, and without passing the message to developer console? I don't think that features like this can be added without at least one use case documented in the bug.
Jeff Miller
Comment 6
2011-04-29 18:14:01 PDT
(In reply to
comment #5
)
> Thank you, I now see what the technical difference is. > > What is the use case for WTFPrint? When do we want to print from the current process to error log unconditionally, and without passing the message to developer console? I don't think that features like this can be added without at least one use case documented in the bug.
The use case is the classic "printf debugging" idiom, where you're trying to debug something that's difficult to reproduce by stepping in the debugger because of timing issues, for example. Today, I have to resort to doing something like: WTFLogChannel LogAlways = { WTFLogChannelOn, 0, 0}; LOG(Always, "this is a test"); which is a pain. I don't forsee any instances where you would actually check in code that uses PRINTF().
Alexey Proskuryakov
Comment 7
2011-04-29 20:03:37 PDT
Thanks Jeff, that makes sense. Since I'm usually debugging on Mac, plain printf works great for me, but I can see how this can be difficult on Windows, even with /console switch. And LOG_ERROR output is probably not very suitable due to its ERROR: prefix.
> I don't forsee any instances where you would actually check in code that uses PRINTF().
I'm wondering if permanently adding an Always channel would communicate this better. The reason I'm asking is that the choice of macros in Assertions.h is already fairly confusing (do I use LOG or LOG_ASSERT?..) Not sure if the idea with LOG(Always) is better than PRINTF tbhough
Jeff Miller
Comment 8
2011-04-30 14:15:09 PDT
(In reply to
comment #7
)
> Thanks Jeff, that makes sense. > > Since I'm usually debugging on Mac, plain printf works great for me, but I can see how this can be difficult on Windows, even with /console switch. And LOG_ERROR output is probably not very suitable due to its ERROR: prefix. > > > I don't forsee any instances where you would actually check in code that uses PRINTF(). > > I'm wondering if permanently adding an Always channel would communicate this better. The reason I'm asking is that the choice of macros in Assertions.h is already fairly confusing (do I use LOG or LOG_ASSERT?..) Not sure if the idea with LOG(Always) is better than PRINTF tbhough
Personally, I vote for PRINTF(), it's straightforward and most people understand what it means.
Alexey Proskuryakov
Comment 9
2011-04-30 15:33:10 PDT
Comment on
attachment 91756
[details]
Update patch to include PRINTF macro, based on feedback from Dan. View in context:
https://bugs.webkit.org/attachment.cgi?id=91756&action=review
OK, I'm fine with adding PRINTF. I think that I have too many comments to justify another quick review round though.
> Source/JavaScriptCore/wtf/Assertions.cpp:299 > + if (format[strlen(format) - 1] != '\n')
What if format is zero length?
> Source/JavaScriptCore/wtf/Assertions.h:363 > +#define PRINTF() ((void)0)
Perhaps #define PRINTF printf ?
> Source/JavaScriptCore/wtf/Assertions.h:365 > +#define PRINTF(arg...) ((void)0)
Ditto.
> Source/JavaScriptCore/wtf/Assertions.h:367 > +#elif LOG_DISABLED > +#define PRINTF(...) ((void)0)
I don't think that LOG_DISABLED should affect PRINTF. In fact, printf debugging is very useful in release builds, too.
Jeff Miller
Comment 10
2011-04-30 15:54:44 PDT
(In reply to
comment #9
)
> (From update of
attachment 91756
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=91756&action=review
> > OK, I'm fine with adding PRINTF. I think that I have too many comments to justify another quick review round though. > > > Source/JavaScriptCore/wtf/Assertions.cpp:299 > > + if (format[strlen(format) - 1] != '\n') > > What if format is zero length?
I'll fix this bug that was in the existing code.
> > > Source/JavaScriptCore/wtf/Assertions.h:363 > > +#define PRINTF() ((void)0) > > Perhaps #define PRINTF printf ?
See my comments below.
> > > Source/JavaScriptCore/wtf/Assertions.h:365 > > +#define PRINTF(arg...) ((void)0) > > Ditto. > > > Source/JavaScriptCore/wtf/Assertions.h:367 > > +#elif LOG_DISABLED > > +#define PRINTF(...) ((void)0) > > I don't think that LOG_DISABLED should affect PRINTF. In fact, printf debugging is very useful in release builds, too.
Dan was adamant that it should be turned off when logging is disabled. I originally had it enabled all the time. I don't have a strong preference either way, if you feel strongly you should try to convince Dan.
Alexey Proskuryakov
Comment 11
2011-04-30 15:57:44 PDT
We should try to convince Dan :-)
Jeff Miller
Comment 12
2011-05-02 10:46:57 PDT
(In reply to
comment #9
)
> (From update of
attachment 91756
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=91756&action=review
> > > Source/JavaScriptCore/wtf/Assertions.h:363 > > +#define PRINTF() ((void)0) > > Perhaps #define PRINTF printf ? > > > Source/JavaScriptCore/wtf/Assertions.h:365 > > +#define PRINTF(arg...) ((void)0) > > Ditto. >
I'd be hesitant to do this, just because we don't implement LOG() and the other macros on these platforms at all. Seems like it would be better to match the existing idiom.
mitz
Comment 13
2011-05-02 10:50:27 PDT
(In reply to
comment #9
)
> I don't think that LOG_DISABLED should affect PRINTF. In fact, printf debugging is very useful in release builds, too.
It’s generally bad form for production code, especially a framework with arbitrary clients, to do any writing to stdout/stderr. Who is supposed to read this?
Jeff Miller
Comment 14
2011-05-02 10:58:49 PDT
(In reply to
comment #13
)
> (In reply to
comment #9
) > > I don't think that LOG_DISABLED should affect PRINTF. In fact, printf debugging is very useful in release builds, too. > > It’s generally bad form for production code, especially a framework with arbitrary clients, to do any writing to stdout/stderr. Who is supposed to read this?
I would assume that the engineer writing the code will want to read this when reproducing a bug that doesn't happen in debug code, for example. Of course, you could also set LOG_DISABLED to 0 temporarily in production in this case, or just call WTFPrintf() directly, since it (as well as the other logging routines) are always implemented, regardless of whether LOG_DISABLED is set or not.
mitz
Comment 15
2011-05-02 11:04:03 PDT
(In reply to
comment #14
)
> (In reply to
comment #13
) > > (In reply to
comment #9
) > > > I don't think that LOG_DISABLED should affect PRINTF. In fact, printf debugging is very useful in release builds, too. > > > > It’s generally bad form for production code, especially a framework with arbitrary clients, to do any writing to stdout/stderr. Who is supposed to read this? > > I would assume that the engineer writing the code will want to read this when reproducing a bug that doesn't happen in debug code, for example. Of course, you could also set LOG_DISABLED to 0 temporarily in production in this case, or just call WTFPrintf() directly, since it (as well as the other logging routines) are always implemented, regardless of whether LOG_DISABLED is set or not.
That would be one way of doing it. I mostly want to minimize the risk of spurious stderr output making it into production builds. Perhaps making the distinction Debug/Release vs. Production instead of Debug vs. Production/Release is the right way to do this, but traditionally I think we’ve kept Release as similar as possible to Production.
Jeff Miller
Comment 16
2011-05-02 11:14:10 PDT
(In reply to
comment #15
)
> (In reply to
comment #14
) > > (In reply to
comment #13
) > > > (In reply to
comment #9
) > > > > I don't think that LOG_DISABLED should affect PRINTF. In fact, printf debugging is very useful in release builds, too. > > > > > > It’s generally bad form for production code, especially a framework with arbitrary clients, to do any writing to stdout/stderr. Who is supposed to read this? > > > > I would assume that the engineer writing the code will want to read this when reproducing a bug that doesn't happen in debug code, for example. Of course, you could also set LOG_DISABLED to 0 temporarily in production in this case, or just call WTFPrintf() directly, since it (as well as the other logging routines) are always implemented, regardless of whether LOG_DISABLED is set or not. > > That would be one way of doing it. I mostly want to minimize the risk of spurious stderr output making it into production builds. Perhaps making the distinction Debug/Release vs. Production instead of Debug vs. Production/Release is the right way to do this, but traditionally I think we’ve kept Release as similar as possible to Production.
Yeah, I agree with minimizing risk. I personally think it's OK to turn off PRINTF() when LOG_DISABLE is on, since there are straight-forward ways around this if you really need to do a PRINTF() in a Production/Release build (which should be rare anyway).
Jeff Miller
Comment 17
2011-05-02 11:22:34 PDT
(In reply to
comment #10
)
> (In reply to
comment #9
) > > (From update of
attachment 91756
[details]
[details]) > > View in context:
https://bugs.webkit.org/attachment.cgi?id=91756&action=review
> > > > > Source/JavaScriptCore/wtf/Assertions.cpp:299 > > > + if (format[strlen(format) - 1] != '\n') > > > > What if format is zero length? >
I wrote up a separate bug (
https://bugs.webkit.org/show_bug.cgi?id=59949
) to fix the existing code in WTFLog() and WTFLogVerbose().
Alexey Proskuryakov
Comment 18
2011-05-02 11:23:29 PDT
Rebuilding with a new LOG_DISABLED value would be huge and unnecessary pain. And again, having a LOG-related setting control PRINTF would be hugely confusing in my opinion. I don't think there is anything special to protect from here. Poor review can let code with printf get landed, and it's the same with PRINTF.
Alexey Proskuryakov
Comment 19
2011-05-02 11:26:00 PDT
Is there a way to resolve the practical problem by making printf() work as required for debugging instead? I think that there must be such a way.
Jeff Miller
Comment 20
2011-05-02 11:31:55 PDT
I think it's best to just table this for now, since there's a lot of disagreement about this.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug