[Development] cast ... increases required alignment of target type [-Werror=cast-align]
Rohan McGovern
This code in qlist.h:
409: template <typename T>
410: Q_INLINE_TEMPLATE void QList<T>::node_destruct(Node *from, Node *to)
411: {
412: if (QTypeInfo<T>::isLarge || QTypeInfo<T>::isStatic)
413: while(from != to) --to, delete reinterpret_cast<T*>(to->v);
414: else if (QTypeInfo<T>::isComplex)
415: while (from != to) --to, reinterpret_cast<T*>(to)->~T();
416: }
...when compiled for ARM, causes this warning (or error with -Werror):
src/corelib/tools/qlist.h: In member function ‘void QList<T>::node_destruct(QList<T>::Node*, QList<T>::Node*) [with T = QVariant]’:
src/corelib/tools/qlist.h:738:5: instantiated from ‘void QList<T>::dealloc(QListData::Data*) [with T = QVariant]’
src/corelib/tools/qlist.h:714:9: instantiated from ‘QList<T>::~QList() [with T = QVariant]’
src/corelib/statemachine/qstatemachine.h:81:59: instantiated from here
src/corelib/tools/qlist.h:415:28: error: cast from ‘QList<QVariant>::Node*’ to ‘QVariant*’ increases required alignment of target type [-Werror=cast-align]
"do not compile with -Wcast-align" may be a valid answer, but it would
be good to hear from someone who can confidently say whether or not this
issue can actually harm us in practice.
_______________________________________________
Development mailing list
Devel...@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
Thiago Macieira
> Does anyone have a suggestion on how to fix this warning?
>
> This code in qlist.h:
>
> 409: template <typename T>
> 410: Q_INLINE_TEMPLATE void QList<T>::node_destruct(Node *from, Node *to)
> 411: {
> 412: if (QTypeInfo<T>::isLarge || QTypeInfo<T>::isStatic)
> 413: while(from != to) --to, delete reinterpret_cast<T*>(to->v);
> 414: else if (QTypeInfo<T>::isComplex)
> 415: while (from != to) --to, reinterpret_cast<T*>(to)->~T();
> 416: }
> src/corelib/tools/qlist.h:415:28: error: cast from
> ‘QList<QVariant>::Node*’ to ‘QVariant*’ increases required alignment of
> target type [-Werror=cast-align]
QList<QVariant>::Node is {
void *v;
}; (sizeof == 4, alignof == 4)
QVariant is {
struct Private {
union Data {
// among other things
qlonglong ll;
qulonglong ull;
double d;
};
uint type;
} d;
} (sizeof == 12, alignof == 8)
There is an increased alignment requirement in there. However, the side of the
branch being considered, line 415, is never executed because isLarge == true.
So this warning is a false positive on a line that gets never compiled.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
Intel Sweden AB - Registration Number: 556189-6027
Knarrarnäsgatan 15, 164 40 Kista, Stockholm, Sweden
Rohan McGovern
> On quarta-feira, 7 de março de 2012 09.34.35, Rohan McGovern wrote:
> > Does anyone have a suggestion on how to fix this warning?
> >
> > This code in qlist.h:
> >
> > 409: template <typename T>
> > 410: Q_INLINE_TEMPLATE void QList<T>::node_destruct(Node *from, Node *to)
> > 411: {
> > 412: if (QTypeInfo<T>::isLarge || QTypeInfo<T>::isStatic)
> > 413: while(from != to) --to, delete reinterpret_cast<T*>(to->v);
> > 414: else if (QTypeInfo<T>::isComplex)
> > 415: while (from != to) --to, reinterpret_cast<T*>(to)->~T();
> > 416: }
>
> > src/corelib/tools/qlist.h:415:28: error: cast from
> > ‘QList<QVariant>::Node*’ to ‘QVariant*’ increases required alignment of
> > target type [-Werror=cast-align]
>
> branch being considered, line 415, is never executed because isLarge == true.
> So this warning is a false positive on a line that gets never compiled.
>
Thanks, so can we use #pragma GCC diagnostic to selectively silence
warnings like this? There doesn't seem to be much precedent for it so
far (just one usage I can find in qatomic.h).
Atlant Schmidt
Because no one more-authoritative has answered you,
I'll give it a try.
I think that this occurs because QVariant must have
sufficiently-strict alignment constraints to hold,
properly aligned, the largest POD datum that can be
stored in the variant. Other, more-specific objects
may be known to not have such a large POD datum
within them and so may have looser alignment
requirements.
When the more-loosely aligned object is cast to an
object type with tighter alignment constraints, the
compiler objects.
In this exact case, it doesn't look like it will hurt
you, but I think it would be good to code this in
such a way as to avoid the warning (without disabling
the warning). Unfortunately, I have no advice on how
to do this.
Atlant
This e-mail and the information, including any attachments, it contains are intended to be a confidential communication only to the person or entity to whom it is addressed and may contain information that is privileged. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately notify the sender and destroy the original message.
Thank you.
Please consider the environment before printing this email.
Thiago Macieira
> Thanks, so can we use #pragma GCC diagnostic to selectively silence
> warnings like this? There doesn't seem to be much precedent for it so
> far (just one usage I can find in qatomic.h).
We can do that to silence the warning.
However, the proper solution is to redesign the function in question so that
the bad code never appears on it. The STL solution for this is to use tag
types and indirect via another function call.
João knows all about this and he had a plan to implement this cleaner code at
one point.
That's why we need static_if in C++2x :-)
Holger Hans Peter Freyther
> On quarta-feira, 7 de março de 2012 19.36.17, Rohan McGovern wrote:
>> Thanks, so can we use #pragma GCC diagnostic to selectively silence
>> warnings like this? There doesn't seem to be much precedent for it so
>> far (just one usage I can find in qatomic.h).
>
> We can do that to silence the warning.
>
> However, the proper solution is to redesign the function in question so that
> the bad code never appears on it. The STL solution for this is to use tag
> types and indirect via another function call.
Hi,
tests/auto/other/headersclean fails for MIPS due alignment warnings as well
(QList/QVector...). Is there any plan to resolve this before Qt Alpha?
regards
holger
lars....@nokia.com
wrote:
>On 03/07/2012 05:12 PM, Thiago Macieira wrote:
>> On quarta-feira, 7 de março de 2012 19.36.17, Rohan McGovern wrote:
>>> Thanks, so can we use #pragma GCC diagnostic to selectively silence
>>> warnings like this? There doesn't seem to be much precedent for it so
>>> far (just one usage I can find in qatomic.h).
>>
>> We can do that to silence the warning.
>>
>> However, the proper solution is to redesign the function in question so
>>that
>> the bad code never appears on it. The STL solution for this is to use
>>tag
>> types and indirect via another function call.
>
>
>Hi,
>
>tests/auto/other/headersclean fails for MIPS due alignment warnings as
>well
>(QList/QVector...). Is there any plan to resolve this before Qt Alpha?
Only if someone provides a patch. I won't stop the alpha because of a
header file warning.
Cheers,
Lars
Holger Hans Peter Freyther
>>
>> tests/auto/other/headersclean
> Only if someone provides a patch. I won't stop the alpha because of a
> header file warning.
it is a build breakage of the above test (the .pri file adds various -W* flags
to the compilation, e.g. -Wcast-align) and I didn't ask for the alpha to be
stopped because of that.
Possible options:
a.) fix the underlying warning (probably very tough for alpha one)
b.) remove -Wcast-align from the headersclean.pri for the alpha one?
c.) don't mind that qtbase does not compile for MIPS in the alpha
d.) ???
cheers
holger
lars....@nokia.com
wrote:
>On 03/14/2012 10:26 PM, lars....@nokia.com wrote:
>
>>>
>>> tests/auto/other/headersclean
>
>> Only if someone provides a patch. I won't stop the alpha because of a
>> header file warning.
>
>it is a build breakage of the above test (the .pri file adds various -W*
>flags
>to the compilation, e.g. -Wcast-align) and I didn't ask for the alpha to
>be
>stopped because of that.
>
>Possible options:
> a.) fix the underlying warning (probably very tough for alpha one)
> b.) remove -Wcast-align from the headersclean.pri for the alpha one?
> c.) don't mind that qtbase does not compile for MIPS in the alpha
> d.) ???
I'd go for (b), but if possible only for MIPS.
Cheers,
Lars
Thiago Macieira
> >Possible options:
> > a.) fix the underlying warning (probably very tough for alpha one)
> > b.) remove -Wcast-align from the headersclean.pri for the alpha one?
> > c.) don't mind that qtbase does not compile for MIPS in the alpha
> > d.) ???
>
> I'd go for (b), but if possible only for MIPS.
Note that most users do not build the tests. So the build will not stop for
them.
-Werror should never be enabled on non-developer builds.
Rohan McGovern
> On quarta-feira, 14 de março de 2012 21.37.23, lars....@nokia.com wrote:
> > >Possible options:
> > > a.) fix the underlying warning (probably very tough for alpha one)
> > > b.) remove -Wcast-align from the headersclean.pri for the alpha one?
> > > c.) don't mind that qtbase does not compile for MIPS in the alpha
> > > d.) ???
> >
> > I'd go for (b), but if possible only for MIPS.
>
That is exactly what's done for ARM, it should be fine to do it for MIPS
too. http://codereview.qt-project.org/20033 does this.
> Note that most users do not build the tests. So the build will not stop for
> them.
>
> -Werror should never be enabled on non-developer builds.
>
You did approve the change which enables it, though :)
7e970eb58c71dc08981575c648a04d258dbf0684, I mean.
Do you think this test should now only be enabled on developer builds?
IMHO that goes against the intent that the real headers we ship in a
publicly-configured Qt are free of warnings.
Thiago Macieira
> You did approve the change which enables it, though
> Do you think this test should now only be enabled on developer builds?
> IMHO that goes against the intent that the real headers we ship in a
> publicly-configured Qt are free of warnings.
Well, since this is a test, no. The -Werror should stay.
However, anywhere else than a test, -Werror should not be enabled.
Thiago Macieira
> On quinta-feira, 15 de março de 2012 08.32.43, Rohan McGovern wrote:
> > You did approve the change which enables it, though
> > 7e970eb58c71dc08981575c648a04d258dbf0684, I mean.
> > Do you think this test should now only be enabled on developer builds?
> > IMHO that goes against the intent that the real headers we ship in a
> > publicly-configured Qt are free of warnings.
>
> Well, since this is a test, no. The -Werror should stay.
>
> However, anywhere else than a test, -Werror should not be enabled.
Thanks to Rohan for removing the warning.
But maybe we should remove it completely because it's quite clear that our
headers are not cast-alignment-clean. Until QList is fixed, the warning should
Holger Hans Peter Freyther
>
> That is exactly what's done for ARM, it should be fine to do it for MIPS
> too. http://codereview.qt-project.org/20033 does this.
>
thanks rohan!
PS: I have not tried building Qt for SH4 but I assume we run into similar
issues there.
Atlant Schmidt
> -Werror should never be enabled on non-developer builds.
Why not? Is the plan to ship code that contains
warnings?*
This is the sort of thing that destroyed Symbian.
When I last counted, the Symbian build routinely
produced more than 7,000 warnings. No one at Nokia
besides myself seemed to have any concern about
this nor any real idea what any of those warnings
represented.
And when I started tracking them down, I was able
to prove that at least a few of those warnings (30
or so, IIRC) were genuine coding failures and at
least a handful of those were the root cause of
bugs that were *ALREADY* in the bug tracking system.
Think of that: If some software engineer had only
taken the time to fix that (e.g.) "Variable used
before being initialized" warning, some set of
users in the field wouldn't be having their
Symbian phones routinely crashing!
But amid 7,000+ warnings, what's one more, ehh? ;-)
All code should build warning free, at least until
the customer introduces changes. And then, the fact
that the warning count went from *0* to n will probably
tip the user off that they just did something wrong,
whereas a change from 7,397 to 7,398 will probably
go unnoticed, just like it always did at Nokia.
Atlant
* Yes, I understand that in a configurable build,
eliminating all warnings from all configurations
is hard work. But that's why we get paid the
big bucks/Euros/Kroner/etc.
This e-mail and the information, including any attachments, it contains are intended to be a confidential communication only to the person or entity to whom it is addressed and may contain information that is privileged. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately notify the sender and destroy the original message.
Thank you.
Please consider the environment before printing this email.
Robin Burchell
<asch...@dekaresearch.com> wrote:
> Thiago:
>
>> -Werror should never be enabled on non-developer builds.
>
> Why not? Is the plan to ship code that contains
> warnings?*
Because those non-developers aren't developing Qt, and may not have
the capability to fix warnings, if they arise. They just want to build
Qt and use it.
Developer builds are another matter entirely, and I don't think anyone
would honestly be against an attempt to enable -Werror, though it's a
big job.
Atlant Schmidt
> Because those non-developers aren't developing Qt,
> and may not have the capability to fix warnings,
> if they arise. They just want to build Qt and use it.
So you don't want their bug reports when
their particular configuration (that the
Qt developers may not have tested) doesn't
work without producing warnings?
Atlant
-----Original Message-----
From: ro...@viroteck.net [mailto:ro...@viroteck.net] On Behalf Of Robin Burchell
Sent: Thursday, March 15, 2012 07:36
To: Atlant Schmidt
Cc: Thiago Macieira; devel...@qt-project.org
Subject: Re: [Development] cast ... increases required alignment of target type [-Werror=cast-align]
On Thu, Mar 15, 2012 at 12:20 PM, Atlant Schmidt
<asch...@dekaresearch.com> wrote:
> Thiago:
>
>> -Werror should never be enabled on non-developer builds.
>
> Why not? Is the plan to ship code that contains
> warnings?*
Because those non-developers aren't developing Qt, and may not have
the capability to fix warnings, if they arise. They just want to build
Qt and use it.
Developer builds are another matter entirely, and I don't think anyone
would honestly be against an attempt to enable -Werror, though it's a
big job.
Click https://www.mailcontrol.com/sr/lh1Nv5z4bnHTndxI!oX7UsdpzMR7Bo2KKNS9DqhBQIz1lKII56rKFTGW0a7+3ezIdfwN1IpAz7f55a0nEA+RCg== to report this email as spam.
This e-mail and the information, including any attachments, it contains are intended to be a confidential communication only to the person or entity to whom it is addressed and may contain information that is privileged. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately notify the sender and destroy the original message.
Thank you.
Please consider the environment before printing this email.
andre....@nokia.com
> > -Werror should never be enabled on non-developer builds.
>
> Why not? Is the plan to ship code that contains warnings?*
That is not the _plan_. But the plan is also not to jump through
hoops to silence bogus warnings, and the plan is also not to
purchase crystal balls to predict what the next release
of some compiler might consider worth a warning.
The released packages have to be usable for a while, turning
on -Werror is a safe way to make them unusable out-of-the-box
with the next patch release of your favourite compiler.
The released packages have also to be usable in environments
where "system" headers produce warnings.
For a quick check, pull a couple of source packages for your
favourite distribution and check how many have -Werror on
by default.
> And when I started tracking them down, I was able
> to prove that at least a few of those warnings
Nobody prevents you from turning on -Werror on a local
build and submit fixes.
> All code should build warning free, at least until
> the customer introduces changes.
I am afraid this is wishful thinking, certainly as long as you
don't fully control the environment in which the code is compiled.
Regards,
Andre'
Thiago Macieira
> > -Werror should never be enabled on non-developer builds.
>
> Why not? Is the plan to ship code that contains
> warnings?*
Qt should be always built with -Wall -Wextra, plus maybe one or two non-
standard warnings (candidates are -Wcast-align, -Wshadow, etc.). That's for
everyone:
- Qt developers working on Qt
- other developers building Qt
- other developers building their own apps that use Qt headers
Now, the -Werror flag is only enabled for the first group. We, the Qt
developers, fix those warnings. Other developers, in their course of using Qt,
which may involve building it first, should not have to deal with build errors
caused by warnings.
Warnings change frequently depending on the compiler version and optimisation
settings. New versions of the compilers will produce new warnings that we
hadn't seen before, and old versions we have not tested will produce old
false-positive warnings. For those reasons, enabling -Werror for anyone but us
is a mistake.