Bug 59830 – Add WTFPrintf()

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Web Template Framework

[Jeff Miller]

Add WTFPrintf()

[Jeff Miller]

Created attachment 91737 [details]
Add WTFPrintf()

[Jeff Miller]

Created attachment 91756 [details]
Update patch to include PRINTF macro, based on feedback from Dan.

[Alexey Proskuryakov]

How is this better than a plain fprintf(stderr, ...)?

[Jeff Miller]

(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]

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]

(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]

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]

(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 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]

(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]

We should try to convince Dan :-)

[Jeff Miller]

(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.

(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]

(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.

(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]

(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]

(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]

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]

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]

I think it's best to just table this for now, since there's a lot of disagreement about this.