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.

Sign up for GitHub

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

Jump to bottom

bpo-1635741: sha256 heap types #22134

Merged
merged 3 commits into from
Sep 8, 2020
Merged

bpo-1635741: sha256 heap types #22134

merged 3 commits into from
Sep 8, 2020

Conversation

koubaa
Copy link
Contributor

@koubaa koubaa commented Sep 7, 2020 •
edited by bedevere-bot
Loading

@koubaa koubaa requested a review from tiran as a code owner September 7, 2020 14:46
vstinner
vstinner approved these changes Sep 7, 2020
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM at the first iteration, good job :-) Let's see if the CI agrees with me.

@vstinner
Copy link
Member

vstinner commented Sep 7, 2020

CI is unhappy:

Debug memory block at address p=0x7f408d1944a0: API 'm'
    0 bytes originally requested
    The 7 pad bytes at p-7 are FORBIDDENBYTE, as expected.
    The 8 pad bytes at tail=0x7f408d1944a0 are not all FORBIDDENBYTE (0xfd):
        at tail+0: 0x00 *** OUCH
        at tail+1: 0x00 *** OUCH
        at tail+2: 0x00 *** OUCH
        at tail+3: 0x00 *** OUCH
        at tail+4: 0x00 *** OUCH
        at tail+5: 0x00 *** OUCH
        at tail+6: 0x00 *** OUCH
        at tail+7: 0x00 *** OUCH

Enable tracemalloc to get the memory block allocation traceback

Fatal Python error: _PyMem_DebugRawFree: bad trailing pad byte
Python runtime state: finalizing (tstate=0x55e690e76d60)

Current thread 0x00007f408e7e0080 (most recent call first):
<no Python frame>

@vstinner
Copy link
Member

vstinner commented Sep 7, 2020

Please try to build Python and run the test suite before pushing a change, run at least the modified test (test_hashlib in your case, if I followed correctly).

@vstinner
Copy link
Member

vstinner commented Sep 7, 2020

Note: To develop, it's better to build Python in debug mode.

@koubaa
Copy link
Contributor Author

koubaa commented Sep 7, 2020

@vstinner I did build locally and there were errors, so I pushed to see if it was windows-specific. I haven't yet requested a review.

What is the proper etiquette for this in CPython, should I have added [DRAFT] or [DONTREVIEW] to the title?

@koubaa koubaa force-pushed the bpo-1635741-sha256-heap-types branch from 371b994 to 57a0629 Compare September 7, 2020 18:00
@koubaa
Copy link
Contributor Author

koubaa commented Sep 7, 2020

@vstinner @shihai1991 @corona10 please review

@vstinner
Copy link
Member

vstinner commented Sep 7, 2020

@vstinner I did build locally and there were errors, so I pushed to see if it was windows-specific.

If a test fails on Windows, a PR cannot be merged, since tests must pass on Linux and Windows.

In general, the advice is not publish a PR before it works as expected. It's rare that people propose incomplete/non-working PRs.

vstinner
vstinner approved these changes Sep 7, 2020
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM, but please address the minor comments.

Modules/sha256module.c Outdated Show resolved Hide resolved
Modules/sha256module.c Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Core and Builtins/2020-09-07-09-45-47.bpo-1635741.QuDIut.rst Outdated Show resolved Hide resolved
@vstinner vstinner merged commit 52a2df1 into python:master Sep 8, 2020
shihai1991 added a commit to shihai1991/cpython that referenced this pull request Sep 10, 2020
@shihai1991
Merge remote-tracking branch 'origin/master' into bpo_1635741_1
00b300c 
* origin/master: (1373 commits)
  bpo-1635741: Port mashal module to multi-phase init (python#22149)
  bpo-1635741: Port _string module to multi-phase init (pythonGH-22148)
  bpo-1635741: Convert _sha256 types to heap types (pythonGH-22134)
  bpo-1635741: Port the termios to multi-phase init (PEP 489) (pythonGH-22139)
  bpo-41732: add iterator to memoryview (pythonGH-22119)
  bpo-40744: Drop support for SQLite pre 3.7.3 (pythonGH-20909)
  bpo-41316: Make tarfile follow specs for FNAME (pythonGH-21511)
  bpo-41720: Add "return NotImplemented" in turtle.Vec2D.__rmul__(). (pythonGH-22092)
  bpo-1635741 port _curses_panel to multi-phase init (PEP 489) (pythonGH-21986)
  bpo-1635741: Port _overlapped module to multi-phase init (pythonGH-22051)
  bpo-1635741: Port _opcode module to multi-phase init (PEP 489) (pythonGH-22050)
  bpo-1635741 port zlib module to multi-phase init (pythonGH-21995)
  [doc] Add link to Generic in typing (pythonGH-22125)
  bpo-41513: Expand comments and add references for a better understanding (pythonGH-22123)
  bpo-1635741: Port _sha1, _sha512, _md5 to multiphase init (pythonGH-21818)
  closes bpo-41723: Fix an error in the py_compile documentation. (pythonGH-22110)
  [doc] Fix padding in some typing definitions (pythonGH-22114)
  Fix documented Python version for venv --upgrade-deps (pythonGH-22113)
  bpo-40318: Migrate to SQLite3 trace v2 API (pythonGH-19581)
  bpo-41687: Fix sendfile implementation to work with Solaris (python#22040)
  ...
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
@koubaa
bpo-1635741: Convert _sha256 types to heap types (pythonGH-22134)
729a5f2 
Convert the _sha256 extension module types to heap types.
@kylotan kylotan mannequin mentioned this pull request Sep 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@tiran tiran Awaiting requested review from tiran

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@koubaa @vstinner @the-knights-who-say-ni @bedevere-bot