Change type and add _Py_ prefix to COUNT_ALLOCS variables #49100

abalkin · 2009-01-05T20:48:45Z

BPO	4850
Nosy	@malemburg, @loewis, @abalkin
Files	issue4850.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 = None
closed_at = <Date 2009-01-07.18:43:28.070>
created_at = <Date 2009-01-05.20:48:45.338>
labels = ['interpreter-core', 'type-feature']
title = 'Change type and add _Py_ prefix to COUNT_ALLOCS variables'
updated_at = <Date 2009-01-07.18:43:28.069>
user = 'https://github.com/abalkin'

bugs.python.org fields:

activity = <Date 2009-01-07.18:43:28.069>
actor = 'loewis'
assignee = 'none'
closed = True
closed_date = <Date 2009-01-07.18:43:28.070>
closer = 'loewis'
components = ['Interpreter Core']
creation = <Date 2009-01-05.20:48:45.338>
creator = 'belopolsky'
dependencies = []
files = ['12607']
hgrepos = []
issue_num = 4850
keywords = ['patch']
message_count = 16.0
messages = ['79199', '79208', '79231', '79246', '79247', '79253', '79254', '79259', '79263', '79265', '79267', '79271', '79311', '79344', '79359', '79361']
nosy_count = 3.0
nosy_names = ['lemburg', 'loewis', 'belopolsky']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue4850'
versions = []

abalkin · 2009-01-05T20:48:45Z

quick_int_allocs, quick_neg_int_allocs, tuple_zero_allocs, and
fast_tuple_allocs are exported in -DCOUNT_ALLOCS builds. They should get
a conventional _Py_ prefix. Also since tp_allocs is now Py_ssize_t, these
should be redefined as Py_ssize_t as well. See msg79194 .

abalkin · 2009-01-05T23:06:23Z

Attached patch is fairly straightforward with only one caveat: instead
of prefixing unlist_types_without_objects flag with _Py_, I made it
static. This may not be the right thing if it is intended to be
accessible from third party modules (it is not used outside of object.c
in python code). I am not sure how it is supposed to be used because it
appears to be always 0. (Maybe the intent is to change it manually in
specialized builds or in debugger.)

I am not proposing to move declarations to header files with this patch.
That change would cause a massive recompile for everyone and if a
decision is made that globals' declarations belong to header files, this
should probably be done it one sweep with bpo-4805 .

abalkin · 2009-01-06T05:22:09Z

Martin,

Can you comment on whether unlist_types_without_objects should be global?
Svn blame point to you for introducing it.

Thanks.

$ svn blame Objects/object.c | grep  unlist_types_without_objects
 45527 martin.v.loewis int unlist_types_without_objects;

loewis · 2009-01-06T10:28:11Z

Making it static is fine. I don't recall what kind of customization I
did when I introduced it.

I'm skeptical about the entire issue, though. Why is it that these
variables should be prefixed? I disagree that all names need to be
prefixed: those only present in certain debug builds need not.

malemburg · 2009-01-06T10:30:45Z

On 2009-01-06 11:28, Martin v. Löwis wrote:

Martin v. Löwis <martin@v.loewis.de> added the comment:

Making it static is fine. I don't recall what kind of customization I
did when I introduced it.

I'm skeptical about the entire issue, though. Why is it that these
variables should be prefixed? I disagree that all names need to be
prefixed: those only present in certain debug builds need not.

For completeness and to make things easier for all developers,
all exported symbols should be prefixed with _Py or Py - regardless
of how their export is enabled.

loewis · 2009-01-06T12:28:21Z

For completeness

I don't think completeness is a useful thing to have here.

and to make things easier for all developers,

It is not easier, but more difficult. It now requires a change,
whereas leaving things as-is requires no change.

all exported symbols should be prefixed with _Py or Py - regardless
of how their export is enabled.

Ah - but they aren't exported symbols.

malemburg · 2009-01-06T13:01:30Z

On 2009-01-06 13:28, Martin v. Löwis wrote:

Martin v. Löwis <martin@v.loewis.de> added the comment:

> For completeness

I don't think completeness is a useful thing to have here.

> and to make things easier for all developers,

It is not easier, but more difficult. It now requires a change,
whereas leaving things as-is requires no change.

Sure it does: consistent rules always make things easier, since
they avoid doubt.

> all exported symbols should be prefixed with _Py or Py - regardless
> of how their export is enabled.

Ah - but they aren't exported symbols.

I'm only referring to exported symbols. Static globals, of course,
don't need such a prefix.

loewis · 2009-01-06T13:22:03Z

I'm only referring to exported symbols. Static globals, of course,
don't need such a prefix.

Ok, the symbols in question are not exported from pythonxy.dll.

malemburg · 2009-01-06T13:45:44Z

On 2009-01-06 14:22, Martin v. Löwis wrote:

Martin v. Löwis <martin@v.loewis.de> added the comment:

> I'm only referring to exported symbols. Static globals, of course,
> don't need such a prefix.

Ok, the symbols in question are not exported from pythonxy.dll.

Right, because for MS VC you have to explicitly mark symbols for
DLL export which is done via the PyAPI_* macros.

However, they are still exported from the object files, so can cause
name clashes with other libraries you link with.

On Unix, libpythonX.X.a always includes those symbols and they
are also in the global symbol namespace when running the executable
and dynamically loading other libraries.

Even production builds contain a few such symbols which require
the _Py or Py prefix (or need to be made static) - these are for
Python 2.6 and 2.7:

asdl_int_seq_new
asdl_seq_new

Here's the grep line I used for that:

nm libpython2.6.a | egrep ' [TD] ([^P_]|P[^y]|_[^P])' | sort

abalkin · 2009-01-06T14:57:58Z

On Tue, Jan 6, 2009 at 7:28 AM, Martin v. Löwis <report@bugs.python.org> wrote:
..

It is not easier, but more difficult. It now requires a change,
whereas leaving things as-is requires no change.

I actually agree with Martin on this one. I would not touch this if
not for the Py_ssize_t vs. int issue. I am only +0 on renaming: if
type change and format spec changes go in, renaming is cost free.

malemburg · 2009-01-06T15:53:48Z

On 2009-01-06 15:57, Alexander Belopolsky wrote:

Alexander Belopolsky <belopolsky@users.sourceforge.net> added the comment:

On Tue, Jan 6, 2009 at 7:28 AM, Martin v. Löwis <report@bugs.python.org> wrote:
..
> It is not easier, but more difficult. It now requires a change,
> whereas leaving things as-is requires no change.

I actually agree with Martin on this one. I would not touch this if
not for the Py_ssize_t vs. int issue. I am only +0 on renaming: if
type change and format spec changes go in, renaming is cost free.

I don't follow you: those symbols are not meant for public use anyway,
so we can easily change them without any "costs". The same goes for any
of the private, but still exported symbols in the API, ie. all _Py*
APIs.

abalkin · 2009-01-06T16:27:08Z

On Tue, Jan 6, 2009 at 10:53 AM, Marc-Andre Lemburg
<report@bugs.python.org> wrote:
..

I don't follow you: those symbols are not meant for public use anyway,
so we can easily change them without any "costs". The same goes for any
of the private, but still exported symbols in the API, ie. all _Py*
APIs.

This is the same that I said: "renaming is cost free" if it is done
together with the type change. Martin was right that there is cost
associated with any change. The benefit of "completeness" for a
specialized build would not justify even having this discussion IMO.
On the other hand since the type change needs to be done in order to
make the code correct on a 64 bit platform and compile without
warnings with gcc, a patch is needed anyways. Since we are already
paying the cost of change, renaming is free. There is no disagreement
between you and me here.

loewis · 2009-01-07T09:33:51Z

However, they are still exported from the object files,

Ah. Those are "global symbols", not "exported symbols"; "export"
is a concept specific to Win32.

so can cause
name clashes with other libraries you link with.

See, and in this specific case, they can't, because they are used
only in a debug build. Furthermore, they all have names that are
unlikely to collide. Even if they get a _Py_ prefix, there could
still be a conflict.

Even production builds contain a few such symbols which require
the _Py or Py prefix (or need to be made static) - these are for
Python 2.6 and 2.7:

asdl_int_seq_new

asdl_seq_new

No. They don't require the Py_ prefix, because they already
have the asdl_ prefix.

abalkin · 2009-01-07T15:49:54Z

On Wed, Jan 7, 2009 at 4:33 AM, Martin v. Löwis <report@bugs.python.org> wrote:

Martin v. Löwis <martin@v.loewis.de> added the comment:
.. Furthermore, they all have names that are
unlikely to collide. Even if they get a _Py_ prefix, there could
still be a conflict.

My main interest in this patch are the type and %d to %zd change, but
by throwing in the name change, I unintentionally made it more
controversial. Would it help if I resubmit the patch without name
changes or is this something that a committer can take care of if the
patch is partially accepted?

I already stated that I am only +0 on the name change and that "+" is
mostly motivated by the fact that I already made the changes in my
sandbox. Other than that, I don't think it matters. Let's stop
here and refocus the type change.

> Even production builds contain a few such symbols which require
> the _Py or Py prefix (or need to be made static) - these are for
> Python 2.6 and 2.7:
>
> * asdl_int_seq_new
> * asdl_seq_new

No. They don't require the Py_ prefix, because they already
have the asdl_ prefix.

That's true, but asdl_ prefix is not a well-known prefix reserved for
Python symbols. Someone who is debugging linker errors and sees
something like "asdl_seq_new: symbol not found" will be hard pressed
to realize that he/she forgot to link python library or linked an old
version.

Also a grep through nm output that Marc-Andre did is a good check to
run from time to time and there is no reason to have false positives.

malemburg · 2009-01-07T18:32:48Z

On 2009-01-07 10:33, Martin v. Löwis wrote:

Martin v. Löwis <martin@v.loewis.de> added the comment:

> However, they are still exported from the object files,

Ah. Those are "global symbols", not "exported symbols"; "export"
is a concept specific to Win32.

Not at all. .so share libs and .a libs on Unix provide exports
just as well.

> so can cause
> name clashes with other libraries you link with.

See, and in this specific case, they can't, because they are used
only in a debug build. Furthermore, they all have names that are
unlikely to collide. Even if they get a _Py_ prefix, there could
still be a conflict.

> Even production builds contain a few such symbols which require
> the _Py or Py prefix (or need to be made static) - these are for
> Python 2.6 and 2.7:
>
> * asdl_int_seq_new
> * asdl_seq_new

No. They don't require the Py_ prefix, because they already
have the asdl_ prefix.

Oh please. The convention is that *all* Python exported symbols
should start with either _Py or Py. That's an old convention which
was introduced with the Great Renaming in Python 1.4.

The above symbols violate this convention, as do any symbols
that get additionally exported in debug builds or any other
variant build of Python.

I consider it a bug that Python still exports symbols that
do not follow the convention. This makes it harder to tell
whether a symbol originated in Python or some other extension
or linked object/library file.

loewis · 2009-01-07T18:43:07Z

My main interest in this patch are the type and %d to %zd change, but
by throwing in the name change, I unintentionally made it more
controversial. Would it help if I resubmit the patch without name
changes or is this something that a committer can take care of if the
patch is partially accepted?

I think it is indeed better to focus on this part of the patch only.

I just reviewed it, and I think it is incorrect: We cannot assume that
the CRT supports %zd, therefore, PY_FORMAT_SIZE_T has to be used.

I fixed it, and removed the _Py_ prefixing, and committed the result
as r68381.

Also a grep through nm output that Marc-Andre did is a good check to
run from time to time and there is no reason to have false positives.

make smelly

abalkin added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Jan 5, 2009

loewis mannequin closed this as completed Jan 7, 2009

ezio-melotti transferred this issue from another repository Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change type and add _Py_ prefix to COUNT_ALLOCS variables #49100

Change type and add _Py_ prefix to COUNT_ALLOCS variables #49100

abalkin commented Jan 5, 2009

abalkin commented Jan 5, 2009

abalkin commented Jan 5, 2009

abalkin commented Jan 6, 2009

loewis mannequin commented Jan 6, 2009

malemburg commented Jan 6, 2009

loewis mannequin commented Jan 6, 2009

malemburg commented Jan 6, 2009

loewis mannequin commented Jan 6, 2009

malemburg commented Jan 6, 2009

abalkin commented Jan 6, 2009

malemburg commented Jan 6, 2009

abalkin commented Jan 6, 2009

loewis mannequin commented Jan 7, 2009

abalkin commented Jan 7, 2009

malemburg commented Jan 7, 2009

loewis mannequin commented Jan 7, 2009

Navigation Menu

Change type and add _Py_ prefix to COUNT_ALLOCS variables #49100

Change type and add _Py_ prefix to COUNT_ALLOCS variables #49100

Comments

abalkin commented Jan 5, 2009

abalkin commented Jan 5, 2009

abalkin commented Jan 5, 2009

abalkin commented Jan 6, 2009

loewis mannequin commented Jan 6, 2009

malemburg commented Jan 6, 2009

loewis mannequin commented Jan 6, 2009

malemburg commented Jan 6, 2009

loewis mannequin commented Jan 6, 2009

malemburg commented Jan 6, 2009

abalkin commented Jan 6, 2009

malemburg commented Jan 6, 2009

abalkin commented Jan 6, 2009

loewis mannequin commented Jan 7, 2009

abalkin commented Jan 7, 2009

malemburg commented Jan 7, 2009

loewis mannequin commented Jan 7, 2009