These are chat archives for dropbox/pyston

14th
Aug 2015
Sun
@Daetalus
Aug 14 2015 02:42
Trying to repoduce that error.
Kevin Modzelewski
@kmod
Aug 14 2015 02:44
debugging through travis-ci is annoying but possible
you could try cutting down the changes to try to isolate the one that causes it
Sun
@Daetalus
Aug 14 2015 02:45
I see. But every time I need to update the PR? Could I do it in my personal repo?
Kevin Modzelewski
@kmod
Aug 14 2015 03:03
yes, though it's a bit of a pain
once the PR is open though, it should be pretty easy to update it
you just have to push -f to the branch you used to create it
Sun
@Daetalus
Aug 14 2015 03:04
ok, I will try it, thanks!
Kevin Modzelewski
@kmod
Aug 14 2015 04:40
I managed to reproduce it :)
I changed test_float to do while True: test_main()
instead of just calling it once
and then it started crashing under pyston_release_gcc
still looking into the actual cause
Kevin Modzelewski
@kmod
Aug 14 2015 05:13
oh man
I think I found it
we tag our gc allocation function with __attribute__((malloc))
which tells gcc that it is free to skip any writes to the memory if it can prove that it won't do any reads to it
so it sees a temporary float object
and realizes it doesn't have to set the cls on it since no one reads it
so it doesn't!
and then when the gc comes around it sees a malformed object
Sun
@Daetalus
Aug 14 2015 05:40
Thanks @kmod , I also locate the position.
https://gist.github.com/Daetalus/b156fcecf7b178e0f648
I believe the backtrace prove your found.
Kevin Modzelewski
@kmod
Aug 14 2015 05:51
ok I think #835 should fix it
Sun
@Daetalus
Aug 14 2015 05:56
Thanks! New knowledge get! About __attribute__((malloc))
Sun
@Daetalus
Aug 14 2015 06:14
Can't wait #835 merge.
It already solved the problem in my machine.
Kevin Modzelewski
@kmod
Aug 14 2015 06:16
ok merged :)
Sun
@Daetalus
Aug 14 2015 06:17
BTW, after narrow the error further, the crash randomly. Could you tell me why? Or why repeatly call it will produce the error, please?
Kevin Modzelewski
@kmod
Aug 14 2015 06:18
well, there's some randomness to it
Vinzenz Feenstra
@vinzenz
Aug 14 2015 06:18
@kmod I just rebased my changes if you find time, please have a look at them
thanks
Kevin Modzelewski
@kmod
Aug 14 2015 06:18
because the test calls random.random, so the heap ends up in a random state
Vinzenz Feenstra
@vinzenz
Aug 14 2015 06:18
ok you did
lol
Kevin Modzelewski
@kmod
Aug 14 2015 06:18
race condition :)
Sun
@Daetalus
Aug 14 2015 06:18
Ok, I will write it done.
^down
Kevin Modzelewski
@kmod
Aug 14 2015 06:19
but yeah these sorts of GC crashes will manifest in weird ways
since it will end up using previous data
Sun
@Daetalus
Aug 14 2015 06:19
After finished it, I will update the #817
Kevin Modzelewski
@kmod
Aug 14 2015 06:19
well, at least in this case, that's what happened
Sun
@Daetalus
Aug 14 2015 06:21
Got it!
What is the cheapest way to find a class has __float__ attribute? cls->getattr(attr)?
Vinzenz Feenstra
@vinzenz
Aug 14 2015 06:24
hasattr, no?
Sun
@Daetalus
Aug 14 2015 06:25
complex constructor not only need to check whether the class has __complex__, but also need to check whether it has __float__.
Kevin Modzelewski
@kmod
Aug 14 2015 06:28
nb_float is probably your best bet
Vinzenz Feenstra
@vinzenz
Aug 14 2015 06:28
@kmod I'd like to ask you also to look at dropbox/pyston#805 if the code is at least ok
Kevin Modzelewski
@kmod
Aug 14 2015 06:28
it should be set whenever a __float__ exists
Sun
@Daetalus
Aug 14 2015 06:28
Ok.
Vinzenz Feenstra
@vinzenz
Aug 14 2015 06:28
the CI failures on that one were not really related to the changes, at least it shouldn't have been
Kevin Modzelewski
@kmod
Aug 14 2015 06:29
as a detail (shouldn't matter for this), it can be faster to call the python attribute than calling nb_float, but if you just want to check if it exists nb_float is better
this is probably rare enough that's it worth just doing exactly what cpython does
@vinzenz ok cool :)
yeah I just wasn't sure if it was ready to review or not
Vinzenz Feenstra
@vinzenz
Aug 14 2015 06:30
=> TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'
on the build
I don't understand this at all
from the customize compiler stuff
Sun
@Daetalus
Aug 14 2015 06:31
Emm, I need to check it first, if exist, call it. Otherwise go to other cases.
Vinzenz Feenstra
@vinzenz
Aug 14 2015 06:34
@kmod what I should do about that assert?
Kevin Modzelewski
@kmod
Aug 14 2015 06:41
yeah I'd say for now just call it like cpython does
was just mentioning as an fyi that there are ways we could make it faster, but I don't think that that is an issue here
@vinzenz to match cpython, it looks like dictMerge should let the AttributeError propagate
ie set null_on_nonexistent back to false
and then the code in rearrangeArguments should catch the attributeerror and raise that typeerror
Vinzenz Feenstra
@vinzenz
Aug 14 2015 06:48
@kmod ok
@kmod but are you sure about the AttributeError? I would expect a type error here
Kevin Modzelewski
@kmod
Aug 14 2015 06:53
this is what CPython has:
            if (PyDict_Update(d, kwdict) != 0) {
                Py_DECREF(d);
                /* PyDict_Update raises attribute
                 * error (percolated from an attempt
                 * to get 'keys' attribute) instead of
                 * a type error if its second argument
                 * is not a mapping.
                 */
                if (PyErr_ExceptionMatches(PyExc_AttributeError)) {
                    PyErr_Format(PyExc_TypeError,
                            "%.200s%.200s argument after ** "
                            "must be a mapping, not %.200s",
                            PyEval_GetFuncName(func),
                            PyEval_GetFuncDesc(func),
                            kwdict->ob_type->tp_name);
                }
                goto ext_call_fail;
            }
Vinzenz Feenstra
@vinzenz
Aug 14 2015 06:53
i see
Ok
I'll check anyway what PyDict_Update does
And again thread_memory_model_test.py FAILED on #832
This test is really flaky
Sun
@Daetalus
Aug 14 2015 07:46
@kmod Would you mind to revie #830 if you have time?
Kevin Modzelewski
@kmod
Aug 14 2015 09:24
is there are reason you uncommented those assertions?
Sun
@Daetalus
Aug 14 2015 09:28
Sorry, to who? You mean the test in test_float?
Kevin Modzelewski
@kmod
Aug 14 2015 09:30
oh, yeah
it seems reasonable, but there's just that cpython comment saying why they have it commented out
it seems fine, was just curious
Sun
@Daetalus
Aug 14 2015 09:33
Sorry, I didn't consider that much. Just enable it and it could pass. So I didn' comment it again.
Maybe I need to comment it again?
Kevin Modzelewski
@kmod
Aug 14 2015 09:38
oh, I think it's fine either way
well, it'd be nice to put a quick note saying that it's a pyston change
Sun
@Daetalus
Aug 14 2015 09:40
Ok, I will do it right now.
Vinzenz Feenstra
@vinzenz
Aug 14 2015 10:44
@kmod btw I found another bug causing an assert. if you call id(1, **{'foo': 1}) it will cause an assert, it does not trigger "TypeError: id() takes no keyword arguments"
I am trying to figure out where the problem is
ok found it
Vinzenz Feenstra
@vinzenz
Aug 14 2015 13:53
awesome, I know where the problem is but I can't find out the right condition to avoid it from crashing and handle the named parameters + kwargs correctly
Sun
@Daetalus
Aug 14 2015 13:54
Does there has anything that I do?
Vinzenz Feenstra
@vinzenz
Aug 14 2015 14:45
no no