◐ Shell
reader mode source ↗
Skip to content

gh-111140: Fix edge case in PyLong_AsNativeBytes where large negative longs may require an extra byte#116053

Merged
encukou merged 19 commits into
python:mainfrom
zooba:gh-111140
Apr 5, 2024
Merged

gh-111140: Fix edge case in PyLong_AsNativeBytes where large negative longs may require an extra byte#116053
encukou merged 19 commits into
python:mainfrom
zooba:gh-111140

Conversation

@zooba

@zooba zooba commented Feb 28, 2024 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

@zooba zooba marked this pull request as ready for review February 28, 2024 15:58
@bedevere-app bedevere-app Bot mentioned this pull request Feb 28, 2024
Closed
@zooba zooba requested a review from gpshead February 28, 2024 16:22
@gpshead gpshead marked this pull request as draft February 28, 2024 23:55
@zooba

zooba commented Mar 1, 2024

Copy link
Copy Markdown
Member Author

I've improved detection of the two edge cases, though for now there isn't a full solution for the positive input->MSB set output case.

What I have tried though is returning a wildly different value from those cases and specifically testing for that, and it comes up just fine. So when we decide how we want to say "it's okay for positive inputs to set the MSB in the result", those TODOs can just come out and set res = n (plus whatever extra condition is being tested).

@zooba zooba marked this pull request as ready for review March 19, 2024 22:14
@zooba zooba mentioned this pull request Mar 28, 2024
Open
@zooba

zooba commented Mar 28, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Updated the endianness argument to flags so we can pass more options in there. Existing calls still work, as the 0/1/-1 options are unchanged - the only changed existing test was looking for an error condition that no longer exists.

I still need to add new tests.

@encukou you are likely interested in the parameter change here

encukou
encukou reviewed Mar 29, 2024
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

The feature looks good to me. (I haven't looked at the implementation yet though.)

The docs need another pass: change remaining endianness args, ensure all the text is in sync, consider making it clearer that -1 isn't a combinable “flag”.

@zooba

zooba commented Apr 2, 2024

Copy link
Copy Markdown
Member Author

I updated the failing fuzz test output to look more like this:

0:00:00 Run 1 test sequentially
0:00:00 [1/1] test_capi
test test_capi failed -- Traceback (most recent call last):
  File "D:\cpython\Lib\test\test_capi\test_long.py", line 663, in test_long_asnativebytes_fuzz
    self.assertEqual(1, v)
    ~~~~~~~~~~~~~~~~^^^^^^
AssertionError: 1 != 194879951359646889511981556009531064497545[375 chars]50206

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "D:\cpython\Lib\test\test_capi\test_long.py", line 678, in test_long_asnativebytes_fuzz
    raise AssertionError(f"Value: 0x{value_hex}") from ex
AssertionError: Value: 0xB44EA82B9AED740C_917368BEEE924893_F6AC715721FFBD60_F1D8E1457AE009BD_473372C7566AAC3C_B12DB10F31323A43_163CEA7A98C03632_9B4C6D6C737DDAE0_1D991B33E907FAE5_B7734C3A3D49EE6B_8A01109C7FA994E7_CF15BB131B8AA2E5_6E3D4056AA61F565_9F6DD199547C4B72_1CD249CBAD015CAC_5D9AB391DEF40235_6496CC0033280BCA_DAE52FF4DE1DC081_5C43492B2921C5D2_99E4110E8C6969E5_EB220536D71B9C65_4AA79A2B1529FE

test_capi failed (1 failure)

In verbose mode:

test_long_asnativebytes_fuzz (test.test_capi.test_long.LongTests.test_long_asnativebytes_fuzz) ...
52 bytes
hex = 1A768B041B_DFE1F8B17913408F_4A6B4BD1D6C1781B_5D3A78F4627EC370_5BFE4445BD03E6DD_8A72C2EC13593BB8_D8F338A07F90D5
int = 17493562917047520716638432759788459383790970486783581286602317582080085198061893821730624261730678412225580277506146335428821
FAIL

Thoughts? I don't think making it a subTest is helpful, since there's no reasonable way to request the test run with a particular value, or to efficiently request a value be skipped.

encukou
encukou reviewed Apr 4, 2024
View reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Thank you! This looks great; I did find a few nitpicks.

@encukou

encukou commented Apr 4, 2024

Copy link
Copy Markdown
Member

Also, please consider these changes to the docs: zooba#33

@encukou

encukou commented Apr 5, 2024

Copy link
Copy Markdown
Member

Thank you!

@encukou encukou merged commit 6876168 into python:main Apr 5, 2024
@zooba zooba deleted the gh-111140 branch April 5, 2024 14:54
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@zooba @diegorusso
pythongh-111140: PyLong_From/AsNativeBytes: Take *flags* rather than … …
29db213 
…just *endianness* (pythonGH-116053)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead left review comments

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

@zooba @encukou @gpshead