These are chat archives for dropbox/pyston

12th
May 2016
Marius Wachtler
@undingen
May 12 2016 10:22
so I run into a problematic piece of code in M2Crypto.
on of there capi functions sets an error and than returns None.
In BoxedCApiFunction::tppCall we don't look at the return value but will always throw a c++ exception if the capi error is set. But in the rewritter path we only raise the exception when the c function returned NULL
should I change to code to always call checkAndThrowCAPIException()?
Kevin Modzelewski
@kmod
May 12 2016 15:19
would it work to change the non-rewritten path to check the return value?
I think that might be closer to what cpython does
we might need to disable the "assert(!PyErr_Occurred())" checks
or maybe we could make them "assert(!PyErr_Occurred() || thirdPartModuleLoaded())" like for the refcounting check
Marius Wachtler
@undingen
May 12 2016 15:26
throwing the exception only when the the return value is 0 would make us fail one of the M2Crypto tests because it expects the error to get raised. (and in debug build we run into some assert which make sure we don't overwrite a already set exception)
That's why I asked. I guess always checking for the return value and patching M2Crypto to not return None is the better fix
but I'm not sure
Marius Wachtler
@undingen
May 12 2016 15:38
I will check where cpython does the check
Kevin Modzelewski
@kmod
May 12 2016 15:54
well I guess with cpython, they can continue doing things even after an exception gets "raised"
so it might not end up getting checked until getting back to ceval or something
I think checking the return value is the "right" way to do it that extensions should be using
but anyway, the rewritten path and the normal path should definitely do the same thing, whatever it is :)
Sun
@Daetalus
May 12 2016 16:04
Hi, how to disable the JIT?
Kevin Modzelewski
@kmod
May 12 2016 16:06
turn off ENABLE_OSR and ENABLE_REOPT in src/core/options.cpp
Sun
@Daetalus
May 12 2016 16:06
Thank you! And I think dropbox/pyston#1161 is related with the topic you discussed?
Marius Wachtler
@undingen
May 12 2016 16:14
so I stepped though what cpyhon is doing: it ignores that the error gets set and later on the python code actually checks if the function returned nonzero and if not raises an exception which ends up overwriting the already set exception.
so this means we should probably also just check the return value
like you said
Marius Wachtler
@undingen
May 12 2016 16:33
ok so if we clear the existing exception in raise3 with PyErr_Clear() the test passes for us too (and comment out some of our !PyErr_Occured() checks...) without clearing it first it fails
Nathaniel J. Smith
@njsmith
May 12 2016 19:52
IME setting an error + failing to return the proper error flag leads to all kinds of bizarro behavior from CPython -- usually the "ghost" exception ends up getting raised a few bytecode later with the wrong traceback so debugging is obnoxious. As an extension author I am perfectly happy if you don't follow them in detail, and actually happier if you make this an immediate obvious assertion failure.
Marius Wachtler
@undingen
May 12 2016 21:37
I agree, but currently I think we are too strict with some of this asserts because they break some useful uses of the capi. So we will have to figure out at some point what the correct trade of is.
pycrypto and cffi run into a problem that Py_REF_DEBUG is not defined in a release build but is defined when building the extensions
Kevin Modzelewski
@kmod
May 12 2016 21:41
one thing that I ran into, which might be the same thing, is that we don't use different suffixes for debug and release builds
ie .pyston.so
so if you run the test in debug mode, and then run it again in release mode, only the first run will build the extensions since the second run will see them as up to date
but then they got compiled with a different set of flags
I think this is the same behavior we had on master (we would share extension module builds between debug and release mode), but the difference is that I added something that changes the cflags for extensions in the two builds
so it used to not be an issue that we used them interchangably
Marius Wachtler
@undingen
May 12 2016 21:45
I have seen this problem too.
But I think the ones for this tests is different. I guess they just don't have NDEBUG defined when they get build and thats why they will always get build with the additional refcounting checks.
For example when I looked at one of the objects files which cffi freshly generated during the execution of a small example it contained references to the debug variables. So I'm not sure if it's just a case of we have to change some config file to tell what defines it has to set when building an extension. Or if we have to solve it by setting the options more explicit in the pyconfig.h etc...
Kevin Modzelewski
@kmod
May 12 2016 21:49
oh, interesting
we have a number of places where we hard-code some stuff that should be generated by a configure script
I changed some of them to include the debug flags but maybe there were more places
I'm surprised that it didn't get the NDEBUG flag though
Marius Wachtler
@undingen
May 12 2016 21:54
let me check again
Kevin Modzelewski
@kmod
May 12 2016 21:56
You could try reverting 0e67e50f30938e5ecba3e41f27828e45b80734f8 and see if that helps
Nathaniel J. Smith
@njsmith
May 12 2016 21:57
Upstream packaging just backported a bunch of the new cabi checking to py27
So like pip now knows the difference between debug builds and regular builds of CPython 2.7 and that they have incompatible abis
I don't know if that's relevant at all to these issue, but it might be something useful to look at for handling these issues in general
Marius Wachtler
@undingen
May 12 2016 22:22
thanks! I will take a look at this tomorrow
strace from the pycrypto extension build in release mode:
execve("/usr/bin/gcc", ["gcc", "-std=gnu99", "-O3", "-fwrapv", "-Wall", "-Wstrict-prototypes", "-g", "-O2", "-Wall", "-Wextra", "-Wno-missing-field-initializers", "-Wno-unused-parameter", "-DHAVE_CONFIG_H", "-fPIC", "-Isrc/", "-I/home/malloc/pyston/build/Release/from_cpython/Include", "-c", "-Werror=implicit-function-declaration", "src/SHA256.c", "-o", "build/temp.linux-x86_64-2.7/src/SHA256.o"]
so it's really not receiving the NDEBUG
good thing -O3 and -O2 are passed at the same time too :-D
Marius Wachtler
@undingen
May 12 2016 22:30
from pycryptos setup.py:
if self.compiler.compiler_type in ('unix', 'cygwin', 'mingw32'):
            # Make assert() statements always work
            self.__remove_compiler_option("-DNDEBUG")
this was it now it works :-)
Kevin Modzelewski
@kmod
May 12 2016 22:40
oh man :)
I guess we shouldn't key Py_REF_DEBUG and friends off of NDEBUG
Marius Wachtler
@undingen
May 12 2016 22:43
yeah maybe cmake should enable this defines explicitly when it configures a debug build