Bug 471829

Can you prove that's a but instead of you shooting yourself on the feet?

(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

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...").

Mirco can you please have a look to see if there's any value in these claims?

(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.

(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.

(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

(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

> (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.

(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.

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

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