compact dict : SystemError: returned NULL without setting an error. #72227
Comments
|
I've been able to reliably trigger dict.pop to raise a system error: I have issues getting a small test-case that triggers it, though by changing the pop method of dict to print the returned value: PyObject *
_PyDict_Pop(PyDictObject *mp, PyObject *key, PyObject *deflt){
...
printf("\nreturn End value %x\n", old_value );
return old_value;} I've been able to see that the last return sometime return Null: [snip] return End value 0
Fatal Python error: a function returned NULL without setting an error
SystemError: <built-in method pop of dict object at 0x10473f238> returned NULL without setting an errorCurrent thread 0x00007fff77139300 (most recent call first): Aborted Which I suppose is not desirable. I'm quite uncomfortable with C so I'm far from being able to propose a patch or describe why this would happen... Victor Stinner seem to have made the last changes to these methods in http://bugs.python.org/issue27350 . Not sure if the etiquette is to add them to the nosy list in this case. Discovered because of nightly continuous integration with travis on github.com/xonsh/xonsh |
Carreau
mannequin
added
3.7 (EOL)
|
Oh, also I've bisect it to the compact-dict commit, Reliable same location on travis-ci and locally, but travis-ci location and local differ on where it triggers this. |
|
I've forgot to convert split table into combined table when del and .pop(). |
|
Are you sure INADA? The previous dict implementation has the same constraint but does merge in dict_pop. |
|
does should be does not. Sorry. |
Yes. New dict implementation preserves insertion order. class A:
...a, b = A(), A() assert list(a.__dict__) == ["a", "c", "b"] This is difficult for key-sharing dict (aka. split table). We may be able to allow some simple del and pop operation for split table. |
No worries, that's what nightly are for right ? And I only dug up a failing tests :-) Thanks to you for writing this !
I'm not going to pretend I understand the details, though I applied the latest patch: And it does fix this issue for me (at least on my local machine) Looking at the code the only thing I see is that you seem to return NULL in Pop and -1 in Clear. Not sure if this what you meant, I'm un familar with all this; but thanks a lot again for the your work and the quick response. |
|
I would really appreciate if someone can write an unit test. I'm not confident to fix a bug without unit test, on such tricky part of Python internals (splitted/combined dict). |
|
Added test which reproduce the issue on current master. |
|
New changeset 4a5b61b0d090 by Victor Stinner in branch 'default': |
Oh, great :-) Thanks for the quick fix AND test. I pushed your latest change. |
|
I'd like to reopen this one since I still don't think this change is needed and suggest to revert since this change make split table combined on deletion. Deletion will not alter split dict's order. Only insertion after deletion will. But this case is already handled in insertdict[0]. So I think we don't have to combine once an item is deleted from split dict. The example INADA gives works well with the previous implementation(before this change). The problem there is a SystemError I think is dict.pop() doesn't handle pending state (del dict does, so you can produce the same failure when try del dict[]). We only add poc.patch reverts the change (preserve the tests, eliminating the size compare part) and add pending state handling in dict.pop(). It passes. And if you remove the pending state handling, you can product the SystemError. I'd like to know if my idea is totally wrong. :) [0] https://hg.python.org/cpython/file/tip/Objects/dictobject.c#l1057 |
|
Ohh, mistakes.
|
|
Xiang: I'll describe why it's harder than you think. Current implementation allow insertion to split table only when: /* When insertion order is different from shared key, we can't share
* the key anymore. Convert this instance to combine table.
*/
if (_PyDict_HasSplitTable(mp) &&
((ix >= 0 && *value_addr == NULL && mp->ma_used != ix) ||
(ix == DKIX_EMPTY && mp->ma_used != mp->ma_keys->dk_nentries))) {
if (insertion_resize(mp) < 0) {
For example: a, b = C() del b.a # mp->ma_used = 2 b.b = 42 # It's OK because mp->ma_used == ix == 1. Keep sharing.assert(list(b.__dict__.keys()) == ["c", "b"]) # AssertionError! Actual order is [(PENDING "a",) "b", "c"] |
So what if we delete mp->ma_used == ix or use mp->ma_keys->dk_nentries == ix? Do we still have any case breaking the order? |
|
Xiang: I will remove the now useless check, but after beta 1. This issue is about SystemError and you now want to reuse the issue to |
|
The Blaze test suite segfaults with 4a5b61b0d090. |
What is the Blaze test suite? Are you sure that you really recompiled Python from scratch? Try: make distclean && ./configure --with-pydebug && make |
|
Yes, I'm sure. I even cloned a fresh repo. Also, I tested in |
|
The immediate question is: is this serious enough to block 3.6.0b1 or can it wait for b2? The b1 bits are just about ready to be published. |
|
I'm not sure. In the Blaze test suite, 1e7b636b6009 has the SystemError. Blaze is pushing Python's dynamic capabilities to absolute limits, For Blaze *itself* there is no difference whether this is fixed now |
|
OK, thanks, Stefan! OK with you, Victor? |
|
If Victor can't reply now (it's getting late in Europe), I'd just release. Pretend that I set it to deferred blocker. :) |
|
Yes, he's had a *long* day. I'll take your advice, Stefan. Thanks. |
|
I didn't see any segfault on the Python test suite on buildbots. It's |
|
It could still be a stack overflow, but on the surface it does ==3442== Invalid read of size 8 |
Yes. class C:
...a, b = C() # b has [a, b] pending slot too, and no guard when inserting pending slot. assert list(b.__dict__.keys()) == ['b', 'a']) # AssertionError! --- a, b = C() # Since ma_keys is shared, b's dk_nentries == 2
b.a = 3 # ix = 0, dk_nentries = 2; stop using sharing keys.To keep using sharing key, my idea is adding one more member to dict: mp->ma_values_end. When inserting to pending or empty slot, check that a, b = C() # Both of ma_values_end = 0 But testing such an implementation detail is hard from pure Python. (No API for checking ma_values_end, I don't know adding such hack is worth enough. I think new OrderedDict implementation based on new dict implementation is more worth. |
No, there is no error. Once an pending slot is encountered it is combined. But inserting pending slots is prohibited and it's far from ideal. We need to know if the pending slot is the last entry or not. But it seems we can't achieve it using the current implementation. Sad. :( In short, I give up. Sorry for the noise. |
|
New changeset 579141d6e353 by Victor Stinner in branch 'default': |
|
Stefan Krah: "It could still be a stack overflow, but on the surface it does not look like one. It's definitely related to the aforementioned The initial bug reported in this issue is now fixed. Even if the Blaze issue is related, I would really prefer to discuss it in a different issue, so I created bpo-28120: "The Blaze test suite segfaults with 4a5b61b0d090". @xiang: I simplified find_empty_slot() to remove code to support split table. Again, if you want to support deletion in split table, please open a new issue. I now close this issue. Thanks for the report Matthias! Thanks for the quick fix and the unit test Naoki! |
|
I don't understand how the original issue is fixed if 1e7b636b6009 Sure, it can be a third party issue, but this constellation Happy to discuss it in another issue though. |
|
Stefan Krah: "I don't understand how the original issue is fixed if 1e7b636b6009 "SystemError: returned NULL without setting an error" is a generic error, basically it says that a C function has bug :-) It's hard to know exactly which C function has the issue. "and the very next revision 4a5b61b0d090 (the fix) segfaults" A segfault is not a SystemError: it's a new bug. "Sure, it can be a third party issue, but this constellation *does* seem pretty strange." In case of doubt, I prefer to open a new issue. Please let's continue in the issue bpo-28120 (see my questions there!). |
|
New changeset 3c7456e28777 by Victor Stinner in branch '3.6': |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: