Skip to content

bpo-41366: Fix clang warning for sign conversion #21592

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

Merged
merged 1 commit into from
Jul 23, 2020

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Jul 22, 2020

Affects 3.9 beta and master.

Should I make a back port PR too?

https://bugs.python.org/issue41366

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@henryiii

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@WildCard65
Copy link
Contributor

Are you using a std-C implementation that doesn't define a "size_t" type?

@henryiii
Copy link
Contributor Author

henryiii commented Jul 22, 2020

I see a few other uses of size_t in the code base, so I assume this is allowed?

The problem is that length is now forced to Py_ssize_t (it used to be a macro argument in Python 3.8), so length * sizeof(Py_UNICODE) is signed * unsigned, and then used as unsigned in the memcpy function. Clang warns about this sign conversion unless it is explicit.

Note the Azure error is a timeout.

@WildCard65
Copy link
Contributor

WildCard65 commented Jul 22, 2020

@henryiii I ask because Python defines Py_ssize_t to size_t IF it exists.

#ifdef HAVE_SSIZE_T
typedef ssize_t         Py_ssize_t;
#elif SIZEOF_VOID_P == SIZEOF_SIZE_T
typedef Py_intptr_t     Py_ssize_t;
#else
#   error "Python needs a typedef for Py_ssize_t in pyport.h."
#endif

Edit: NVM, ssize_t is signed.

@henryiii
Copy link
Contributor Author

The issue here is signed vs. unsigned - ssize_t vs. size_t. size_t is part of the standard, ssize_t is not, which might be why that define is there.

@corona10 corona10 requested a review from methane July 23, 2020 02:13
@methane methane added needs backport to 3.9 only security fixes skip news type-bug An unexpected behavior, bug, or error labels Jul 23, 2020
@methane methane merged commit 680254a into python:master Jul 23, 2020
@miss-islington
Copy link
Contributor

Thanks @henryiii for the PR, and @methane for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @henryiii and @methane, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 680254a8dc64e3ada00f88a7c42d41eb02108353 3.9

@methane methane added needs backport to 3.9 only security fixes and removed needs backport to 3.9 only security fixes labels Jul 23, 2020
@miss-islington
Copy link
Contributor

Thanks @henryiii for the PR, and @methane for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 23, 2020
(cherry picked from commit 680254a)

Co-authored-by: Henry Schreiner <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Jul 23, 2020
@bedevere-bot
Copy link

GH-21600 is a backport of this pull request to the 3.9 branch.

miss-islington added a commit that referenced this pull request Jul 23, 2020
(cherry picked from commit 680254a)

Co-authored-by: Henry Schreiner <[email protected]>
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Aug 4, 2020
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Aug 20, 2020
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants