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 compiler warnings from utf8.c on 32-bit build #22899

Open
wants to merge 3 commits into
base: blead
Choose a base branch
from

Conversation

t-a-k
Copy link
Contributor

@t-a-k t-a-k commented Jan 8, 2025

This set of changes will silence compiler warnings from Perl_utf8_to_uv_msgs_helper_ function at utf8.c on 32-bit (e.g. x86) build:

utf8.c:1819:35: warning: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘int’} and ‘unsigned int’ [-Wsign-compare]
 1819 |             && UNLIKELY(expectlen > OFFUNISKIP(uv)))
      |                                   ^
utf8.c:1895:45: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘ssize_t’ {aka ‘int’} [-Wsign-compare]
 1895 |                 for (unsigned i = curlen; i < expectlen; i++) {
      |                                             ^
utf8.c:2098:34: warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘U32’ {aka ‘long unsigned int’} [-Wformat=]
 2098 |                 Perl_croak(aTHX_ "panic: Unexpected case value in "
      |                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2099 |                                  " utf8n_to_uvchr_msgs() %d", this_problem);
      |                                                               ~~~~~~~~~~~~
      |                                                               |
      |                                                               U32 {aka long unsigned int}

  • These changes should not change the behavior of perl, so I think that it does not require a perldelta entry.

t-a-k added 2 commits January 8, 2025 22:35
…_t in Perl_utf8_to_uv_msgs_helper_()

Changed the type of `curlen`, `expectlen`, and `avail_len` from `SSize_t`
to `Size_t`, to shut up compiler warnings about comparison between
same-sized signed and unsigned values on 32-bit build.
These variables represent the number of bytes which cannot be negative,
so it should be safe to be unsigned type.
…s_helper_

This call of croak() used to attempt to display U32 variable
`this_problem` with %d format specifier, which triggered a compiler
warning on 32-bit build where U32 is typedef'ed to unsigned long.
@jkeenan jkeenan added the build-time-warnings Replaces [META] Build-time warnings RT #133556 label Jan 8, 2025
utf8.c Outdated
@@ -1634,12 +1634,12 @@ Perl_utf8_to_uv_msgs_helper_(const U8 * const s0,
* than a single character */
const U8 * send = e;

SSize_t curlen = send - s0;
Size_t curlen = send - s0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't this be negative?

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be negative in non-DEBUGGING builds, where s0 <= send is treated as a reported error. For DEBUGGING builds perl asserts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for comments. I've forgotten a possibility of s0 > send.
I pushed a new commit to move this subtraction to the point after checking s0 < send to avoid underflow.

@jkeenan
Copy link
Contributor

jkeenan commented Jan 8, 2025

Per suggestion from @mauke on #p5p: We can compare the logs from two different CI runs, one without the patch in this p.r., one with it.

https://github.com/Perl/perl5/actions/runs/12667183737/job/35300550360#step:7:64

vs.

https://github.com/Perl/perl5/actions/runs/12674636619/job/35324408185?pr=22899#step:7:64

This suggests that the patch has the intended effect. However, I can't evaluate the code per se, and @mauke's concern above should be answered.

In Perl_utf8_to_uv_msgs_helper_(),  "curlen = send - s0;" used to be done
earlier in this function, but this subtraction might underflow as
"send >= s0" (that is, "e >= s0") does not necessarily hold true.

Thanks to @mauke and @tonycoz for pointing this out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-time-warnings Replaces [META] Build-time warnings RT #133556
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants