Re: [Python-Dev] Re: [Bug #121013] Bug in <stringobject>.join(<unicodestring>)
"M.-A. Lemburg" <mal@lemburg.com> writes:
Date: 2000-Nov-27 10:12 By: mwh
Comment: I hope you're all suitably embarrassed - please see patch #102548 for the trivial fix...
Hehe, that was indeed a trivial patch. What was that about trees in a forest...
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!".
I still think that the PySequence_Fast_GETITEM() macro should at least include a fall-back type check which causes some exception in case the used object was not "fixed" using PySequence_Fast() (which I only noticed in string_join() just now).
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:
"M.-A. Lemburg" <mal@lemburg.com> writes:
Date: 2000-Nov-27 10:12 By: mwh
Comment: I hope you're all suitably embarrassed - please see patch #102548 for the trivial fix...
Hehe, that was indeed a trivial patch. What was that about trees in a forest...
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!".
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 ?
I still think that the PySequence_Fast_GETITEM() macro should at least include a fall-back type check which causes some exception in case the used object was not "fixed" using PySequence_Fast() (which I only noticed in string_join() just now).
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.
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...
Fredrik's PySequence_Fast_* APIs look interesting, BTW. Should be used more often :-)
Yes. But they're pretty new, aren't they?
Yep. Fredrik added them quite late in the 2.0 release process.
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).
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 ?
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...
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:
Michael Hudson wrote:
I still think that the PySequence_Fast_GETITEM() macro should at least include a fall-back type check which causes some exception in case the used object was not "fixed" using PySequence_Fast() (which I only noticed in string_join() just now).
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.
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...
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.
Fredrik's PySequence_Fast_* APIs look interesting, BTW. Should be used more often :-)
Yes. But they're pretty new, aren't they?
Yep. Fredrik added them quite late in the 2.0 release process.
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).
I think Fredrik speculated on having the compiler optimizers taking care of the check...
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.
hmm, it would probably also help to somehow declare PyTypeObject slots "const" -- is this possible in a struct definition ?
I have no idea! Cheers, M.
Michael Hudson wrote:
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...
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.
Yes.
[Fredrik's PySequence_Fast_* APIs]
I think Fredrik speculated on having the compiler optimizers taking care of the check...
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.
hmm, it would probably also help to somehow declare PyTypeObject slots "const" -- is this possible in a struct definition ?
I have no idea!
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 wrote:
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...
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
sorry, but that's just silly -- the function is called GET_ITEM, not GetItem. it's no less safe than PyList_GET_ITEM or PyTuple_GET_ITEM or any other "trade speed for safety" macro. </F>
[Michael Hudson]
... [*] 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...
All tests pass for me under current CVS, Windows. More info?
Michael Hudson wrote:
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.
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) ...
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).
yeah, the type isn't likely to change inside the loop... </F>
On Mon, 27 Nov 2000, Fredrik Lundh wrote:
Michael Hudson wrote:
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.
if you can figure out a way to do that with C macros, I'd love to see it...
No. You can consider my above comment as a round-about way of saying "I hate C" :-)
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) ...
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