Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix shift-sign issue in Convert.c #7838

Merged
merged 3 commits into from Mar 9, 2024
Merged

Conversation

r-barnes
Copy link
Contributor

@r-barnes r-barnes commented Feb 27, 2024

Fixes

libImaging/Convert.c:513:25: error: signed shift result (0xFF000000) sets the sign bit of the shift expression's type ('int') and becomes negative [-Werror,-Wshift-sign-overflow]
    UINT32 trns = (0xff << 24) | ((b & 0xff) << 16) | ((g & 0xff) << 8) | (r & 0xff);
                   ~~~~ ^  ~~

Fixes
```
libImaging/Convert.c:513:25: error: signed shift result (0xFF000000) sets the sign bit of the shift expression's type ('int') and becomes negative [-Werror,-Wshift-sign-overflow]
    UINT32 trns = (0xff << 24) | ((b & 0xff) << 16) | ((g & 0xff) << 8) | (r & 0xff);
                   ~~~~ ^  ~~
```
@radarhere
Copy link
Member

Hi. A quick glance through some of our CI jobs, and I don't see this problem on main. Could you provide some more information about the environment where you're seeing this?

@r-barnes
Copy link
Contributor Author

@radarhere - We compile pillow using a separate build system to get better LTO.

This build system has -Wshift-sign-overflow enabled for code safety reasons (it catches bugs!). It's therefore advantageous to have as much upstream code as possible be able to compile safely.

You could enable -Wshift-sign-overflow in pillow, though I don't know how to do this myself, but, in the meantime, godbolt demonstrates the failure indicated above.

src/libImaging/Convert.c Outdated Show resolved Hide resolved
src/libImaging/Convert.c Outdated Show resolved Hide resolved
src/libImaging/Convert.c Outdated Show resolved Hide resolved
src/libImaging/Convert.c Outdated Show resolved Hide resolved
@radarhere radarhere merged commit 38cec87 into python-pillow:main Mar 9, 2024
55 of 58 checks passed
nulano added a commit to nulano/Pillow that referenced this pull request Mar 9, 2024
@nulano
Copy link
Contributor

nulano commented Mar 9, 2024

You could enable -Wshift-sign-overflow in pillow, though I don't know how to do this myself,

Generally, compiler flags specific to CI are listed here

CFLAGS="-coverage -Werror=implicit-function-declaration" python3 -m pip -v install .

but it seems this warning is not supported by GCC, only Clang.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants