Skip to content

Comments

bpo-37732: Fix GCC warning in _PyObject_Malloc()#15333

Merged
vstinner merged 2 commits intopython:masterfrom
vstinner:pymalloc_alloc_warn
Aug 20, 2019
Merged

bpo-37732: Fix GCC warning in _PyObject_Malloc()#15333
vstinner merged 2 commits intopython:masterfrom
vstinner:pymalloc_alloc_warn

Conversation

@vstinner
Copy link
Member

@vstinner vstinner commented Aug 19, 2019 •
edited by bedevere-bot
Loading

pymalloc_alloc() now returns directly the pointer, return NULL on
memory allocation error.

allocate_from_new_pool() already uses NULL as marker for "allocation
failed".

https://bugs.python.org/issue37732

@vstinner
bpo-37732: Fix GCC warning in _PyObject_Malloc()
a0113f0 
pymalloc_alloc() now returns directly the pointer, return NULL on
memory allocation error.

allocate_from_new_pool() already uses NULL as marker for "allocation
failed".
@vstinner vstinner requested a review from methane August 19, 2019 10:27
@vstinner
Copy link
Member Author

vstinner commented Aug 19, 2019 •
edited by bedevere-bot
Loading

PR #15309 proposes a different fix: always initialize ptr to NULL. This change is only needed to make GCC warning quiet, whereas ptr is always initialized.

My change rewrites the code to avoid the warning without adding an useless initialization.

Note: I wrote the pymalloc_alloc(void *ctx, void **ptr_p, size_t nbytes)helper function as part of a larger refactoring change, commit 9ed83c4:

commit 9ed83c40855b57c10988f76770a4eb825e034cd8
Author: Victor Stinner <[email protected]>
Date:   Tue Oct 31 12:18:10 2017 -0700

    [bpo-18835](https://bugs.python.org/issue18835): Cleanup pymalloc (#4200)
    
    Cleanup pymalloc:
    
    * Rename _PyObject_Alloc() to pymalloc_alloc()
    * Rename _PyObject_FreeImpl() to pymalloc_free()
    * Rename _PyObject_Realloc() to pymalloc_realloc()
    * pymalloc_alloc() and pymalloc_realloc() don't fallback on the raw
      allocator anymore, it now must be done by the caller
    * Add "success" and "failed" labels to pymalloc_alloc() and
      pymalloc_free()
    * pymalloc_alloc() and pymalloc_free() don't update
      num_allocated_blocks anymore: it should be done in the caller
    * _PyObject_Calloc() is now responsible to fill the memory block
      allocated by pymalloc with zeros
    * Simplify pymalloc_alloc() prototype
    * _PyObject_Realloc() now calls _PyObject_Malloc() rather than
      calling directly pymalloc_alloc()
    
    _PyMem_DebugRawAlloc() and _PyMem_DebugRawRealloc():
    
    * document the layout of a memory block
    * don't increase the serial number if the allocation failed
    * check for integer overflow before computing the total size
    * add a 'data' variable to make the code easiler to follow
    
    test_setallocators() of _testcapimodule.c now test also the context.

@vstinner vstinner mentioned this pull request Aug 19, 2019
Closed
serhiy-storchaka
serhiy-storchaka approved these changes Aug 19, 2019
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka 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 one check is redundant now.

fusioncid
fusioncid reviewed Aug 20, 2019
View reviewed changes
Objects/obmalloc.c
assert(bp != NULL);

if (UNLIKELY((pool->freeblock = *(block **)bp) == NULL)) {
// Reached the end of the free list, try to extend it.
Copy link
Contributor

@fusioncid fusioncid Aug 20, 2019

Choose a reason for hiding this comment

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

It's better to use comment style "/* */" instead of "//" here，to ensure consistent context.

Suggested change
// Reached the end of the free list, try to extend it.
/* Reached the end of the free list, try to extend it.
*/

Copy link
Member Author

@vstinner vstinner Aug 20, 2019

Choose a reason for hiding this comment

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

This code was likely written before PEP 7 was updated to allow a subset of C99. My PR doesn't add these comments ;-)

@vstinner vstinner merged commit 18f8dcf into python:master Aug 20, 2019
@vstinner vstinner deleted the pymalloc_alloc_warn branch August 20, 2019 11:28
@miss-islington
Copy link
Contributor

miss-islington commented Aug 20, 2019

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

miss-islington commented Aug 20, 2019

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

miss-islington commented Aug 20, 2019

Sorry, @vstinner, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 18f8dcfa10d8a858b152d12a9ad8fa83b7e967f0 3.7

@miss-islington
Copy link
Contributor

miss-islington commented Aug 20, 2019

Sorry @vstinner, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 18f8dcfa10d8a858b152d12a9ad8fa83b7e967f0 3.8

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Aug 20, 2019

👍

@bedevere-bot
Copy link

bedevere-bot commented Aug 20, 2019

GH-15342 is a backport of this pull request to the 3.8 branch.

vstinner added a commit that referenced this pull request Aug 20, 2019
@vstinner
bpo-37732: Fix GCC warning in _PyObject_Malloc() (GH-15333) (GH-15342)
30e5aff 
pymalloc_alloc() now returns directly the pointer, return NULL on
memory allocation error.

allocate_from_new_pool() already uses NULL as marker for "allocation
failed".

(cherry picked from commit 18f8dcf)
vstinner added a commit that referenced this pull request Aug 20, 2019
@vstinner
bpo-37732: Fix GCC warning in _PyObject_Malloc() (GH-15333) (GH-15342) (
c9a484a 
GH-15343)

pymalloc_alloc() now returns directly the pointer, return NULL on
memory allocation error.

allocate_from_new_pool() already uses NULL as marker for "allocation
failed".

(cherry picked from commit 18f8dcf)
(cherry picked from commit 30e5aff)
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@vstinner @lisroach
bpo-37732: Fix GCC warning in _PyObject_Malloc() (pythonGH-15333)
c6a45e8 
pymalloc_alloc() now returns directly the pointer, return NULL on
memory allocation error.

allocate_from_new_pool() already uses NULL as marker for "allocation
failed".
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@vstinner @DinoV
bpo-37732: Fix GCC warning in _PyObject_Malloc() (pythonGH-15333)
e6362ce 
pymalloc_alloc() now returns directly the pointer, return NULL on
memory allocation error.

allocate_from_new_pool() already uses NULL as marker for "allocation
failed".
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
@vstinner @websurfer5
bpo-37732: Fix GCC warning in _PyObject_Malloc() (pythonGH-15333)
d6d53e5 
pymalloc_alloc() now returns directly the pointer, return NULL on
memory allocation error.

allocate_from_new_pool() already uses NULL as marker for "allocation
failed".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@methane methane methane approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@pablogsal pablogsal pablogsal approved these changes

+1 more reviewer

@fusioncid fusioncid fusioncid left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@vstinner vstinner

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@vstinner @miss-islington @serhiy-storchaka @bedevere-bot @methane @pablogsal @fusioncid @the-knights-who-say-ni