Skip to content

Fix -Wsign-compare on arm32#2432

Closed
kinkie wants to merge 4 commits into
squid-cache:masterfrom
kinkie:fix-cachedigest-overflow-v2
Closed

Fix -Wsign-compare on arm32#2432
kinkie wants to merge 4 commits into
squid-cache:masterfrom
kinkie:fix-cachedigest-overflow-v2

Conversation

@kinkie
Copy link
Copy Markdown
Contributor

@kinkie kinkie commented Jun 2, 2026

Due to ssize_t differences on 32/64 bit platforms, changes
to peerDigestSwapInMask in commit 556b91a cause
signedness comparison errors.
Refactor to be safe both on 32- and 64-bit platforms

@kinkie
Copy link
Copy Markdown
Contributor Author

kinkie commented Jun 2, 2026

Error message on gentoo/arm7l

src/peer_digest.cc: In function 'int peerDigestSwapInMask(void*, char*, ssize_t)':
src/peer_digest.cc:562:35: error: comparison of integer expressions of different signedness: 'uint32_t' {aka 'unsigned int'} and 'ssize_t' {aka 'int'} [-Werror=sign-compare]
  562 |     if (fetch->mask_offset + size > static_cast<ssize_t>(pd->cd->mask_size)) {
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

see https://github.com/kinkie/dockerfiles/actions/runs/26706938522/job/78709886696

Copy link
Copy Markdown
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this regression!

src/peer_digest.cc:562:35: error: comparison of integer expressions of different signedness: 'uint32_t' {aka 'unsigned int'} and 'ssize_t' {aka 'int'} [-Werror=sign-compare]

Refactor ... for 32-bit safety ... Due to ssize_t differences on 32/64 bit platforms ...

The jump from "expressions of different signedness" in the error message to "32-bit safety" feels unnecessary here because signedness is different on both 32- and 64-bit platforms AFAICT.

I suggest using something like this for the PR title: Fix -Wsign-compare on arm32.

Please mention the problematic commit SHA in the PR description (or title).

See 4efdc65 for an example.

Comment thread src/peer_digest.cc Outdated
@kinkie kinkie changed the title Refactor peerDigestSwapInMask for 32-bit safety Fix -Wsign-compare on arm32 Jun 2, 2026
@kinkie kinkie mentioned this pull request Jun 2, 2026
rousskov
rousskov previously approved these changes Jun 2, 2026
@kinkie kinkie requested a review from rousskov June 2, 2026 20:51
@kinkie kinkie added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) backport-to-v7 maintainer has approved these changes for v7 backporting labels Jun 2, 2026
@kinkie
Copy link
Copy Markdown
Contributor Author

kinkie commented Jun 2, 2026

@yadij would you mind fast tracking this PR? It's the only blocker I have for releasing 7.6

squid-anubis pushed a commit that referenced this pull request Jun 4, 2026
Due to ssize_t differences on 32/64 bit platforms, changes
to peerDigestSwapInMask in commit 556b91a cause
signedness comparison errors.
Refactor to be safe both on 32- and 64-bit platforms
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Jun 4, 2026
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Jun 4, 2026
@squidadm squidadm removed the backport-to-v7 maintainer has approved these changes for v7 backporting label Jun 4, 2026
@squidadm
Copy link
Copy Markdown
Collaborator

squidadm commented Jun 4, 2026

queued for backport to v7

kinkie added a commit that referenced this pull request Jun 4, 2026
Due to ssize_t differences on 32/64 bit platforms, changes
to peerDigestSwapInMask in commit 556b91a cause
signedness comparison errors.
Refactor to be safe both on 32- and 64-bit platforms
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

M-merged https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants