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

2016-03-04 Thread Thiago Macieira 
On sábado, 5 de março de 2016 02:17:59 PST Marc Mutz wrote:
> On Saturday 05 March 2016 00:21:38 Thiago Macieira wrote:
> > On sexta-feira, 4 de março de 2016 23:27:49 PST Marc Mutz wrote:
> > > Do you also have a fix for this one:
> > >   ../../include/QtCore/../../../../qt5/qtbase/src/corelib/kernel/qobject
> > >   d
> > >   efs
> > > 
> > > .h:175:108: error:  ‘visibility’ attribute ignored [-Werror=attributes]
> > > 
> > >Q_DECL_HIDDEN_STATIC_METACALL static void
> > >qt_static_metacall(QObject
> > > 
> > > *,  QMetaObject::Call, int, void **); \
> > 
> > Yes, but I can't find it. I might have dropped it because it wasn't
> > working, due to a GCC bug. The bug has since been fixed.
> > 
> > We need to make Q_DECL_HIDDEN_STATIC_METACALL include the pragma disabling
> > that particular warning.
> > 
> > This is a blind, untested change:
> > https://codereview.qt-project.org/151467
> 
> Didn't work for me, but gave me the right idea. Fixed now. Updated.

Thanks.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

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

2016-03-04 Thread Marc Mutz 
On Saturday 05 March 2016 00:21:38 Thiago Macieira wrote:
> On sexta-feira, 4 de março de 2016 23:27:49 PST Marc Mutz wrote:
> > Do you also have a fix for this one:
> >   ../../include/QtCore/../../../../qt5/qtbase/src/corelib/kernel/qobjectd
> >   efs
> > 
> > .h:175:108: error:  ‘visibility’ attribute ignored [-Werror=attributes]
> > 
> >Q_DECL_HIDDEN_STATIC_METACALL static void
> >qt_static_metacall(QObject
> > 
> > *,  QMetaObject::Call, int, void **); \
> 
> Yes, but I can't find it. I might have dropped it because it wasn't
> working, due to a GCC bug. The bug has since been fixed.
> 
> We need to make Q_DECL_HIDDEN_STATIC_METACALL include the pragma disabling
> that particular warning.
> 
> This is a blind, untested change:
> https://codereview.qt-project.org/151467

Didn't work for me, but gave me the right idea. Fixed now. Updated.

Thanks,
Marc

-- 
Marc Mutz  | Senior Software Engineer
KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company
Tel: +49-30-521325470
KDAB - The Qt Experts
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

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

2016-03-04 Thread Thiago Macieira 
On sexta-feira, 4 de março de 2016 23:27:49 PST Marc Mutz wrote:
> Do you also have a fix for this one:
> 
>   ../../include/QtCore/../../../../qt5/qtbase/src/corelib/kernel/qobjectdefs
> .h:175:108: error:  ‘visibility’ attribute ignored [-Werror=attributes]
>Q_DECL_HIDDEN_STATIC_METACALL static void qt_static_metacall(QObject
> *,  QMetaObject::Call, int, void **); \

Yes, but I can't find it. I might have dropped it because it wasn't working, 
due to a GCC bug. The bug has since been fixed.

We need to make Q_DECL_HIDDEN_STATIC_METACALL include the pragma disabling 
that particular warning.

This is a blind, untested change:
https://codereview.qt-project.org/151467

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

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

2016-03-04 Thread Marc Mutz 
On Friday 04 March 2016 08:59:59 Thiago Macieira wrote:
> > /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.

Do you also have a fix for this one:

  
../../include/QtCore/../../../../qt5/qtbase/src/corelib/kernel/qobjectdefs.h:175:108:
 error: 
‘visibility’ attribute ignored [-Werror=attributes]
   Q_DECL_HIDDEN_STATIC_METACALL static void qt_static_metacall(QObject *, 
QMetaObject::Call, int, void **); \

  ^
  /home/marc/Qt/qt5/qtbase/src/widgets/dialogs/qcolordialog.cpp:181:5: note: in 
expansion of 
macro ‘Q_OBJECT’
   Q_OBJECT
   ^~~~

?

Thanks,
Marc

-- 
Marc Mutz  | Senior Software Engineer
KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company
Tel: +49-30-521325470
KDAB - The Qt Experts
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

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

2016-03-04 Thread 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 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

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

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

2016-03-04 Thread Knoll Lars 
On 04/03/16 08:59, "Development on behalf of Thiago Macieira" 
 wrote:



>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 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.
>
>But I am asking to be sure.

Qt QML should be ok. I went through those issues and fixed the at some point.

Cheers,
Lars

>
>> /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.
>
>-- 
>Thiago Macieira - thiago.macieira (AT) intel.com
>  Software Architect - Intel Open Source Technology Center
>
>___
>Development mailing list
>Development@qt-project.org
>http://lists.qt-project.org/mailman/listinfo/development
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

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

2016-03-04 Thread 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 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.

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.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

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

2016-03-03 Thread Marc Mutz 
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 that = this;
  // ...
  if (!that) return;

Not sure it's affected, though.

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.

We should eventually put a ubsan build into the CI, we're not /that/ far away 
from building cleanly. It's some time since I last checked, but I can't 
remember any 3rd-party libs triggering in a ubsan Qt builds.

/me upgrades to GCC 6.

Thanks,
Marc

-- 
Marc Mutz  | Senior Software Engineer
KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company
Tel: +49-30-521325470
KDAB - The Qt Experts
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development