GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-1635741: sha256 heap types by koubaa · Pull Request #22134 · python/cpython

Conversation

@koubaa

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue1635741

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@koubaa
koubaa requested a review from tiran as a code owner September 7, 2020 14:46
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Sep 7, 2020
@bedevere-bot bedevere-bot added the awaiting review label Sep 7, 2020
vstinner
vstinner approved these changes Sep 7, 2020
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Sep 7, 2020
@vstinner

vstinner commented Sep 7, 2020

Copy link
Copy Markdown
Member

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>
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Sep 7, 2020

Copy link
Copy Markdown
Member

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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Sep 7, 2020

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@koubaa

koubaa commented Sep 7, 2020

Copy link
Copy Markdown
Contributor Author

@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
koubaa added 2 commits September 7, 2020 13:00
@koubaa
convert to heap type
90a4c35
@koubaa
blurb
57a0629
@koubaa
koubaa force-pushed the bpo-1635741-sha256-heap-types branch from 371b994 to 57a0629 Compare September 7, 2020 18:00
@koubaa

koubaa commented Sep 7, 2020

Copy link
Copy Markdown
Contributor Author

@vstinner @shihai1991 @corona10 please review
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Sep 7, 2020

Copy link
Copy Markdown
Member

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner approved these changes Sep 7, 2020
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/sha256module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/sha256module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Core and Builtins/2020-09-07-09-45-47.bpo-1635741.QuDIut.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@koubaa
minor changes
255724b
@vstinner
vstinner merged commit 52a2df1 into python:master Sep 8, 2020
@bedevere-bot bedevere-bot removed the awaiting merge label 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 @xzy3-cdc
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
Py_Finalize() doesn't clear all Python objects at exit #44470
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@koubaa @vstinner @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.