Re: [Python-Dev] Re: [Bug #121013] Bug in <stringobject>.join(<unicodestring>)
"M.-A. Lemburg" <mal@lemburg.com> writes:
The way I found it was perhaps instructive. I was looking at the function, and thought "that's a bit complicated" so I rewrote it (My rewrite also seems to be bit quicker so I'll upload it as soon as make test has finished[*]). In the course of rewriting it, I saw the line my patch touched and went "duh!".
It's hard to see how; you're not going to check each invocation of PySequence_Fast_GETITEM for a NULL return, are you? It's possible that PySequence_Fast should raise an exception on being passed a string or Unicode object... but probably not.
Fredrik's PySequence_Fast_* APIs look interesting, BTW. Should be used more often :-)
Yes. But they're pretty new, aren't they? I find them a bit unsatisfactory that it's not possible to hoist the type check out of the inner loop. Still, it seems my PII's branch predictor nails that one... (i.e. changing it so that it didn't check inside the loop made naff-all difference to the running time). Cheers, M. [*] which reminds me: test_unicodedata is failing for me at the moment. Anyone else seeing this? It looks like a python regrtest.py -g is all that's needed... -- 7. It is easier to write an incorrect program than understand a correct one. -- Alan Perlis, http://www.cs.yale.edu/homes/perlis-alan/quotes.html
Michael Hudson wrote:
Yeah. The bug must have sneaked in there when the function was updated to the PySequence_Fast_* implementation. BTW, could you also add a patch for the test_string.py and test_unicode.py tests ?
Since not using PySequence_Fast() to initialize the protocol, I'd suggest doing a Py_FatalError() with some explanatory text which gets printed to stderr -- still better than a segfault at some later point due to some dangling pointers...
Yep. Fredrik added them quite late in the 2.0 release process.
I think Fredrik speculated on having the compiler optimizers taking care of the check... hmm, it would probably also help to somehow declare PyTypeObject slots "const" -- is this possible in a struct definition ?
Is that for the CVS version or the release version ? -- Marc-Andre Lemburg ______________________________________________________________________ Company: http://www.egenix.com/ Consulting: http://www.lemburg.com/ Python Pages: http://www.lemburg.com/python/
On Mon, 27 Nov 2000, M.-A. Lemburg wrote:
So you'd want PySequence_Fast_GETITEM to look like #define PySequence_Fast_GETITEM(s,i) \ (PyList_Check((s)) ? PyList_GET_ITEM((s),(i)) : \ (PyTuple_Check((s))? PyTuple_GET_ITEM((s),(i)) : \ Py_FatalError("muffin!") )) ? That'd probably be fair - you'd want to check any performance hit, but I wouldn't be surprised if it was neglible.
gcc 2.95.1 doesn't seem to - even with -O9 you get stuff like .stabn 68,0,825,.LM312-string_join .LM312: movl PyList_Type@GOT(%ebx),%eax cmpl %eax,4(%edi) jne .L875 movl 12(%edi),%eax jmp .L898 .p2align 4,,7 .L875: leal 12(%edi),%eax .L898: movl -32(%ebp),%edx movl (%eax,%edx,4),%esi ... but I'm no expert in assembly reading.
I have no idea! Cheers, M.
Michael Hudson wrote:
Yes.
Me neither -- it could help the compiler when moving type checks out of inner loops: the type slot of PyObject normally doesn't change (except on Windows where you have to patch it in order make VC++ happy). Anyone got a working idea in this direction ? -- Marc-Andre Lemburg ______________________________________________________________________ Company: http://www.egenix.com/ Consulting: http://www.lemburg.com/ Python Pages: http://www.lemburg.com/python/
Michael Hudson wrote:
if you can figure out a way to do that with C macros, I'd love to see it... on the other hand, PySequence_Fast guarantees that the returned object is either a list or a tuple, so you can easily move the test outside the loop yourself: if (PyTuple_Check(seq)) for (...) item = PyTuple_GET_ITEM(seq, i) ... else for (...) item = PyList_GET_ITEM(seq, i) ...
yeah, the type isn't likely to change inside the loop... </F>
On Mon, 27 Nov 2000, Fredrik Lundh wrote:
No. You can consider my above comment as a round-about way of saying "I hate C" :-)
But doing that would slightly eliminate the point of the PySequenceFast* API wouldn't it? Rhetorical question; it doesn't really seem to matter. Slightly less rhetorical question: string_join currently just assumes a list of strings. But it is conceivable that one day it might be changed to call __str__ on instances it finds, and then you'd have the issue of __str__ methods changing the length of the list you're iterating over (this might more plausibly apply in other situations where you might want to use PySequence_Fast but I haven't gone looking for them). So would it be feasible to change PySequence_Fast to smash the type of a list it's given with the "immutable_list_type" from listobject.c and then have a PySequence_Fast_End function that would decref the sequence and un-smash the type as appropriate? Cheers, M.
Michael Hudson wrote:
Yeah. The bug must have sneaked in there when the function was updated to the PySequence_Fast_* implementation. BTW, could you also add a patch for the test_string.py and test_unicode.py tests ?
Since not using PySequence_Fast() to initialize the protocol, I'd suggest doing a Py_FatalError() with some explanatory text which gets printed to stderr -- still better than a segfault at some later point due to some dangling pointers...
Yep. Fredrik added them quite late in the 2.0 release process.
I think Fredrik speculated on having the compiler optimizers taking care of the check... hmm, it would probably also help to somehow declare PyTypeObject slots "const" -- is this possible in a struct definition ?
Is that for the CVS version or the release version ? -- Marc-Andre Lemburg ______________________________________________________________________ Company: http://www.egenix.com/ Consulting: http://www.lemburg.com/ Python Pages: http://www.lemburg.com/python/
On Mon, 27 Nov 2000, M.-A. Lemburg wrote:
So you'd want PySequence_Fast_GETITEM to look like #define PySequence_Fast_GETITEM(s,i) \ (PyList_Check((s)) ? PyList_GET_ITEM((s),(i)) : \ (PyTuple_Check((s))? PyTuple_GET_ITEM((s),(i)) : \ Py_FatalError("muffin!") )) ? That'd probably be fair - you'd want to check any performance hit, but I wouldn't be surprised if it was neglible.
gcc 2.95.1 doesn't seem to - even with -O9 you get stuff like .stabn 68,0,825,.LM312-string_join .LM312: movl PyList_Type@GOT(%ebx),%eax cmpl %eax,4(%edi) jne .L875 movl 12(%edi),%eax jmp .L898 .p2align 4,,7 .L875: leal 12(%edi),%eax .L898: movl -32(%ebp),%edx movl (%eax,%edx,4),%esi ... but I'm no expert in assembly reading.
I have no idea! Cheers, M.
Michael Hudson wrote:
Yes.
Me neither -- it could help the compiler when moving type checks out of inner loops: the type slot of PyObject normally doesn't change (except on Windows where you have to patch it in order make VC++ happy). Anyone got a working idea in this direction ? -- Marc-Andre Lemburg ______________________________________________________________________ Company: http://www.egenix.com/ Consulting: http://www.lemburg.com/ Python Pages: http://www.lemburg.com/python/
Michael Hudson wrote:
if you can figure out a way to do that with C macros, I'd love to see it... on the other hand, PySequence_Fast guarantees that the returned object is either a list or a tuple, so you can easily move the test outside the loop yourself: if (PyTuple_Check(seq)) for (...) item = PyTuple_GET_ITEM(seq, i) ... else for (...) item = PyList_GET_ITEM(seq, i) ...
yeah, the type isn't likely to change inside the loop... </F>
On Mon, 27 Nov 2000, Fredrik Lundh wrote:
No. You can consider my above comment as a round-about way of saying "I hate C" :-)
But doing that would slightly eliminate the point of the PySequenceFast* API wouldn't it? Rhetorical question; it doesn't really seem to matter. Slightly less rhetorical question: string_join currently just assumes a list of strings. But it is conceivable that one day it might be changed to call __str__ on instances it finds, and then you'd have the issue of __str__ methods changing the length of the list you're iterating over (this might more plausibly apply in other situations where you might want to use PySequence_Fast but I haven't gone looking for them). So would it be feasible to change PySequence_Fast to smash the type of a list it's given with the "immutable_list_type" from listobject.c and then have a PySequence_Fast_End function that would decref the sequence and un-smash the type as appropriate? Cheers, M.
participants (5)
-
Fredrik Lundh -
Fredrik Lundh -
M.-A. Lemburg -
Michael Hudson -
Tim Peters