Bug 471829 - kimageformats 5.107.0 build failure: src/imageformats/psd.cpp:828:23: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
Summary: kimageformats 5.107.0 build failure: src/imageformats/psd.cpp:828:23: error: ...
Status: RESOLVED FIXED
Alias: None
Product: frameworks-kimageformats
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: 5.107.0
Platform: Gentoo Packages Linux
: NOR normal
Target Milestone: ---
Assignee: Alex Merry
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-07-01 12:56 UTC by kocelfc
Modified: 2023-07-16 08:07 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
complete build log (70.55 KB, text/x-log)
2023-07-01 12:56 UTC, kocelfc
Details

Note You need to log in before you can comment on or make changes to this bug.
Description kocelfc 2023-07-01 12:56:06 UTC
Created attachment 160020 [details]
complete build log

SUMMARY
kimageformats 5.107.0 fails to compile with -Werror=strict-aliasing 

FAILED: src/imageformats/CMakeFiles/kimg_psd.dir/psd.cpp.o 
/usr/bin/x86_64-pc-linux-gnu-g++ -DKF_DEPRECATED_WARNINGS_SINCE=0x60000 -DKF_DISABLE_DEPRECATED_BEFORE_AND_AT=0x55f00 -DQT_CORE_LIB -DQT_DEPRECATED_WARNINGS_SINCE=0x60000 -DQT_DISABLE_DEPRECATED_BEFORE=0x50f02 -DQT_GUI_LIB -DQT_NO_CAST_FROM_ASCII -DQT_NO_CAST_FROM_BYTEARRAY -DQT_NO_CAST_TO_ASCII -DQT_NO_DEBUG -DQT_NO_FOREACH -DQT_NO_KEYWORDS -DQT_NO_NARROWING_CONVERSIONS_IN_CONNECT -DQT_NO_URL_CAST_FROM_STRING -DQT_STRICT_ITERATORS -DQT_USE_QSTRINGBUILDER -D_GNU_SOURCE -D_LARGEFILE64_SOURCE -Dkimg_psd_EXPORTS -I/var/tmp/portage/kde-frameworks/kimageformats-5.107.0/work/kimageformats-5.107.0_build/src/imageformats -I/var/tmp/portage/kde-frameworks/kimageformats-5.107.0/work/kimageformats-5.107.0/src/imageformats -I/var/tmp/portage/kde-frameworks/kimageformats-5.107.0/work/kimageformats-5.107.0_build/src/imageformats/kimg_psd_autogen/include -isystem /usr/include/qt5 -isystem /usr/include/qt5/QtGui -isystem /usr/include/qt5/QtCore -isystem /usr/lib64/qt5/mkspecs/linux-g++  -DQT_NO_DEBUG -O2 -Werror=strict-aliasing -Wl,-O1 -Wl,--as-needed -fno-operator-names -fno-exceptions -Wall -Wextra -Wcast-align -Wchar-subscripts -Wformat-security -Wno-long-long -Wpointer-arith -Wundef -Wnon-virtual-dtor -Woverloaded-virtual -Werror=return-type -Werror=init-self -Werror=undef -Wvla -Wdate-time -Wsuggest-override -Wlogical-op -pedantic -Wzero-as-null-pointer-constant -Wmissing-include-dirs -fdiagnostics-color=always -std=c++17 -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -fPIC -MD -MT src/imageformats/CMakeFiles/kimg_psd.dir/psd.cpp.o -MF src/imageformats/CMakeFiles/kimg_psd.dir/psd.cpp.o.d -o src/imageformats/CMakeFiles/kimg_psd.dir/psd.cpp.o -c /var/tmp/portage/kde-frameworks/kimageformats-5.107.0/work/kimageformats-5.107.0/src/imageformats/psd.cpp
/var/tmp/portage/kde-frameworks/kimageformats-5.107.0/work/kimageformats-5.107.0/src/imageformats/psd.cpp: In instantiation of ‘void {anonymous}::planarToChunchyFloat(uchar*, const char*, qint32, qint32, qint32) [with T = unsigned int; T min = 0; T max = 1; uchar = unsigned char; qint32 = int]’:
/var/tmp/portage/kde-frameworks/kimageformats-5.107.0/work/kimageformats-5.107.0/src/imageformats/psd.cpp:1143:50:   required from here
/var/tmp/portage/kde-frameworks/kimageformats-5.107.0/work/kimageformats-5.107.0/src/imageformats/psd.cpp:828:23: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
  828 |         auto ftmp = (*reinterpret_cast<float*>(&tmp) - double(min)) / (double(max) - double(min));
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: some warnings being treated as errors


STEPS TO REPRODUCE
1. Add -Werror=strict-aliasing to CXXFLAGS
2. Try to build
3. Build failure

SOFTWARE/OS VERSIONS
Gentoo Linux 2.13
KDE Plasma Version: 5.27.6
KDE Frameworks Version: 5.107.0
Qt Version: 5.15.10

ADDITIONAL INFORMATION
GCC version: Gentoo 13.1.1_p20230527
Comment 1 Albert Astals Cid 2023-07-02 19:54:36 UTC
Can you prove that's a but instead of you shooting yourself on the feet?
Comment 2 Albert Astals Cid 2023-07-02 19:55:38 UTC
(In reply to Albert Astals Cid from comment #1)
> Can you prove that's a but instead of you shooting yourself on the feet?

but -> bug
Comment 3 Sam James 2023-07-02 20:04:07 UTC
planarToChunchyFloat gets called with a quint32. You can't just reinterpret_cast to treat it as a float*.

Putting it another way: why would it be okay?

This warning appears regardless of -Werror and is enabled by -Wall, the user here is just setting -Werror to find likely miscompilations. GCC warngs by default only for -Wstrict-aliasing=3, where it's extremely confident that aliasing rules were violated (hence the "will break...").
Comment 4 Albert Astals Cid 2023-07-09 21:47:57 UTC
Mirco can you please have a look to see if there's any value in these claims?
Comment 5 Ninpo 2023-07-10 00:49:29 UTC
(In reply to Albert Astals Cid from comment #4)
> Mirco can you please have a look to see if there's any value in these claims?

Might be a language barrier thing, but this comes across as rather condescending and hostile to people reporting a legitimate build failure and bug. Commenting as someone that's used KDE almost exclusively as a DE since 1999 I'm extremely disappointed.
Comment 6 Mirco Miranda 2023-07-10 05:48:56 UTC
(In reply to Albert Astals Cid from comment #4)
> Mirco can you please have a look to see if there's any value in these claims?

The code was intentionally written like this. I think btw it could be rewritten a little cleaner.
Anyway, if it hits that function it's because the data is a 32-bit float. 32-bit PSD files are always floats.

If also on my debian adding -Werror=strict-aliasing I get the same error I try to rewrite that piece of code.
Comment 7 Mirco Miranda 2023-07-10 13:22:51 UTC
(In reply to Mirco Miranda from comment #6)

> If also on my debian adding -Werror=strict-aliasing I get the same error I
> try to rewrite that piece of code.

I was be able to reproduce it using GCC 13.0 (release only).
I wrote a work-around. If you can try it: https://invent.kde.org/frameworks/kimageformats/-/merge_requests/159
Comment 8 kocelfc 2023-07-10 13:30:56 UTC
(In reply to Mirco Miranda from comment #7)
> (In reply to Mirco Miranda from comment #6)
> 
> > If also on my debian adding -Werror=strict-aliasing I get the same error I
> > try to rewrite that piece of code.
> 
> I was be able to reproduce it using GCC 13.0 (release only).
> I wrote a work-around. If you can try it:
> https://invent.kde.org/frameworks/kimageformats/-/merge_requests/159

Can confirm that the MR makes it build properly
Comment 9 Albert Astals Cid 2023-07-10 17:37:55 UTC
> (In reply to Albert Astals Cid from comment #4)
> > Mirco can you please have a look to see if there's any value in these claims?
> 
> Might be a language barrier thing, but this comes across as rather
> condescending and hostile to people reporting a legitimate build failure and
> bug. Commenting as someone that's used KDE almost exclusively as a DE since
> 1999 I'm extremely disappointed.

It is not a legitimate build failure, the reporter is adding -Werror on their own and suffering because of it, noone forced them to have this problem but themselves.
Comment 10 Ninpo 2023-07-10 19:02:06 UTC
(In reply to Albert Astals Cid from comment #9)
> > (In reply to Albert Astals Cid from comment #4)
> > > Mirco can you please have a look to see if there's any value in these claims?
> > 
> > Might be a language barrier thing, but this comes across as rather
> > condescending and hostile to people reporting a legitimate build failure and
> > bug. Commenting as someone that's used KDE almost exclusively as a DE since
> > 1999 I'm extremely disappointed.
> 
> It is not a legitimate build failure, the reporter is adding -Werror on
> their own and suffering because of it, noone forced them to have this
> problem but themselves.

Ummm...what?  -Wall enables -Wstrict-aliasing=3 by default. From the gcc man page:
> Level 3 (default for -Wstrict-aliasing): Should have very few false positives and few false negatives.  Slightly slower than levels 1 or 2 when optimization is enabled.  Takes care of the common pun+dereference pattern in the front end: "*(int*)&some_float".  If optimization is enabled, it also runs in the back end, where it deals with multiple statement cases using flow-sensitive points-to information.  Only warns when the converted pointer is dereferenced.  Does not warn about incomplete types.

These are typically things you want to catch and are legitimate bugs worth reporting. -Werror=strict-aliasing makes it easier to spot and report these rather than waiting for a random run time error where the user then has to get backtraces to report the same bug.

Do you think turning the -Werror=strict-aliasing off makes the bug go away?  I assure you it does not, here's my build log for the same package:
> [with T = unsigned int; T min = 0; T max = 1; uchar = unsigned char; qint32 = int]’:
> ../kimageformats-5.107.0/src/imageformats/psd.cpp:1143:50:   required from here
> ../kimageformats-5.107.0/src/imageformats/psd.cpp:828:23: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
>  828 |         auto ftmp = (*reinterpret_cast<float*>(&tmp) - double(min)) / (double(max) - double(min));

It's a legitimate build failure (last I checked there's no DO NOT USE in the gcc man page for the flag) based off a legitimate strict-aliasing bug in the code. So it's been reported to upstream, as is the way with FOSS.

You haven't answered why it's OK to violate strict aliasing in this code (making this a not legitimate bug). It's only an illegitimate build failure if the user turned -Werror=strict-aliasing on by accident which they clearly did not.
Comment 11 Albert Astals Cid 2023-07-16 08:04:02 UTC
Git commit c3a91c3bc62bdd913c55dd83f4e1159ed25310c5 by Albert Astals Cid, on behalf of Mirco Miranda.
Committed on 16/07/2023 at 08:03.
Pushed by aacid into branch 'kf5'.

psd: Fix UB type punning

M  +26   -7    src/imageformats/psd.cpp

https://invent.kde.org/frameworks/kimageformats/-/commit/c3a91c3bc62bdd913c55dd83f4e1159ed25310c5
Comment 12 Albert Astals Cid 2023-07-16 08:07:17 UTC
Git commit 491b223c1597f21e688d158318d7c1a4cb39b1ab by Albert Astals Cid, on behalf of Mirco Miranda.
Committed on 16/07/2023 at 08:07.
Pushed by aacid into branch 'master'.

psd: Fix UB type punning

M  +13   -6    src/imageformats/psd.cpp

https://invent.kde.org/frameworks/kimageformats/-/commit/491b223c1597f21e688d158318d7c1a4cb39b1ab