Skip to content

Fix shift-sign issue in Convert.c#7838

Merged
radarhere merged 3 commits intopython-pillow:mainfrom
r-barnes:patch-1
Mar 9, 2024
Merged

Fix shift-sign issue in Convert.c#7838
radarhere merged 3 commits intopython-pillow:mainfrom
r-barnes:patch-1

Conversation

@r-barnes
Copy link
Copy Markdown
Contributor

@r-barnes r-barnes commented Feb 27, 2024 •
edited
Loading

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);
                   ~~~~ ^  ~~

@r-barnes
Fix shift-sign issue in Convert.c
6fe0d63 
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
Copy Markdown
Member

radarhere commented Feb 27, 2024

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
Copy Markdown
Contributor Author

r-barnes commented Feb 29, 2024

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

radarhere
radarhere reviewed Mar 1, 2024
View reviewed changes
radarhere
radarhere reviewed Mar 1, 2024
View reviewed changes
radarhere
radarhere reviewed Mar 9, 2024
View reviewed changes
radarhere
radarhere reviewed Mar 9, 2024
View reviewed changes
@radarhere radarhere merged commit 38cec87 into python-pillow:main Mar 9, 2024
nulano added a commit to nulano/Pillow that referenced this pull request Mar 9, 2024
@nulano
Revert "Fix shift-sign issue in Convert.c (python-pillow#7838)"
b49c98b 
This reverts commit 38cec87.
@nulano
Copy link
Copy Markdown
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

Pillow/Makefile

Line 63 in e42528e

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

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

@data-sync-user data-sync-user mentioned this pull request Jun 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@radarhere radarhere radarhere approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@r-barnes @radarhere @nulano