[Development] Are we free of code that checks this isn't null?

[Olivier Goffart]

Am Donnerstag, 3. März 2016, 23:59:59 CET schrieb Thiago Macieira:
> On sexta-feira, 4 de março de 2016 09:58:53 PST Marc Mutz wrote:
> > On Friday 04 March 2016 07:52:15 Thiago Macieira wrote:
> > > Found in GCC 6's changelog (http://gcc.gnu.org/gcc-6/changes.html):
> > > > Value range propagation now assumes that the this pointer of C++
> > > > member
> > > > functions is non-null. This eliminates common null pointer checks but
> > > > also breaks some non-conforming code-bases (such as Qt-5, Chromium,
> > > > KDevelop). As a temporary work-around -fno-delete-null-pointer-checks
> > > > can be used. Wrong code can be identified by using
> > > > -fsanitize=undefined.
> > > 
> > > Are we free of such mistakes? Or do we need to enable -fno-delete-null-
> > > pointer-checks?
> > 
> > This comes to mind;
> > 
> >   QPointer<QObject> that = this;
> >   // ...
> >   if (!that) return;
> > 
> > Not sure it's affected, though.
> 
> Shouldn't be because that !that call isn't checking the this pointer, but
> instead is checking the strong refcount inside the QSharedPointer.
> 
> > Don't know off-handedly anything else that where this == nullptr would be
> > valid. Only way to find out is to enable -fno-delete-null-pointer-checks
> > except for -developer-build and trying to fix the ubsan issues.
> 
> It's not valid. The this pointer is never null, according to the standard.
> 
> And yet we had code like that in V4 (qtdeclarative). Clang started
> complaining about it and I tried to fix it by just removing the checks, but
> they were apparently in use. Since I don't remember seeing that warning
> anymore, it's likely to be fixed.

It was fixed in 0704d2be63b484cb579c1507223db3f914b1338a

> 
> But I am asking to be sure.
> 
> > /me upgrades to GCC 6.
> 
> I might still have a patch or two that aren't merged that fix GCC 6
> warnings. But when you compile with it, can you make sure it doesn't print
> any misleading-identation warnings? There were a couple of false positives
> that I reported and GCC fixed.


I grepped for the warning. Here is all the files I could find.

https://code.woboq.org/qt5/qtscript/src/3rdparty/javascriptcore/JavaScriptCore/API/OpaqueJSString.h.html#50
https://code.woboq.org/qt5/qtscript/src/3rdparty/javascriptcore/JavaScriptCore/API/OpaqueJSString.cpp.html#44
https://code.woboq.org/qt5/qtwebkit/Source/JavaScriptCore/API/OpaqueJSString.h.html#56
https://code.woboq.org/qt5/qtwebkit/Source/JavaScriptCore/API/OpaqueJSString.cpp.html#44
https://code.woboq.org/qt5/qtwebkit/Source/JavaScriptCore/parser/SourceProvider.h.html#58
https://code.woboq.org/qt5/qtwebkit/Source/WebCore/html/HTMLElement.cpp.html#509
https://code.woboq.org/qt5/qtwebkit/Source/WebCore/rendering/RenderBlock.cpp.html#7403
https://code.woboq.org/qt5/qtwebkit/Source/WebCore/rendering/RenderObject.cpp.html#1593


So only in deprecated modules.

-- 
Olivier 

Woboq - Qt services and support - https://woboq.com - https://code.woboq.org