Bug 90823 – Ease printf debugging in WebKit.

• Status: ASSIGNED
• Component: Web Template Framework

[Alexis Menard (darktears)]

Ease printf debugging in WebKit.

[Alexis Menard (darktears)]

Created attachment 151321 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #1)
> Created an attachment (id=151321) [details]
> Patch

Some work in progress/idea.

Obviously need to be finished.

[Antonio Gomes]

see discussion here: https://bugs.webkit.org/show_bug.cgi?id=79176

Comment on attachment 151321 [details]
Patch

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

> Source/WTF/wtf/DebugHelpers.h:32
> +#include "windows.h"

<windows.h>?

> Source/WTF/wtf/DebugHelpers.h:61
> +            OutputDebugString((wchar_t*)output.utf8().data());

I think it is better to use OutputDebugStringW(wchar_t* utf16ButNoSurrogate) or converting code page CP_ACP with OutputDebugStringA(char* mbcsStringButNotUTF8)

[Alexis Menard (darktears)]

Comment on attachment 151321 [details]
Patch

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

>> Source/WTF/wtf/DebugHelpers.h:32
>> +#include "windows.h"
> 
> <windows.h>?

Well for OutputDebugString. I don't have a Windows machine to test so I would rely on someone else to guide me.

[Alexis Menard (darktears)]

(In reply to comment #5)
> (From update of attachment 151321 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151321&action=review
> 
> >> Source/WTF/wtf/DebugHelpers.h:32
> >> +#include "windows.h"
> > 
> > <windows.h>?
> 
> Well for OutputDebugString. I don't have a Windows machine to test so I would rely on someone else to guide me.

Ah I just saw, yes of course with <> it's better :)

[Alexis Menard (darktears)]

Created attachment 151461 [details]
Patch

[Build Bot]

Comment on attachment 151461 [details]
Patch

Attachment 151461 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13183080

[Alexis Menard (darktears)]

Created attachment 151465 [details]
Patch

[Build Bot]

Comment on attachment 151465 [details]
Patch

Attachment 151465 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13053003

[Alexis Menard (darktears)]

Created attachment 151470 [details]
Patch

[WebKit Review Bot]

Attachment 151470 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmak..." exit_code: 1
Source/WTF/wtf/DebugHelpers.h:52:  Missing spaces around <  [whitespace/operators] [3]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexis Menard (darktears)]

Created attachment 151489 [details]
Patch

[Balazs Kelemen]

Comment on attachment 151489 [details]
Patch

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

My informal comments on the patch. In general I think this is useful stuff.

> Source/WTF/wtf/DebugHelpers.h:76
> +    inline WTFDebug& space()
> +    {
> +        m_stream->m_space = true;
> +        m_stream->buffer.append(' ');
> +        return *this;
> +    }
> +    inline WTFDebug& nospace()
> +    {
> +        m_stream->m_space = false;
> +        return *this;
> +    }
> +    inline WTFDebug& maybeSpace()
> +    {
> +        if (m_stream->m_space)
> +            m_stream->buffer.append(' ');
> +        return *this;
> +    }

It's not clear when is maybeSpace() supposed to be used. Could you give an example?

> Source/WTF/wtf/DebugHelpers.h:145
> +    class Stream : public RefCounted<Stream> {

Why is it refcounted?

[Alexis Menard (darktears)]

Comment on attachment 151489 [details]
Patch

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

>> Source/WTF/wtf/DebugHelpers.h:76
>> +    }
> 
> It's not clear when is maybeSpace() supposed to be used. Could you give an example?

All the overload of << use it. And you have the concept with inline WTFDebug& operator<<(WTFDebug debug, const Vector<T>& vector). I set the debug.nospace() in that function, all overloads (depending the template value) which will be used after will have a no space. But the regular case they have a space, that's why the maybeSpace().

>> Source/WTF/wtf/DebugHelpers.h:145
>> +    class Stream : public RefCounted<Stream> {
> 
> Why is it refcounted?

When using the default copy constructor m_stream will not be deep copied.

[Hugo Parente Lima]

Huge discussion on mailing list... on the bug report... so... Any conclusions?

Btw, I'm pro-debug-header-for-printf-style-debug-and-happiness.

[Alexis Menard (darktears)]

(In reply to comment #16)
> Huge discussion on mailing list... on the bug report... so... Any conclusions?
> 
> Btw, I'm pro-debug-header-for-printf-style-debug-and-happiness.

Based on the feedback of webkit-dev I will make another proposal.

[Rafael Brandao]

Created attachment 173272 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 173274 [details]
WIP that I have locally.

[Alexis Menard (darktears)]

Comment on attachment 173272 [details]
Patch

I don't think the flag is needed. It's one file to build so it's fine and you just don't include it if you don't need it. If I recall correctly on the ML it was wtfPrint that people preferred. Also it seems to be a better place on WTF than WebCore. It will not work on Mac also, it seems you need to output in stderr.

[Rafael Brandao]

When I have tried to use the << operator over WTF and isolate the world of debuggers like I did in this patch on it (rather than adding in IntRect the way we use debug with it), I've noticed that it would create a cyclic dependency over WTF and WebCore. WTF should not need to use types like WebCore's IntRect on it. Obviously this just happened because I've went into this isolated approach, but this brought my attention to whether we should put this debug after all. I've decided to move to WebCore instead. Is this a bad move?

It's also by purpose that I didn't put space between debugs. I've went into a simplistic approach, the user should be free to print the way he wants to, like: debug(__PRETTY_FUNCTION__, ": ", rect, "\n");

I've decided to put a flag just to avoid breakage or including unnecessary functions over platforms that don't really think it is useful. But for the others that think it can be helpful, then you should pass --debug-helpers on build to enable it.

[Rafael Brandao]

(In reply to comment #21)
> When I have tried to use the << operator over WTF and isolate the world of debuggers like I did in this patch on it (rather than adding in IntRect the way we use debug with it), I've noticed that it would create a cyclic dependency over WTF and WebCore. WTF should not need to use types like WebCore's IntRect on it. Obviously this just happened because I've went into this isolated approach, but this brought my attention to whether we should put this debug after all. I've decided to move to WebCore instead. Is this a bad move?
> 
> It's also by purpose that I didn't put space between debugs. I've went into a simplistic approach, the user should be free to print the way he wants to, like: debug(__PRETTY_FUNCTION__, ": ", rect, "\n");
> 
> I've decided to put a flag just to avoid breakage or including unnecessary functions over platforms that don't really think it is useful. But for the others that think it can be helpful, then you should pass --debug-helpers on build to enable it.

s/to whether we should/to where we should

[Alexis Menard (darktears)]

(In reply to comment #21)
> When I have tried to use the << operator over WTF and isolate the world of debuggers like I did in this patch on it (rather than adding in IntRect the way we use debug with it), I've noticed that it would create a cyclic dependency over WTF and WebCore. WTF should not need to use types like WebCore's IntRect on it. Obviously this just happened because I've went into this isolated approach, but this brought my attention to whether we should put this debug after all. I've decided to move to WebCore instead. Is this a bad move?
> 

People didn't want << so you should not worry about those.

> It's also by purpose that I didn't put space between debugs. I've went into a simplistic approach, the user should be free to print the way he wants to, like: debug(__PRETTY_FUNCTION__, ": ", rect, "\n");
> 

ok.

> I've decided to put a flag just to avoid breakage or including unnecessary functions over platforms that don't really think it is useful. But for the others that think it can be helpful, then you should pass --debug-helpers on build to enable it.

[Benjamin Poulain]

*** Bug 110452 has been marked as a duplicate of this bug. ***

[Martin Robinson]

Comment on attachment 173272 [details]
Patch

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

> configure.ac:543
> +# check whether to enable DebugHelpers.h support
> +AC_MSG_CHECKING([whether to enable DebugHelpers.h support])
> +AC_ARG_ENABLE(debug_helpers,
> +              AC_HELP_STRING([--enable-debug-helpers],
> +                             [enable support for DebugHelpers.h [default=no]]),
> +              [],[enable_debug_helpers="no"])
> +AC_MSG_RESULT([$enable_debug_helpers])

This change and the change to the makefile shouldn't be necessary. Please see: http://trac.webkit.org/wiki/AddingFeatures

[Martin Robinson]

A feature flag for easier printf support? That seems a bit overkill, but I don't know the whole story.

[Hugo Parente Lima]

The original idea was to have only a header in WebKit source used by nobody, so when someone want to debug something he just include this header and enjoy the happiness of an ease print-style-debug :-)

[Filip Pizlo]

Comment on attachment 173272 [details]
Patch

Have you seen wtf/DataLog and wtf/PrintStream?  Would that do what you want?

In particular you can make any object printable by just implementing WTF::printInternal(PrintStream&, const YourType&)