From d4f060f0e4e88544528fd0e7953b6aa3e9155e09 Mon Sep 17 00:00:00 2001 From: Brett Cannon Date: Tue, 9 Feb 2016 14:41:04 -0800 Subject: [PATCH 01/12] Add (failing) tests for BUILD_LIST_UNPACK --- Tests/Tests.vcxproj | 2 + Tests/Tests.vcxproj.filters | 6 ++ Tests/test_emission.cpp | 109 ++++++++++++++++++++++++++++++++++++ Tests/test_inference.cpp | 24 +------- Tests/testing_util.cpp | 55 ++++++++++++++++++ Tests/testing_util.h | 9 +++ 6 files changed, 182 insertions(+), 23 deletions(-) create mode 100644 Tests/test_emission.cpp create mode 100644 Tests/testing_util.cpp create mode 100644 Tests/testing_util.h diff --git a/Tests/Tests.vcxproj b/Tests/Tests.vcxproj index 500e19996..ed565d4e8 100644 --- a/Tests/Tests.vcxproj +++ b/Tests/Tests.vcxproj @@ -161,7 +161,9 @@ Create Create + + diff --git a/Tests/Tests.vcxproj.filters b/Tests/Tests.vcxproj.filters index 3b5a36e79..7c9905f7b 100644 --- a/Tests/Tests.vcxproj.filters +++ b/Tests/Tests.vcxproj.filters @@ -35,5 +35,11 @@ Source Files + + Source Files + + + Source Files + \ No newline at end of file diff --git a/Tests/test_emission.cpp b/Tests/test_emission.cpp new file mode 100644 index 000000000..22a90dbdf --- /dev/null +++ b/Tests/test_emission.cpp @@ -0,0 +1,109 @@ +/* +* The MIT License (MIT) +* +* Copyright (c) Microsoft Corporation +* +* Permission is hereby granted, free of charge, to any person obtaining a +* copy of this software and associated documentation files (the "Software"), +* to deal in the Software without restriction, including without limitation +* the rights to use, copy, modify, merge, publish, distribute, sublicense, +* and/or sell copies of the Software, and to permit persons to whom the +* Software is furnished to do so, subject to the following conditions: +* +* The above copyright notice and this permission notice shall be included +* in all copies or substantial portions of the Software. +* +* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +* OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL +* THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR +* OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, +* ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR +* OTHER DEALINGS IN THE SOFTWARE. +* +*/ + +/** + Test JIT code emission. +*/ + +#include "stdafx.h" +#include "catch.hpp" +#include "testing_util.h" +#include +#include +#include + +class EmissionTest { +private: + PyCodeObject *m_code; + PyJittedCode *m_jittedcode; + +public: + EmissionTest(const char *code) { + m_code = CompileCode(code); + if (m_code == nullptr) { + FAIL("failed to compile code"); + } + m_jittedcode = JitCompile(m_code); + if (m_jittedcode == nullptr) { + FAIL("failed to JIT code"); + } + } + + ~EmissionTest() { + Py_XDECREF(m_code); + Py_XDECREF(m_jittedcode); + } + + const char* returns() { + auto sysModule = PyImport_ImportModule("sys"); + auto globals = PyDict_New(); + auto builtins = PyThreadState_GET()->interp->builtins; + PyDict_SetItemString(globals, "__builtins__", builtins); + PyDict_SetItemString(globals, "sys", sysModule); + + /* XXX Why? + PyRun_String("finalized = False\nclass RefCountCheck:\n def __del__(self):\n print('finalizing')\n global finalized\n finalized = True\n def __add__(self, other):\n return self", Py_file_input, globals, globals); + if (PyErr_Occurred()) { + PyErr_Print(); + return ""; + }*/ + + // Don't DECREF as frames are recycled. + auto frame = PyFrame_New(PyThreadState_Get(), m_code, globals, PyDict_New()); + + auto res = m_jittedcode->j_evalfunc(m_jittedcode->j_evalstate, frame); + Py_DECREF(globals); + Py_DECREF(sysModule); + REQUIRE(!PyErr_Occurred()); + REQUIRE(res != nullptr); + + auto repr = PyUnicode_AsUTF8(PyObject_Repr(res)); + Py_DECREF(res); + auto tstate = PyThreadState_GET(); + REQUIRE(tstate->exc_value == nullptr); + REQUIRE(tstate->exc_traceback == nullptr); + if (tstate->exc_type != nullptr) { + REQUIRE(tstate->exc_type == Py_None); + } + + return repr; + } + + PyObject* raises() { + return nullptr; + } +}; + +TEST_CASE("General list unpacking", "[list][BUILD_LIST_UNPACK][emission]") { + SECTION("common case") { + auto t = EmissionTest("def f(): return [1, *[2], 3]"); + REQUIRE(t.returns() == "[1, 2, 3]"); + } + + SECTION("unpacking a non-iterable") { + auto t = EmissionTest("def f(): return [1, *2, 3]"); + REQUIRE(t.raises() == PyExc_TypeError); + } +} \ No newline at end of file diff --git a/Tests/test_inference.cpp b/Tests/test_inference.cpp index 0bffcb693..9d5dffb9a 100644 --- a/Tests/test_inference.cpp +++ b/Tests/test_inference.cpp @@ -32,34 +32,12 @@ */ #include "stdafx.h" #include "catch.hpp" +#include "testing_util.h" #include #include #include #include -PyCodeObject* CompileCode(const char* code) { - auto globals = PyDict_New(); - auto builtins = PyThreadState_GET()->interp->builtins; - PyDict_SetItemString(globals, "__builtins__", builtins); - - auto locals = PyDict_New(); - PyRun_String(code, Py_file_input, globals, locals); - if (PyErr_Occurred()) { - PyErr_Print(); - Py_DECREF(globals); - FAIL("error occurred during Python compilation"); - return nullptr; - } - auto func = PyObject_GetItem(locals, PyUnicode_FromString("f")); - auto codeObj = (PyCodeObject*)PyObject_GetAttrString(func, "__code__"); - - Py_DECREF(globals); - Py_DECREF(locals); - Py_DECREF(func); - - return codeObj; -} - class InferenceTest { private: std::unique_ptr m_absint; diff --git a/Tests/testing_util.cpp b/Tests/testing_util.cpp new file mode 100644 index 000000000..023ad8a08 --- /dev/null +++ b/Tests/testing_util.cpp @@ -0,0 +1,55 @@ +/* +* The MIT License (MIT) +* +* Copyright (c) Microsoft Corporation +* +* Permission is hereby granted, free of charge, to any person obtaining a +* copy of this software and associated documentation files (the "Software"), +* to deal in the Software without restriction, including without limitation +* the rights to use, copy, modify, merge, publish, distribute, sublicense, +* and/or sell copies of the Software, and to permit persons to whom the +* Software is furnished to do so, subject to the following conditions: +* +* The above copyright notice and this permission notice shall be included +* in all copies or substantial portions of the Software. +* +* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +* OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL +* THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR +* OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, +* ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR +* OTHER DEALINGS IN THE SOFTWARE. +* +*/ + +/** + Testing utility code. +*/ + +#include "stdafx.h" +#include "catch.hpp" +#include + +PyCodeObject* CompileCode(const char* code) { + auto globals = PyDict_New(); + auto builtins = PyThreadState_GET()->interp->builtins; + PyDict_SetItemString(globals, "__builtins__", builtins); + + auto locals = PyDict_New(); + PyRun_String(code, Py_file_input, globals, locals); + if (PyErr_Occurred()) { + PyErr_Print(); + Py_DECREF(globals); + FAIL("error occurred during Python compilation"); + return nullptr; + } + auto func = PyObject_GetItem(locals, PyUnicode_FromString("f")); + auto codeObj = (PyCodeObject*)PyObject_GetAttrString(func, "__code__"); + + Py_DECREF(globals); + Py_DECREF(locals); + Py_DECREF(func); + + return codeObj; +} \ No newline at end of file diff --git a/Tests/testing_util.h b/Tests/testing_util.h new file mode 100644 index 000000000..391f7f103 --- /dev/null +++ b/Tests/testing_util.h @@ -0,0 +1,9 @@ +#pragma once + +#ifndef TESTING_UTIL_H +#define TESTING_UTIL_H 1 + +#include +PyCodeObject* CompileCode(const char*); + +#endif // !TESTING_UTIL_H \ No newline at end of file From 6b9d6f99f592f3f6a8075c679967de1adb46bd41 Mon Sep 17 00:00:00 2001 From: Brett Cannon Date: Tue, 9 Feb 2016 15:03:48 -0800 Subject: [PATCH 02/12] Check in busted BUILD_LIST_UNPACK to get help from Dino --- Pyjion/absint.cpp | 28 ++++++++++++++++++++++++++++ Pyjion/absint.h | 1 + Pyjion/intrins.cpp | 15 +++++++++++++++ Pyjion/intrins.h | 2 ++ Pyjion/ipycomp.h | 2 ++ Pyjion/pycomp.cpp | 5 +++++ Pyjion/pycomp.h | 3 +++ 7 files changed, 56 insertions(+) diff --git a/Pyjion/absint.cpp b/Pyjion/absint.cpp index 1e95b0e67..94ef0f21f 100644 --- a/Pyjion/absint.cpp +++ b/Pyjion/absint.cpp @@ -1536,6 +1536,30 @@ void AbstractInterpreter::build_list(size_t argCnt) { dec_stack(argCnt); } +void AbstractInterpreter::extend_list(size_t argCnt) { + _ASSERTE(argCnt > 0); + + m_comp->emit_new_list(0); + error_check("new list failed"); + + auto valueTmp = m_comp->emit_define_local(); + auto listTmp = m_comp->emit_define_local(); + + m_comp->emit_store_local(listTmp); + for (size_t curArg = 0; curArg < argCnt; curArg++) { + m_comp->emit_store_local(valueTmp); + dec_stack(); + m_comp->emit_load_local(listTmp); + m_comp->emit_load_local(valueTmp); + + m_comp->emit_list_extend(); + int_error_check("list extend failed"); + } + + m_comp->emit_free_local(valueTmp); + m_comp->emit_load_and_free_local(listTmp); +} + void AbstractInterpreter::build_set(size_t argCnt) { m_comp->emit_new_set(); @@ -2029,6 +2053,10 @@ JittedCode* AbstractInterpreter::compile_worker() { build_list(oparg); inc_stack(); break; + case BUILD_LIST_UNPACK: + extend_list(oparg); + inc_stack(); + break; case BUILD_MAP: build_map(oparg); inc_stack(); diff --git a/Pyjion/absint.h b/Pyjion/absint.h index d60e98649..e116a2fa9 100644 --- a/Pyjion/absint.h +++ b/Pyjion/absint.h @@ -268,6 +268,7 @@ class __declspec(dllexport) AbstractInterpreter { bool can_skip_lasti_update(int opcodeIndex); void build_tuple(size_t argCnt); void build_list(size_t argCnt); + void extend_list(size_t argCnt); void build_set(size_t argCnt); void unpack_ex(size_t size, int opcode); diff --git a/Pyjion/intrins.cpp b/Pyjion/intrins.cpp index ca78d5e73..22638298f 100644 --- a/Pyjion/intrins.cpp +++ b/Pyjion/intrins.cpp @@ -1192,6 +1192,21 @@ PyObject* PyJit_LoadClassDeref(PyFrameObject* frame, size_t oparg) { return value; } +int PyJit_ExtendList(PyObject *list, PyObject *extension) { + assert(PyList_CheckExact(list)); + auto res = _PyList_Extend((PyListObject*)list, extension); + Py_DECREF(extension); + int flag = 1; + if (res != Py_None) { + flag = 0; + } + else { + Py_DECREF(res); + } + + return flag; +} + int PyJit_StoreMap(PyObject *key, PyObject *value, PyObject* map) { assert(PyDict_CheckExact(map)); auto res = PyDict_SetItem(map, key, value); diff --git a/Pyjion/intrins.h b/Pyjion/intrins.h index b90e6e4af..8c2d7f068 100644 --- a/Pyjion/intrins.h +++ b/Pyjion/intrins.h @@ -178,6 +178,8 @@ int PyJit_Raise(PyObject *exc, PyObject *cause); PyObject* PyJit_LoadClassDeref(PyFrameObject* frame, size_t oparg); +int PyJit_ExtendList(PyObject *list, PyObject *extension); + int PyJit_StoreMap(PyObject *key, PyObject *value, PyObject* map); int PyJit_StoreSubscr(PyObject* value, PyObject *container, PyObject *index); diff --git a/Pyjion/ipycomp.h b/Pyjion/ipycomp.h index cfb70ce98..6e1192b65 100644 --- a/Pyjion/ipycomp.h +++ b/Pyjion/ipycomp.h @@ -216,6 +216,8 @@ class IPythonCompiler { virtual void emit_list_store(size_t argCnt) = 0; // Appends a single value to a list virtual void emit_list_append() = 0; + // Extends a list with a single iterator + virtual void emit_list_extend() = 0; // Creates a new set virtual void emit_new_set() = 0; diff --git a/Pyjion/pycomp.cpp b/Pyjion/pycomp.cpp index 6a52122d9..d0766c6cd 100644 --- a/Pyjion/pycomp.cpp +++ b/Pyjion/pycomp.cpp @@ -370,6 +370,10 @@ void PythonCompiler::emit_list_store(size_t argCnt) { m_il.free_local(listItems); } +void PythonCompiler::emit_list_extend() { + m_il.emit_call(METHOD_EXTENDLIST_TOKEN); +} + void PythonCompiler::emit_new_set() { m_il.load_null(); m_il.emit_call(METHOD_PYSET_NEW); @@ -1127,6 +1131,7 @@ GLOBAL_METHOD(METHOD_BINARY_XOR_TOKEN, &PyJit_BinaryXor, CORINFO_TYPE_NATIVEINT, GLOBAL_METHOD(METHOD_BINARY_OR_TOKEN, &PyJit_BinaryOr, CORINFO_TYPE_NATIVEINT, Parameter(CORINFO_TYPE_NATIVEINT), Parameter(CORINFO_TYPE_NATIVEINT)); GLOBAL_METHOD(METHOD_PYLIST_NEW, &PyList_New, CORINFO_TYPE_NATIVEINT, Parameter(CORINFO_TYPE_NATIVEINT)); +GLOBAL_METHOD(METHOD_EXTENDLIST_TOKEN, &PyJit_ExtendList, CORINFO_TYPE_INT, Parameter(CORINFO_TYPE_NATIVEINT), Parameter(CORINFO_TYPE_NATIVEINT)); GLOBAL_METHOD(METHOD_STOREMAP_TOKEN, &PyJit_StoreMap, CORINFO_TYPE_INT, Parameter(CORINFO_TYPE_NATIVEINT), Parameter(CORINFO_TYPE_NATIVEINT), Parameter(CORINFO_TYPE_NATIVEINT)); GLOBAL_METHOD(METHOD_STORESUBSCR_TOKEN, &PyJit_StoreSubscr, CORINFO_TYPE_INT, Parameter(CORINFO_TYPE_NATIVEINT), Parameter(CORINFO_TYPE_NATIVEINT), Parameter(CORINFO_TYPE_NATIVEINT)); GLOBAL_METHOD(METHOD_DELETESUBSCR_TOKEN, &PyJit_DeleteSubscr, CORINFO_TYPE_INT, Parameter(CORINFO_TYPE_NATIVEINT), Parameter(CORINFO_TYPE_NATIVEINT)); diff --git a/Pyjion/pycomp.h b/Pyjion/pycomp.h index 234189f6b..9dd85c82c 100644 --- a/Pyjion/pycomp.h +++ b/Pyjion/pycomp.h @@ -164,6 +164,8 @@ #define METHOD_GREATER_THAN_EQUALS_INT_TOKEN 0x0000006A #define METHOD_PERIODIC_WORK 0x0000006B +#define METHOD_EXTENDLIST_TOKEN 0x0000006C + // call helpers #define METHOD_CALL0_TOKEN 0x00010000 #define METHOD_CALL1_TOKEN 0x00010001 @@ -265,6 +267,7 @@ class PythonCompiler : public IPythonCompiler { virtual void emit_new_list(size_t argCnt); virtual void emit_list_store(size_t argCnt); + virtual void emit_list_extend(); virtual void emit_new_set(); virtual void emit_dict_store(); From fb103137b939e5daabb905e4b8f375d9417ed6d6 Mon Sep 17 00:00:00 2001 From: Brett Cannon Date: Tue, 9 Feb 2016 15:35:39 -0800 Subject: [PATCH 03/12] Try to avoid using the temp --- Pyjion/absint.cpp | 4 +--- Pyjion/intrins.cpp | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/Pyjion/absint.cpp b/Pyjion/absint.cpp index 94ef0f21f..857ab3bd1 100644 --- a/Pyjion/absint.cpp +++ b/Pyjion/absint.cpp @@ -1547,13 +1547,11 @@ void AbstractInterpreter::extend_list(size_t argCnt) { m_comp->emit_store_local(listTmp); for (size_t curArg = 0; curArg < argCnt; curArg++) { - m_comp->emit_store_local(valueTmp); - dec_stack(); m_comp->emit_load_local(listTmp); - m_comp->emit_load_local(valueTmp); m_comp->emit_list_extend(); int_error_check("list extend failed"); + dec_stack(); } m_comp->emit_free_local(valueTmp); diff --git a/Pyjion/intrins.cpp b/Pyjion/intrins.cpp index 22638298f..410582f50 100644 --- a/Pyjion/intrins.cpp +++ b/Pyjion/intrins.cpp @@ -1192,7 +1192,7 @@ PyObject* PyJit_LoadClassDeref(PyFrameObject* frame, size_t oparg) { return value; } -int PyJit_ExtendList(PyObject *list, PyObject *extension) { +int PyJit_ExtendList(PyObject *extension, PyObject *list) { assert(PyList_CheckExact(list)); auto res = _PyList_Extend((PyListObject*)list, extension); Py_DECREF(extension); From efe94ca403d539f26bf8623ae847b4149a237d37 Mon Sep 17 00:00:00 2001 From: Brett Cannon Date: Tue, 9 Feb 2016 15:45:23 -0800 Subject: [PATCH 04/12] Get everything running (but with reversed values) --- Pyjion/absint.cpp | 2 +- Pyjion/intrins.cpp | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/Pyjion/absint.cpp b/Pyjion/absint.cpp index 857ab3bd1..7240968b6 100644 --- a/Pyjion/absint.cpp +++ b/Pyjion/absint.cpp @@ -1550,8 +1550,8 @@ void AbstractInterpreter::extend_list(size_t argCnt) { m_comp->emit_load_local(listTmp); m_comp->emit_list_extend(); - int_error_check("list extend failed"); dec_stack(); + int_error_check("list extend failed"); } m_comp->emit_free_local(valueTmp); diff --git a/Pyjion/intrins.cpp b/Pyjion/intrins.cpp index 410582f50..522ace822 100644 --- a/Pyjion/intrins.cpp +++ b/Pyjion/intrins.cpp @@ -1196,11 +1196,9 @@ int PyJit_ExtendList(PyObject *extension, PyObject *list) { assert(PyList_CheckExact(list)); auto res = _PyList_Extend((PyListObject*)list, extension); Py_DECREF(extension); - int flag = 1; - if (res != Py_None) { + int flag = 1; // Assume error unless we prove to ourselves otherwise. + if (res == Py_None) { flag = 0; - } - else { Py_DECREF(res); } From dcd5c04cf5e03ffa5be3b881d5d8c10efb7b76ac Mon Sep 17 00:00:00 2001 From: Brett Cannon Date: Tue, 9 Feb 2016 15:53:36 -0800 Subject: [PATCH 05/12] Get the failing case for BUILD_LIST_UNPACK working --- Tests/test_emission.cpp | 50 ++++++++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/Tests/test_emission.cpp b/Tests/test_emission.cpp index 22a90dbdf..995ea8063 100644 --- a/Tests/test_emission.cpp +++ b/Tests/test_emission.cpp @@ -39,6 +39,30 @@ class EmissionTest { PyCodeObject *m_code; PyJittedCode *m_jittedcode; + PyObject *run() { + auto sysModule = PyImport_ImportModule("sys"); + auto globals = PyDict_New(); + auto builtins = PyThreadState_GET()->interp->builtins; + PyDict_SetItemString(globals, "__builtins__", builtins); + PyDict_SetItemString(globals, "sys", sysModule); + + /* XXX Why? + PyRun_String("finalized = False\nclass RefCountCheck:\n def __del__(self):\n print('finalizing')\n global finalized\n finalized = True\n def __add__(self, other):\n return self", Py_file_input, globals, globals); + if (PyErr_Occurred()) { + PyErr_Print(); + return ""; + }*/ + + // Don't DECREF as frames are recycled. + auto frame = PyFrame_New(PyThreadState_Get(), m_code, globals, PyDict_New()); + + auto res = m_jittedcode->j_evalfunc(m_jittedcode->j_evalstate, frame); + Py_DECREF(globals); + Py_DECREF(sysModule); + + return res; + } + public: EmissionTest(const char *code) { m_code = CompileCode(code); @@ -57,27 +81,9 @@ class EmissionTest { } const char* returns() { - auto sysModule = PyImport_ImportModule("sys"); - auto globals = PyDict_New(); - auto builtins = PyThreadState_GET()->interp->builtins; - PyDict_SetItemString(globals, "__builtins__", builtins); - PyDict_SetItemString(globals, "sys", sysModule); - - /* XXX Why? - PyRun_String("finalized = False\nclass RefCountCheck:\n def __del__(self):\n print('finalizing')\n global finalized\n finalized = True\n def __add__(self, other):\n return self", Py_file_input, globals, globals); - if (PyErr_Occurred()) { - PyErr_Print(); - return ""; - }*/ - - // Don't DECREF as frames are recycled. - auto frame = PyFrame_New(PyThreadState_Get(), m_code, globals, PyDict_New()); - - auto res = m_jittedcode->j_evalfunc(m_jittedcode->j_evalstate, frame); - Py_DECREF(globals); - Py_DECREF(sysModule); - REQUIRE(!PyErr_Occurred()); + auto res = run(); REQUIRE(res != nullptr); + REQUIRE(!PyErr_Occurred()); auto repr = PyUnicode_AsUTF8(PyObject_Repr(res)); Py_DECREF(res); @@ -92,7 +98,9 @@ class EmissionTest { } PyObject* raises() { - return nullptr; + auto res = run(); + REQUIRE(res == nullptr); + return PyErr_Occurred(); } }; From 487f2f83e26d34310096743fe05827873a563b22 Mon Sep 17 00:00:00 2001 From: Brett Cannon Date: Tue, 9 Feb 2016 16:32:42 -0800 Subject: [PATCH 06/12] BUILD_LIST_UNPACK recursively, but w/ an error --- Pyjion/absint.cpp | 31 +++++++++++++++++++++---------- Pyjion/absint.h | 1 + 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/Pyjion/absint.cpp b/Pyjion/absint.cpp index 7240968b6..2ce4a6e3a 100644 --- a/Pyjion/absint.cpp +++ b/Pyjion/absint.cpp @@ -1536,29 +1536,40 @@ void AbstractInterpreter::build_list(size_t argCnt) { dec_stack(argCnt); } +void AbstractInterpreter::extend_list_recursively(Local listTmp, size_t argCnt) { + if (argCnt == 0) { + return; + } + + auto valueTmp = m_comp->emit_define_local(); + m_comp->emit_store_local(valueTmp); + + extend_list_recursively(listTmp, --argCnt); + + m_comp->emit_load_local(listTmp); + m_comp->emit_load_local(valueTmp); + + m_comp->emit_list_extend(); + dec_stack(); + int_error_check("list extend failed"); + + m_comp->emit_free_local(valueTmp); +} + void AbstractInterpreter::extend_list(size_t argCnt) { _ASSERTE(argCnt > 0); m_comp->emit_new_list(0); error_check("new list failed"); - auto valueTmp = m_comp->emit_define_local(); auto listTmp = m_comp->emit_define_local(); - m_comp->emit_store_local(listTmp); - for (size_t curArg = 0; curArg < argCnt; curArg++) { - m_comp->emit_load_local(listTmp); - m_comp->emit_list_extend(); - dec_stack(); - int_error_check("list extend failed"); - } + extend_list_recursively(listTmp, argCnt); - m_comp->emit_free_local(valueTmp); m_comp->emit_load_and_free_local(listTmp); } - void AbstractInterpreter::build_set(size_t argCnt) { m_comp->emit_new_set(); error_check("build set failed"); diff --git a/Pyjion/absint.h b/Pyjion/absint.h index e116a2fa9..ada6e8acd 100644 --- a/Pyjion/absint.h +++ b/Pyjion/absint.h @@ -268,6 +268,7 @@ class __declspec(dllexport) AbstractInterpreter { bool can_skip_lasti_update(int opcodeIndex); void build_tuple(size_t argCnt); void build_list(size_t argCnt); + void extend_list_recursively(Local list, size_t argCnt); void extend_list(size_t argCnt); void build_set(size_t argCnt); From 2450d32ddefc14a884e5d76d88df08222ae0dbb6 Mon Sep 17 00:00:00 2001 From: Brett Cannon Date: Tue, 9 Feb 2016 16:40:59 -0800 Subject: [PATCH 07/12] Order is now right, but test fails for some unknown reason --- Pyjion/absint.cpp | 2 +- Pyjion/intrins.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Pyjion/absint.cpp b/Pyjion/absint.cpp index 2ce4a6e3a..f3f93e8b7 100644 --- a/Pyjion/absint.cpp +++ b/Pyjion/absint.cpp @@ -1543,6 +1543,7 @@ void AbstractInterpreter::extend_list_recursively(Local listTmp, size_t argCnt) auto valueTmp = m_comp->emit_define_local(); m_comp->emit_store_local(valueTmp); + dec_stack(); extend_list_recursively(listTmp, --argCnt); @@ -1550,7 +1551,6 @@ void AbstractInterpreter::extend_list_recursively(Local listTmp, size_t argCnt) m_comp->emit_load_local(valueTmp); m_comp->emit_list_extend(); - dec_stack(); int_error_check("list extend failed"); m_comp->emit_free_local(valueTmp); diff --git a/Pyjion/intrins.cpp b/Pyjion/intrins.cpp index 522ace822..771b36ba4 100644 --- a/Pyjion/intrins.cpp +++ b/Pyjion/intrins.cpp @@ -1192,7 +1192,7 @@ PyObject* PyJit_LoadClassDeref(PyFrameObject* frame, size_t oparg) { return value; } -int PyJit_ExtendList(PyObject *extension, PyObject *list) { +int PyJit_ExtendList(PyObject *list, PyObject *extension) { assert(PyList_CheckExact(list)); auto res = _PyList_Extend((PyListObject*)list, extension); Py_DECREF(extension); From 8df56204c0f1ffb86d8ee8caec492e8e96b57072 Mon Sep 17 00:00:00 2001 From: Brett Cannon Date: Tue, 9 Feb 2016 16:45:20 -0800 Subject: [PATCH 08/12] Use std::string for macro support in Catch for comparisons --- Tests/test_emission.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/test_emission.cpp b/Tests/test_emission.cpp index 995ea8063..3de6fec84 100644 --- a/Tests/test_emission.cpp +++ b/Tests/test_emission.cpp @@ -80,7 +80,7 @@ class EmissionTest { Py_XDECREF(m_jittedcode); } - const char* returns() { + std::string& returns() { auto res = run(); REQUIRE(res != nullptr); REQUIRE(!PyErr_Occurred()); @@ -94,7 +94,7 @@ class EmissionTest { REQUIRE(tstate->exc_type == Py_None); } - return repr; + return std::string(repr); } PyObject* raises() { From 9ddb21ac95e697fc85a83e7ad6a5dc50841a87c2 Mon Sep 17 00:00:00 2001 From: Brett Cannon Date: Wed, 10 Feb 2016 10:57:24 -0800 Subject: [PATCH 09/12] Fix refcount issues by introducing py_ptr --- Tests/test_emission.cpp | 47 ++++++++++++++-------------------------- Tests/test_inference.cpp | 11 ++++------ Tests/testing_util.cpp | 18 ++++++--------- Tests/testing_util.h | 40 ++++++++++++++++++++++++++++++++++ 4 files changed, 67 insertions(+), 49 deletions(-) diff --git a/Tests/test_emission.cpp b/Tests/test_emission.cpp index 3de6fec84..3da9c6c7d 100644 --- a/Tests/test_emission.cpp +++ b/Tests/test_emission.cpp @@ -36,57 +36,42 @@ class EmissionTest { private: - PyCodeObject *m_code; - PyJittedCode *m_jittedcode; + py_ptr m_code; + py_ptr m_jittedcode; - PyObject *run() { - auto sysModule = PyImport_ImportModule("sys"); - auto globals = PyDict_New(); + PyObject* run() { + auto sysModule = PyObject_ptr(PyImport_ImportModule("sys")); + auto globals = PyObject_ptr(PyDict_New()); auto builtins = PyThreadState_GET()->interp->builtins; - PyDict_SetItemString(globals, "__builtins__", builtins); - PyDict_SetItemString(globals, "sys", sysModule); - - /* XXX Why? - PyRun_String("finalized = False\nclass RefCountCheck:\n def __del__(self):\n print('finalizing')\n global finalized\n finalized = True\n def __add__(self, other):\n return self", Py_file_input, globals, globals); - if (PyErr_Occurred()) { - PyErr_Print(); - return ""; - }*/ + PyDict_SetItemString(globals.get(), "__builtins__", builtins); + PyDict_SetItemString(globals.get(), "sys", sysModule.get()); // Don't DECREF as frames are recycled. - auto frame = PyFrame_New(PyThreadState_Get(), m_code, globals, PyDict_New()); + auto frame = PyFrame_New(PyThreadState_Get(), m_code.get(), globals.get(), PyObject_ptr(PyDict_New()).get()); auto res = m_jittedcode->j_evalfunc(m_jittedcode->j_evalstate, frame); - Py_DECREF(globals); - Py_DECREF(sysModule); return res; } public: EmissionTest(const char *code) { - m_code = CompileCode(code); - if (m_code == nullptr) { + m_code.reset(CompileCode(code)); + if (m_code.get() == nullptr) { FAIL("failed to compile code"); } - m_jittedcode = JitCompile(m_code); - if (m_jittedcode == nullptr) { + m_jittedcode.reset(JitCompile(m_code.get())); + if (m_jittedcode.get() == nullptr) { FAIL("failed to JIT code"); } } - ~EmissionTest() { - Py_XDECREF(m_code); - Py_XDECREF(m_jittedcode); - } - - std::string& returns() { - auto res = run(); - REQUIRE(res != nullptr); + std::string returns() { + auto res = PyObject_ptr(run()); + REQUIRE(res.get() != nullptr); REQUIRE(!PyErr_Occurred()); - auto repr = PyUnicode_AsUTF8(PyObject_Repr(res)); - Py_DECREF(res); + auto repr = PyUnicode_AsUTF8(PyObject_Repr(res.get())); auto tstate = PyThreadState_GET(); REQUIRE(tstate->exc_value == nullptr); REQUIRE(tstate->exc_traceback == nullptr); diff --git a/Tests/test_inference.cpp b/Tests/test_inference.cpp index 9d5dffb9a..c8e4e5a00 100644 --- a/Tests/test_inference.cpp +++ b/Tests/test_inference.cpp @@ -44,10 +44,9 @@ class InferenceTest { public: InferenceTest(const char* code) { - auto pyCode = CompileCode(code); - m_absint = std::make_unique(pyCode, nullptr); + auto pyCode = py_ptr(CompileCode(code)); + m_absint = std::make_unique(pyCode.get(), nullptr); auto success = m_absint->interpret(); - Py_DECREF(pyCode); if (!success) { FAIL("Failed to interpret code"); } @@ -127,16 +126,14 @@ class AITestCase { }; void VerifyOldTest(AITestCase testCase) { - auto codeObj = CompileCode(testCase.m_code); + auto codeObj = py_ptr(CompileCode(testCase.m_code)); - AbstractInterpreter interpreter(codeObj, nullptr); + AbstractInterpreter interpreter(codeObj.get(), nullptr); if (!interpreter.interpret()) { FAIL("Failed to interprete code"); } testCase.verify(interpreter); - - Py_DECREF(codeObj); } /* ==================================================== */ diff --git a/Tests/testing_util.cpp b/Tests/testing_util.cpp index 023ad8a08..c6dc57340 100644 --- a/Tests/testing_util.cpp +++ b/Tests/testing_util.cpp @@ -29,27 +29,23 @@ #include "stdafx.h" #include "catch.hpp" +#include "testing_util.h" #include PyCodeObject* CompileCode(const char* code) { - auto globals = PyDict_New(); + auto globals = PyObject_ptr(PyDict_New()); auto builtins = PyThreadState_GET()->interp->builtins; - PyDict_SetItemString(globals, "__builtins__", builtins); + PyDict_SetItemString(globals.get(), "__builtins__", builtins); - auto locals = PyDict_New(); - PyRun_String(code, Py_file_input, globals, locals); + auto locals = PyObject_ptr(PyDict_New()); + PyRun_String(code, Py_file_input, globals.get(), locals.get()); if (PyErr_Occurred()) { PyErr_Print(); - Py_DECREF(globals); FAIL("error occurred during Python compilation"); return nullptr; } - auto func = PyObject_GetItem(locals, PyUnicode_FromString("f")); - auto codeObj = (PyCodeObject*)PyObject_GetAttrString(func, "__code__"); - - Py_DECREF(globals); - Py_DECREF(locals); - Py_DECREF(func); + auto func = PyObject_ptr(PyObject_GetItem(locals.get(), PyUnicode_FromString("f"))); + auto codeObj = (PyCodeObject*)PyObject_GetAttrString(func.get(), "__code__"); return codeObj; } \ No newline at end of file diff --git a/Tests/testing_util.h b/Tests/testing_util.h index 391f7f103..0d86e5f0a 100644 --- a/Tests/testing_util.h +++ b/Tests/testing_util.h @@ -4,6 +4,46 @@ #define TESTING_UTIL_H 1 #include + +/** + Use RAII to Py_XDECREF a pointer. + + Inspired by std::unique_ptr. +*/ +template class py_ptr { +private: + T* m_ptr; + +public: + py_ptr() : m_ptr(nullptr) {} + py_ptr(T* ptr) : m_ptr(ptr) {} + + ~py_ptr() { + Py_XDECREF(m_ptr); + } + + void reset(T* ptr) { + Py_XDECREF(m_ptr); + m_ptr = ptr; + } + + T* get() { + return m_ptr; + } + + T* operator->() { + return m_ptr; + } +}; + +/** + A concrete PyObject instance of py_ptr. +*/ +class PyObject_ptr : public py_ptr { +public: + PyObject_ptr(PyObject *ptr) : py_ptr(ptr) {} +}; + PyCodeObject* CompileCode(const char*); #endif // !TESTING_UTIL_H \ No newline at end of file From 54a633075591f7a7f89ef9e3aae1569395f9a22d Mon Sep 17 00:00:00 2001 From: Brett Cannon Date: Wed, 10 Feb 2016 11:14:26 -0800 Subject: [PATCH 10/12] Add trailing newlines to some files --- Tests/test_inference.cpp | 11 +++++++---- Tests/testing_util.cpp | 2 +- Tests/testing_util.h | 2 +- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/Tests/test_inference.cpp b/Tests/test_inference.cpp index c8e4e5a00..a2db421a7 100644 --- a/Tests/test_inference.cpp +++ b/Tests/test_inference.cpp @@ -44,10 +44,11 @@ class InferenceTest { public: InferenceTest(const char* code) { - auto pyCode = py_ptr(CompileCode(code)); - m_absint = std::make_unique(pyCode.get(), nullptr); + auto pyCode = CompileCode(code); + m_absint = std::make_unique(pyCode, nullptr); auto success = m_absint->interpret(); if (!success) { + Py_DECREF(pyCode); FAIL("Failed to interpret code"); } } @@ -126,14 +127,16 @@ class AITestCase { }; void VerifyOldTest(AITestCase testCase) { - auto codeObj = py_ptr(CompileCode(testCase.m_code)); + auto codeObj = CompileCode(testCase.m_code); - AbstractInterpreter interpreter(codeObj.get(), nullptr); + AbstractInterpreter interpreter(codeObj, nullptr); if (!interpreter.interpret()) { FAIL("Failed to interprete code"); } testCase.verify(interpreter); + + Py_DECREF(codeObj); } /* ==================================================== */ diff --git a/Tests/testing_util.cpp b/Tests/testing_util.cpp index c6dc57340..8aecb18eb 100644 --- a/Tests/testing_util.cpp +++ b/Tests/testing_util.cpp @@ -48,4 +48,4 @@ PyCodeObject* CompileCode(const char* code) { auto codeObj = (PyCodeObject*)PyObject_GetAttrString(func.get(), "__code__"); return codeObj; -} \ No newline at end of file +} diff --git a/Tests/testing_util.h b/Tests/testing_util.h index 0d86e5f0a..f0c2ebfe7 100644 --- a/Tests/testing_util.h +++ b/Tests/testing_util.h @@ -46,4 +46,4 @@ class PyObject_ptr : public py_ptr { PyCodeObject* CompileCode(const char*); -#endif // !TESTING_UTIL_H \ No newline at end of file +#endif // !TESTING_UTIL_H From 48af9a93714d23e678e914396f8bdd6bf8a192ae Mon Sep 17 00:00:00 2001 From: Brett Cannon Date: Wed, 10 Feb 2016 11:27:44 -0800 Subject: [PATCH 11/12] Reset the exception state after verifying it has occurred --- Tests/test_emission.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Tests/test_emission.cpp b/Tests/test_emission.cpp index 3da9c6c7d..a59dd68c2 100644 --- a/Tests/test_emission.cpp +++ b/Tests/test_emission.cpp @@ -85,7 +85,9 @@ class EmissionTest { PyObject* raises() { auto res = run(); REQUIRE(res == nullptr); - return PyErr_Occurred(); + auto excType = PyErr_Occurred(); + PyErr_Clear(); + return excType; } }; From 5be7b1cb2ca955b29e43536422ce6f81f5a790bd Mon Sep 17 00:00:00 2001 From: Brett Cannon Date: Wed, 10 Feb 2016 14:02:04 -0800 Subject: [PATCH 12/12] Add a copy constructor and * overload --- Tests/testing_util.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Tests/testing_util.h b/Tests/testing_util.h index f0c2ebfe7..5770cf24e 100644 --- a/Tests/testing_util.h +++ b/Tests/testing_util.h @@ -17,6 +17,10 @@ template class py_ptr { public: py_ptr() : m_ptr(nullptr) {} py_ptr(T* ptr) : m_ptr(ptr) {} + py_ptr(const py_ptr& copy) { + m_ptr = copy.get(); + Py_INCREF(m_ptr); + } ~py_ptr() { Py_XDECREF(m_ptr); @@ -34,6 +38,10 @@ template class py_ptr { T* operator->() { return m_ptr; } + + T* operator*() { + return m_ptr; + } }; /**