classification
Title: Change type and add _Py_ prefix to COUNT_ALLOCS variables
Type: enhancement Stage:
Components: Interpreter Core Versions:
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: Nosy List: belopolsky, lemburg, loewis
Priority: normal Keywords: patch

Created on 2009-01-05 20:48 by belopolsky, last changed 2009-01-07 18:43 by loewis. This issue is now closed.

Files
File name Uploaded Description Edit
issue4850.diff belopolsky, 2009-01-05 23:06
Messages (16)
msg79199 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2009-01-05 20:48
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 .
msg79208 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2009-01-05 23:06
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 issue4805 .
msg79231 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2009-01-06 05:22
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;
msg79246 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-01-06 10:28
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.
msg79247 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2009-01-06 10:30
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.
msg79253 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-01-06 12:28
> 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.
msg79254 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2009-01-06 13:01
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.
msg79259 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-01-06 13:22
> 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.
msg79263 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2009-01-06 13:45
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
msg79265 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2009-01-06 14:57
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.
msg79267 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2009-01-06 15:53
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.
msg79271 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2009-01-06 16:27
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.
msg79311 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-01-07 09:33
> 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.
msg79344 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2009-01-07 15:49
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.
msg79359 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2009-01-07 18:32
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.
msg79361 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2009-01-07 18:43
> 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
History
Date User Action Args
2009-01-07 18:43:28loewissetstatus: open -> closed
resolution: accepted
2009-01-07 18:43:07loewissetmessages: + msg79361
2009-01-07 18:32:48lemburgsetmessages: + msg79359
2009-01-07 15:49:54belopolskysetmessages: + msg79344
2009-01-07 09:33:51loewissetmessages: + msg79311
2009-01-06 16:27:08belopolskysetmessages: + msg79271
2009-01-06 15:53:47lemburgsetmessages: + msg79267
2009-01-06 14:57:58belopolskysetmessages: + msg79265
2009-01-06 13:45:44lemburgsetmessages: + msg79263
2009-01-06 13:22:02loewissetmessages: + msg79259
2009-01-06 13:01:30lemburgsetmessages: + msg79254
2009-01-06 12:28:20loewissetmessages: + msg79253
2009-01-06 10:30:45lemburgsetmessages: + msg79247
2009-01-06 10:28:10loewissetmessages: + msg79246
2009-01-06 05:22:09belopolskysetnosy: + loewis
messages: + msg79231
2009-01-05 23:06:25belopolskysetfiles: + issue4850.diff
nosy: + lemburg
messages: + msg79208
keywords: + patch
2009-01-05 20:48:45belopolskycreate