Bug 136599 – New clang warns about boolean checks for |this| pointer in RenderObject debug methods

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

New builds of clang warn about boolean checks for the |this| pointer being undefined behavior:

Source/WebCore/rendering/RenderObject.cpp:1465:10: error: 'this' pointer cannot be null in wel
l-defined C++ code; pointer may be assumed to always convert to true [-Werror,-Wundefined-bool-conversion]
    if (!this) {
        ~^~~~
Source/WebCore/rendering/RenderObject.cpp:1584:10: error: 'this' pointer cannot be null in wel
l-defined C++ code; pointer may be assumed to always convert to true [-Werror,-Wundefined-bool-conversion]
    if (!this)
        ~^~~~
2 errors generated.

[David Kilzer (:ddkilzer)]

BTW, changing the check from "!this" to "this == nullptr" triggers a different warning:

Source/WebCore/rendering/RenderObject.cpp:1460:9: error: 'this' pointer cannot be null in well-defined C++ code; comparison may be assumed to always evaluate to false [-Werror,-Wtautological-undefined-compare]
    if (this == nullptr) {
        ^~~~    ~~~~~~~
Source/WebCore/rendering/RenderObject.cpp:1579:9: error: 'this' pointer cannot be null in well-defined C++ code; comparison may be assumed to always evaluate to false [-Werror,-Wtautological-undefined-compare]
    if (this == nullptr)
        ^~~~    ~~~~~~~
2 errors generated.

[David Kilzer (:ddkilzer)]

And adding #pragma statements to ignore the warning causes unknown warnings in older clang:

/Volumes/Data/WebKit.git/Source/WebCore/rendering/RenderObject.cpp:1457:34: error: unknown warning group '-Wundefined-bool-conversion', ignored [-Werror,-Wunknown-pragmas]
#pragma clang diagnostic ignored "-Wundefined-bool-conversion"
                                 ^
/Volumes/Data/WebKit.git/Source/WebCore/rendering/RenderObject.cpp:1576:34: error: unknown warning group '-Wundefined-bool-conversion', ignored [-Werror,-Wunknown-pragmas]
#pragma clang diagnostic ignored "-Wundefined-bool-conversion"
                                 ^
2 errors generated.

I guess I either need to ignore the unknown warnings in older clangs, or use __has_feature() (assuming that works for compiler flags).

[David Kilzer (:ddkilzer)]

Created attachment 237735 [details]
Patch v1

[WebKit Commit Bot]

Comment on attachment 237735 [details]
Patch v1

Clearing flags on attachment: 237735

Committed r173357: <http://trac.webkit.org/changeset/173357>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 237735 [details]
Patch v1

I think we should just remove the null check. We should never get here when |this| is null. It's debug-only code anyway.

[alan]

(In reply to comment #6)
> (From update of attachment 237735 [details])
> I think we should just remove the null check. We should never get here when |this| is null. It's debug-only code anyway.
In an lldb session, it's possible to call showRenderTree() on an invalid object.

[Darin Adler]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 237735 [details] [details])
> > I think we should just remove the null check. We should never get here when |this| is null. It's debug-only code anyway.
> In an lldb session, it's possible to call showRenderTree() on an invalid object.

Yes, but if you try it you’ll see there’s no disastrous result. I think we can consider taking out the null checks. Or we could make these non-member functions. I think showRenderTree(x) is just as easy to type in the debugger.

[Darin Adler]

I think we should optimize these various dumping functions for easy typing in lldb. For example, I think they should be called showTree, not showRenderTree, and I think that it’s easier to type showTree(x) than x->showTree().

[alan]

(In reply to comment #9)
> I think we should optimize these various dumping functions for easy typing in lldb. For example, I think they should be called showTree, not showRenderTree, and I think that it’s easier to type showTree(x) than x->showTree().
Currently we've got the following non-member show*Tree() functions.  
    void showNodeTree(const WebCore::RenderObject*);
    void showLineTree(const WebCore::RenderObject*);
    void showRenderTree(const WebCore::RenderObject*);

and the member helpers:
    void showNodeTreeForThis() const;
    void showRenderTreeForThis() const;
    void showLineTreeForThis() const;

so you can type showRenderTree(x) or showRenderTreeForThis() and they output the same when x == this.

[Darin Adler]

(In reply to comment #10)
> Currently we've got the following non-member show*Tree() functions.  
>     void showNodeTree(const WebCore::RenderObject*);
>     void showLineTree(const WebCore::RenderObject*);
>     void showRenderTree(const WebCore::RenderObject*);
> 
> and the member helpers:
>     void showNodeTreeForThis() const;
>     void showRenderTreeForThis() const;
>     void showLineTreeForThis() const;
> 
> so you can type showRenderTree(x) or showRenderTreeForThis() and they output the same when x == this.

OK. Lets take the null checks out of the member helpers and put them only in the free functions. Unless that creates any real inconvenience for someone.

[alan]

Reopening to attach new patch.

[alan]

Created attachment 260444 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 260444 [details]
Patch

r=me

[alan]

(In reply to comment #13)
> Created attachment 260444 [details]
> Patch
This is a follow up patch based on the discussion above. (tl;dr: there'd be no disastrous result, if this check was removed)

[WebKit Commit Bot]

Comment on attachment 260444 [details]
Patch

Rejecting attachment 260444 [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', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 260444, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
', 'dcommit', '--rmdir']" exit_code: 139 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
Authentication realm: <http://svn.webkit.org:80> Mac OS Forge
Password for '[e-mail]': 
Authentication realm: <http://svn.webkit.org:80> Mac OS Forge
Username: error: git-svn died of signal 11

Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 139 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/134415

[alan]

Created attachment 260482 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 260482 [details]
Patch

Rejecting attachment 260482 [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-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 260482, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
', 'dcommit', '--rmdir']" exit_code: 139 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
Authentication realm: <http://svn.webkit.org:80> Mac OS Forge
Password for '[e-mail]': 
Authentication realm: <http://svn.webkit.org:80> Mac OS Forge
Username: error: git-svn died of signal 11

Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 139 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/135259

[WebKit Commit Bot]

Comment on attachment 260482 [details]
Patch

Clearing flags on attachment: 260482

Committed r189310: <http://trac.webkit.org/changeset/189310>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.