Bug 136599 - New clang warns about boolean checks for |this| pointer in RenderObject debug methods
Summary: New clang warns about boolean checks for |this| pointer in RenderObject debug...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-06 04:52 PDT by David Kilzer (:ddkilzer)
Modified: 2015-09-03 16:23 PDT (History)
9 users (show)

See Also:


Attachments
Patch v1 (3.14 KB, patch)
2014-09-06 06:53 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch (2.43 KB, patch)
2015-09-02 15:11 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (2.52 KB, patch)
2015-09-02 20:37 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2014-09-06 04:52:52 PDT
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.
Comment 1 David Kilzer (:ddkilzer) 2014-09-06 05:18:45 PDT
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.
Comment 2 David Kilzer (:ddkilzer) 2014-09-06 05:49:57 PDT
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).
Comment 3 David Kilzer (:ddkilzer) 2014-09-06 06:53:21 PDT
Created attachment 237735 [details]
Patch v1
Comment 4 WebKit Commit Bot 2014-09-06 09:04:36 PDT
Comment on attachment 237735 [details]
Patch v1

Clearing flags on attachment: 237735

Committed r173357: <http://trac.webkit.org/changeset/173357>
Comment 5 WebKit Commit Bot 2014-09-06 09:04:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Simon Fraser (smfr) 2014-09-07 23:11:03 PDT
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.
Comment 7 zalan 2014-09-08 07:19:37 PDT
(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.
Comment 8 Darin Adler 2014-09-08 08:00:26 PDT
(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.
Comment 9 Darin Adler 2014-09-08 08:01:50 PDT
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().
Comment 10 zalan 2014-09-08 08:16:45 PDT
(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.
Comment 11 Darin Adler 2014-09-08 10:25:05 PDT
(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.
Comment 12 zalan 2015-09-02 15:11:22 PDT
Reopening to attach new patch.
Comment 13 zalan 2015-09-02 15:11:26 PDT
Created attachment 260444 [details]
Patch
Comment 14 David Kilzer (:ddkilzer) 2015-09-02 15:21:57 PDT
Comment on attachment 260444 [details]
Patch

r=me
Comment 15 zalan 2015-09-02 15:24:34 PDT
(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)
Comment 16 WebKit Commit Bot 2015-09-02 16:08:47 PDT
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 'commit-queue@webkit.org': 
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
Comment 17 zalan 2015-09-02 20:37:16 PDT
Created attachment 260482 [details]
Patch
Comment 18 WebKit Commit Bot 2015-09-02 21:29:32 PDT
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 'commit-queue@webkit.org': 
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
Comment 19 WebKit Commit Bot 2015-09-03 16:23:50 PDT
Comment on attachment 260482 [details]
Patch

Clearing flags on attachment: 260482

Committed r189310: <http://trac.webkit.org/changeset/189310>
Comment 20 WebKit Commit Bot 2015-09-03 16:23:56 PDT
All reviewed patches have been landed.  Closing bug.