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.

Sign up for GitHub

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

gh-104240: make _PyCompile_CodeGen support different compilation modes #104241

Merged
merged 4 commits into from
May 7, 2023
+43 −15
Merged

gh-104240: make _PyCompile_CodeGen support different compilation modes #104241

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0630b4e
move compiler_enter_scope out of compiler_codegen
iritkatriel May 6, 2023
52f3a59
support different compilation modes in _PyCompile_CodeGen
iritkatriel May 6, 2023
7f654d7
update codegen test (we add return None now)
iritkatriel May 6, 2023
de8b2ba
Revert "move compiler_enter_scope out of compiler_codegen"
iritkatriel May 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Include/internal/pycore_compile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ PyAPI_FUNC(PyObject*) _PyCompile_CodeGen(
PyObject *ast,
PyObject *filename,
PyCompilerFlags *flags,
int optimize);
int optimize,
int compile_mode);

PyAPI_FUNC(PyObject*) _PyCompile_OptimizeCfg(
PyObject *instructions,
Expand Down
1 change: 1 addition & 0 deletions Include/internal/pycore_global_objects_fini_generated.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Include/internal/pycore_global_strings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,7 @@ struct _Py_global_strings {
STRUCT_FOR_ID(code)
STRUCT_FOR_ID(command)
STRUCT_FOR_ID(comment_factory)
STRUCT_FOR_ID(compile_mode)
STRUCT_FOR_ID(consts)
STRUCT_FOR_ID(context)
STRUCT_FOR_ID(cookie)
Expand Down
1 change: 1 addition & 0 deletions Include/internal/pycore_runtime_init_generated.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions Include/internal/pycore_unicodeobject_generated.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions Lib/test/test_compiler_codegen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ def test_if_expression(self):
('LOAD_CONST', 2, 1),
exit_lbl,
('POP_TOP', None),
('LOAD_CONST', 3),
('RETURN_VALUE', None),
]
self.codegen_test(snippet, expected)

Expand All @@ -46,5 +48,7 @@ def test_for_loop(self):
('JUMP', loop_lbl),
exit_lbl,
('END_FOR', None),
('LOAD_CONST', 0),
('RETURN_VALUE', None),
]
self.codegen_test(snippet, expected)
8 changes: 5 additions & 3 deletions Modules/_testinternalcapi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,17 +593,19 @@ _testinternalcapi.compiler_codegen -> object
ast: object
filename: object
optimize: int
compile_mode: int = 0
Apply compiler code generation to an AST.
[clinic start generated code]*/

static PyObject *
_testinternalcapi_compiler_codegen_impl(PyObject *module, PyObject *ast,
PyObject *filename, int optimize)
/*[clinic end generated code: output=fbbbbfb34700c804 input=e9fbe6562f7f75e4]*/
PyObject *filename, int optimize,
int compile_mode)
/*[clinic end generated code: output=40a68f6e13951cc8 input=a0e00784f1517cd7]*/
{
PyCompilerFlags *flags = NULL;
return _PyCompile_CodeGen(ast, filename, flags, optimize);
return _PyCompile_CodeGen(ast, filename, flags, optimize, compile_mode);
}


Expand Down
29 changes: 20 additions & 9 deletions Modules/clinic/_testinternalcapi.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 31 additions & 17 deletions Python/compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1655,16 +1655,10 @@ compiler_body(struct compiler *c, location loc, asdl_stmt_seq *stmts)
static int
compiler_codegen(struct compiler *c, mod_ty mod)
{
_Py_DECLARE_STR(anon_module, "<module>");
RETURN_IF_ERROR(
compiler_enter_scope(c, &_Py_STR(anon_module), COMPILER_SCOPE_MODULE,
mod, 1));

Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of moving this from here out into the (only) two callers of this function? It duplicates this line of code, but it doesn't seem like it would make any other difference, AFAICT.

Copy link
Member Author

Choose a reason for hiding this comment

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

The purpose is to be able to make a distinction between compiler_enter_scope failed, in which case we don't need to exit the scope, and the case where compiler_enter_scope succeeded but compiler_codegen failed later, in which case we do need to exit the scope. I don't think this is currently correct in all cases in main.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense! Beyond adding the compilation modes option, I wasn't clear where this PR was aiming to maintain behavior vs fixing problems in existing behavior.

Looking at the definition of compiler_exit_scope, it seems like if we have one too many it would leave things in an observably broken state (like too small c_nestlevel?) Is there somewhere we can add asserts to show that things are broken today, but not after this fix?

Also, it seems like it would be simplest to get the enter/exit logic correct if we do it just once, inside compiler_codegen, rather than in both callers?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm looking into it, I think it's a bigger mess than I previously thought. There are functions like compile_class which always exit the scope (which they did not enter). And VISIT_SEQ/VISIT_SEQ_IN_SCOPE also call compiler_exit_scope.

Also, it seems like it would be simplest to get the enter/exit logic correct if we do it just once, inside compiler_codegen, rather than in both callers?

I don't think we can exit the scope in compiler_codegen, because we need it until after optimize_and_assemble.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to revert the commit with the enter/exit changes for now - I probably only needed it in a broken state when I was sending bad inputs to the unit tests.

location loc = LOCATION(1, 1, 0, 0);
switch (mod->kind) {
case Module_kind:
if (compiler_body(c, loc, mod->v.Module.body) < 0) {
compiler_exit_scope(c);
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused by this; how does this not leave us with one fewer compiler_exit_scope than we had previously? We still have the same number of compiler_enter_scope (it's just moved from above into both callers of compiler_codegen), and both callers still do the same number of compiler_exit_scope that they did before (1), but we eliminate this compiler_exit_scope. Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think currently we may have one too many - one exit here and then we return to the caller which exits again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I'm wrong in the case of compiler_mod, I'll fix it there.

return ERROR;
}
break;
Expand All @@ -1691,6 +1685,11 @@ static PyCodeObject *
compiler_mod(struct compiler *c, mod_ty mod)
{
int addNone = mod->kind != Expression_kind;
_Py_DECLARE_STR(anon_module, "<module>");
if (compiler_enter_scope(c, &_Py_STR(anon_module), COMPILER_SCOPE_MODULE,
mod, 1) < 0) {
return NULL;
}
if (compiler_codegen(c, mod) < 0) {
return NULL;
}
Expand Down Expand Up @@ -7258,42 +7257,57 @@ cfg_to_instructions(cfg_builder *g)

PyObject *
_PyCompile_CodeGen(PyObject *ast, PyObject *filename, PyCompilerFlags *pflags,
int optimize)
int optimize, int compile_mode)
{
PyObject *res = NULL;
struct compiler *c = NULL;
PyArena *arena = NULL;

if (!PyAST_Check(ast)) {
PyErr_SetString(PyExc_TypeError, "expected an AST");
return NULL;
}

PyArena *arena = _PyArena_New();
arena = _PyArena_New();
if (arena == NULL) {
return NULL;
goto end;
}

mod_ty mod = PyAST_obj2mod(ast, arena, 0 /* exec */);
mod_ty mod = PyAST_obj2mod(ast, arena, compile_mode);
if (mod == NULL || !_PyAST_Validate(mod)) {
_PyArena_Free(arena);
return NULL;
goto end;
}

struct compiler *c = new_compiler(mod, filename, pflags, optimize, arena);
c = new_compiler(mod, filename, pflags, optimize, arena);
if (c == NULL) {
_PyArena_Free(arena);
return NULL;
goto end;
}

_Py_DECLARE_STR(anon_module, "<module>");
if (compiler_enter_scope(c, &_Py_STR(anon_module), COMPILER_SCOPE_MODULE,
mod, 1) < 0) {
goto end;
}

if (compiler_codegen(c, mod) < 0) {
goto finally;
}
int addNone = mod->kind != Expression_kind;
if (add_return_at_end(c, addNone) < 0) {
return NULL;
}

res = instr_sequence_to_instructions(INSTR_SEQUENCE(c));

finally:
compiler_exit_scope(c);
compiler_free(c);
_PyArena_Free(arena);
end:
if (c != NULL) {
compiler_free(c);
}
if (arena != NULL) {
_PyArena_Free(arena);
}
return res;
}

Expand Down