Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't crash on non-mapping like arguments on kwargs. Fixes #805 #831

Open
wants to merge 2,986 commits into
base: master
Choose a base branch
from

Conversation

vinzenz
Copy link
Contributor

@vinzenz vinzenz commented Aug 13, 2015

No description provided.

kmod and others added 30 commits July 26, 2015 19:28
They are tricky since these are types, which means they invoke
the relatively-complicated constructor logic.  ie str() doesn't
just call __str__ on the argument: if the result is a subclass
of str, it calls result.__init__().  Similarly for unicode, except
unicode is even trickier since it takes some more arguments, one
of which is "encoding" which will have non-type-based dynamic
behavior.

I didn't realize that at first and optimized unicode() by exposing an
inner version of it that takes its arguments in registers, which we
can take advantage of using our jit-arg-rearrangement capability.
This means we have to do parts of PyArg_ParseTuple ourselves, so I
added a PyArg_ParseSingle that runs a single object through the
arg-conversion code.  PyArg_ParseSingle could be further optimized if
we want to.  Or rather, if we have functions of the form
PyArg_ParseSingle_s (which corresponds to the "s" format code) we
could skip some more of the overhead.

I had to disable most of that once I realized the encoding issue, but
I left it in since hopefully we will be able to use it again once
we have some "do some guards after mutations if we know how to resume
after a failed guard" rewriter support.
callable(), str(), repr(), PySequence_GetItem(),
and PyObject_HasAttrString()

Mostly by bringing in the CPython versions.
optimize some misc runtime functions
Previously after doing a OSR JIT compilation we continued to JIT every block outside of the loop.
This doesn't show up as a perf change but reduces the number of JITed code / makes it slightly smaller.
The exceptions thrown by len itself can now be either style,
though any exceptions thrown by any called functions (ex __len__)
will still get thrown as C++ exceptions and converted if needed.

Helps in a common case of "try calling len but don't worry if no
len was defined".
Numeric binary operator support for old style class
bjit: add support for most common missing nodes and don't JIT compile cold blocks
For use of PyObject_GetItem

django_template3 ends up calling this a fair amount via unicode_translate
(ie it checks to see if certain entries are in the translation table).
Apparently they can do compile-time evaluations, which is cool.
Fix the errors that report in `test format` and re-enable `test format`.
start templatizing the runtime to be able to choose exception styles
@vinzenz vinzenz force-pushed the kw-args-crash-fix branch 3 times, most recently from c8e6f4a to 04d7bfd Compare August 14, 2015 11:48
kmod and others added 4 commits August 14, 2015 14:37
We currently over-build it quite a bit; I'll try to look into that
issue, but for now we can lessen the effects by at least building
it in parallel.
Probably more compatible, but also much faster.

Also, set str_cls->tp_repr.
@@ -3293,8 +3301,9 @@ void rearrangeArguments(ParamReceiveSpec paramspec, const ParamNames* param_name
getArg(kwargs_idx, oarg1, oarg2, oarg3, oargs) = okwargs;
}

if ((!param_names || !param_names->takes_param_names) && argspec.num_keywords && !paramspec.takes_kwargs) {
raiseExcHelper(TypeError, "%s() doesn't take keyword arguments", func_name);
if ((!param_names || !param_names->takes_param_names) && (argspec.num_keywords || argspec.has_kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is the cause of the test failures, but I think it's perfectly allowable to pass kwargs to a non-keyword-taking function if the kwargs dict is empty.

for (Box* e : given_varargs->pyElements()) {
varargs.push_back(e);
}
} catch (ExcInfo const& e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, but I think our unwinder requires that we catch exceptions by value; maybe the compiler translates the catch-by-const-reference to a catch-by-value but it's probably better to just have it explicitly catch by value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I didn't know that it is required because of that. Good to know. Will fix that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants