Remove PyMalloc_* symbols

[nascheme]

BPO	540394
Nosy	@gvanrossum, @tim-one, @nascheme
Files	pymalloc2.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/nascheme'
closed_at = <Date 2002-04-18.02:19:02.000>
created_at = <Date 2002-04-07.01:07:00.000>
labels = ['interpreter-core']
title = 'Remove PyMalloc_* symbols'
updated_at = <Date 2002-04-18.02:19:02.000>
user = 'https://github.com/nascheme'

bugs.python.org fields:

activity = <Date 2002-04-18.02:19:02.000>
actor = 'nascheme'
assignee = 'nascheme'
closed = True
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2002-04-07.01:07:00.000>
creator = 'nascheme'
dependencies = []
files = ['4129']
hgrepos = []
issue_num = 540394
keywords = ['patch']
message_count = 12.0
messages = ['39496', '39497', '39498', '39499', '39500', '39501', '39502', '39503', '39504', '39505', '39506', '39507']
nosy_count = 3.0
nosy_names = ['gvanrossum', 'tim.peters', 'nascheme']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue540394'
versions = []

[nascheme]

This patch removes all PyMalloc_* symbols from the
source. obmalloc now implements PyObject_{Malloc,
Realloc, Free}. PyObject_{New,NewVar} allocate using
pymalloc.

I also changed PyObject_Del and PyObject_GC_Del
so that they be used as function designators. Is
changing the signature of PyObject_Del going to cause
any problems? I had to add some extra typecasts when
assigning to tp_free.

Please review and assign back to me.

The next phase would be to cleanup the memory API
usage. Do we want to replace all PyObject_Del calls
with PyObject_Free? PyObject_Del seems to match better
with PyObject_GC_Del.

Oh yes, we also need to change PyMem_{Free, Del, ...} to
use pymalloc's free.

[tim-one]

Logged In: YES
user_id=31435

Looks good to me -- thanks!

[tim-one]

Logged In: YES
user_id=31435

Oops -- I hit "Submit" prematurely. More to come.

[tim-one]

Logged In: YES
user_id=31435

Extensions that *currently* call PyObject_Del have its old
macro expansion ("_PyObject_Del((PyObject *)(op))") buried
in them, so getting rid of _PyObject_Del is a binary-API
incompatibility (existing extensions will no longer link
without recompilation).

I personally don't mind that, but I run on Windows
and "binary compatability" never works there across minor
releases for other reasons, so I don't have any real feel
for how much people on other platforms value it. As you
pointed out recently too, binary compatability has, in
reality, not been the case since 1.5.2 anyway.

So that's one for Python-Dev. If we do break binary
compatibility, I'd be sorely tempted to change
the "destructor" typedef to say destructors take void*.
IMO saying they take PyObject* was a poor idea, as you
almost never have a PyObject* when calling one of these
guys. That's why PyObject_Del "had to" be a macro, to hide
the cast to PyObject* almost everyone needs because of
destructor's "correct" but impractical signature.
If "destructor" had a practical signature, there would have
been no temptation to use a macro.

Note that if the typedef of destructor were so changed, you
wouldn't have needed new casts in tp_free slots. And I'd
rather break binary compatability than make extension
authors add new casts.

Hmm. I'm assigning this to Guido for comment: Guido, what
are your feelings about binary compatibility here? C
didn't define free() as taking a void* by mistake <wink>.

Back to Neil: I wouldn't bother changing PyObject_Del to
PyObject_Free. The former isn't in the "recommended"
minimal API, but neither is it discouraged. I expect
TMTOWTDI here forever.

[gvanrossum]

Logged In: YES
user_id=6380

I'm looking at this now...

[gvanrossum]

Logged In: YES
user_id=6380

(Wouldn't it be more efficient to take this to email
between the three of us?)

Extensions that *currently* call PyObject_Del have
its old macro expansion ("_PyObject_Del((PyObject
*)(op))") buried in them, so getting rid of
_PyObject_Del is a binary-API incompatibility
(existing extensions will no longer link without
recompilation). I personally don't mind that, but
I run on Windows and "binary compatability" never
works there across minor releases for other
reasons, so I don't have any real feel for how
much people on other platforms value it. As you
pointed out recently too, binary compatability
has, in reality, not been the case since 1.5.2
anyway.

Still, tradition has it that we keep such entry
points around for a long time. I propose that we do
so now, too.

So that's one for Python-Dev. If we do break
binary compatibility, I'd be sorely tempted to
change the "destructor" typedef to say destructors
take void*. IMO saying they take PyObject* was a
poor idea, as you almost never have a PyObject*
when calling one of these guys.

Huh? "destructor" is used to declare tp_dealloc,
which definitely needs a PyObject * (or some
"subclass" of it, like PyIntObject *).

It's also used to declare tp_free, which arguably
shouldn't take a PyObject * (since by the time
tp_free is called, most of the object's contents
have been destroyed by tp_dealloc). So maybe
tp_free (a newcomer in 2.2) should be declared to
take something else, but then the risk is breaking
code that defines a tp_free with the correct
signature.

That's why PyObject_Del "had to" be a macro, to
hide the cast to PyObject* almost everyone needs
because of destructor's "correct" but impractical
signature. If "destructor" had a practical
signature, there would have been no temptation to
use a macro.

I don't understand this at all.

Note that if the typedef of destructor were so
changed, you wouldn't have needed new casts in
tp_free slots. And I'd rather break binary
compatability than make extension authors add new
casts.

Nor this.

Hmm. I'm assigning this to Guido for comment:
Guido, what are your feelings about binary
compatibility here? C didn't define free() as
taking a void* by mistake <wink>.

I want binary compatibility, but I don't understand
your comments very well.

Back to Neil: I wouldn't bother changing PyObject_Del
to PyObject_Free. The former isn't in the
"recommended" minimal API, but neither is it
discouraged. I expect TMTOWTDI here forever.

I prefer PyObject_Del -- like PyObject_GC_Del, and
like we did in the past. Plus, I like New to match
Del and Malloc to match Free. Since it's
PyObject_New, it should be _Del.

I'm not sure what to say of Neil's patch, except
that I'm glad to be rid of the PyMalloc_XXX family.
I wish we didn't have to change all the places that
used to say _PyObject_Del. Maybe it's best to keep
that name around? The patch would (psychologically)
become a lot smaller. I almost wish that this would
work:

#define PyObject_Del  ((destructor)PyObject_Free)

Or maybe it *does* work???

[gvanrossum]

Logged In: YES
user_id=6380

I've not fully read Tim's response in email, but instead
I've reviewed and discussed the patch with Tim.

I think the only thing to which I object at this point is
the removal of the entry point _PyObject_Del. I believe
that for source and binary compatibility with 2.2, that
entry point should remain, with the same meaning, but it
should not be used at all by the core. (Motivation to keep
it: it's the only thing you can reasonably stick in tp_free
that works for 2.2 as well as for 2.3.)

One minor question: there are a bunch of #undefs in
gcmodule.c (e.g. PyObject_GC_Track) that don't seem to make
sense -- at least I cannot find where these would be
#defined any more. Ditto for #indef PyObject_Malloc in
obmalloc.c.

I suggest that you check this thing in, but keeping
_PyObject_Del alive, and we'll take it from there.

[tim-one]

Logged In: YES
user_id=31435

Clarifying or just repeating Guido here:

+ Binary compatibility is important. It's better on Unix
than it appears <wink> -- while you'll get a warning if you
run an old 1.5.2 extension with 2.2 today and without
recompiling, it will almost certainly work anyway. So in
the case of macros that expanded to a private API function
before, that private API function must still exist, but the
macro needn't expand to that anymore (nor even *be* a macro
anymore). _PyObject_Del is a particular problem cuz it's
even documented in the C API manual -- there simply wasn't
a public API function before that did the same thing and
could be used as a function designator. You're making life
better for future generations.

+ Casts on tp_free slots are par for the course,
because "destructor" has an impractical signature. I'm
afraid that can't change either, so the casts stay.

+ Fred and I agreed to add PyObject_Del to the "minimal
recommended API", so, for the next round of this, feel
wholly righteous in leaving existing PyObject_Del calls
alone.

If anything's unclear, hit me.

[nascheme]

Logged In: YES
user_id=35752

It might be a day or two before I get to this.

Regarding the type of tp_free, could we change it to be
something like:

  typedef void (*freefunc)(void *);
  ...
  freefunc tp_free;

and leave the type of tp_dealloc alone. Maybe it's too
late now that 2.2 is out and uses 'destructor'. I don't
see how this relates to binary compatibility though.
Why does it matter if the function takes a PyObject pointer
or a void pointer? The worse I see happening is that people
could get warnings when they compile their extension
modules.

[tim-one]

Logged In: YES
user_id=31435

It'll be a day or two before PLabs can get back to Python
work anyway.

Reassigning to Guido -- I'm not even going to try to
channel him on backwards compatibility, or the feasibility
of introducing possible warnings. If I were you I'd check
in the patch with the casts in; they can be taken out again
later if Guido is agreeable.

[gvanrossum]

Logged In: YES
user_id=6380

The binary compatibility issue is extensions compiled for
2.2 that have references to _PyObject_Del compiled into them
and aren't recompiled for 2.3. I think that should work
(even if they get a warning). To make it work, the
_PyObject_Del entry point must continue to exist.

Back to Neil, I think my instructions are clear enough.

[nascheme]

Logged In: YES
user_id=35752

A modified version of the patch has been commited.