Skip to content

gh-104635: Add NO_EXCEPTION flag to opcode metadata #106394

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

Closed
wants to merge 3 commits into from
Closed

gh-104635: Add NO_EXCEPTION flag to opcode metadata #106394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11cf8ec
gh-104635: Add NO_EXCEPTION flag to opcode metadata
corona10 Jul 4, 2023
47b2995
Add application example
corona10 Jul 4, 2023
3c59a3b
remove GO_TO_INSTRUCTION case
corona10 Jul 5, 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
2 changes: 2 additions & 0 deletions Python/flowgraph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1495,6 +1495,8 @@ optimize_basic_block(PyObject *const_cache, basicblock *bb, PyObject *consts)
if (opcode == nextop &&
oparg == bb->b_instr[i+1].i_oparg &&
bb->b_instr[i].i_loc.lineno == bb->b_instr[i+1].i_loc.lineno) {
assert(OPCODE_NO_EXCEPTION(opcode));
assert(OPCODE_NO_EXCEPTION(nextop));
bb->b_instr[i].i_opcode = POP_TOP;
bb->b_instr[i].i_oparg = 0;
}
Expand Down
32 changes: 17 additions & 15 deletions Python/opcode_metadata.h
View file
Open in desktop

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

17 changes: 16 additions & 1 deletion Tools/cases_generator/generate_cases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ class InstructionFlags:
HAS_CONST_FLAG: bool
HAS_NAME_FLAG: bool
HAS_JUMP_FLAG: bool
NO_EXCEPTION_FLAG: bool
Copy link
Member

Choose a reason for hiding this comment

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

Negative flags tend to lead to confusing double-negative code constructs. So I would prefer for the flag to be CAN_RAISE_FLAG than NO_EXCEPTION_FLAG, and similar throughout the PR.


def __post_init__(self):
self.bitmask = {
Expand All @@ -265,11 +266,12 @@ def fromInstruction(instr: "AnyInstruction"):
HAS_CONST_FLAG=variable_used(instr, "FRAME_CO_CONSTS"),
HAS_NAME_FLAG=variable_used(instr, "FRAME_CO_NAMES"),
HAS_JUMP_FLAG=variable_used(instr, "JUMPBY"),
NO_EXCEPTION_FLAG=no_exception(instr),
)

@staticmethod
def newEmpty():
return InstructionFlags(False, False, False, False)
return InstructionFlags(False, False, False, False, False)

def add(self, other: "InstructionFlags") -> None:
for name, value in dataclasses.asdict(other).items():
Expand Down Expand Up @@ -1585,6 +1587,19 @@ def variable_used(node: parser.Node, name: str) -> bool:
)


def no_exception(node: parser.Node) -> bool:
if node.name.startswith("JUMP"):
return False
for token in node.tokens:
token_text = token.text.lower()
if token.kind == "IDENTIFIER":
if token_text == "error_if":
return False
if token_text.startswith("py") or token_text.startswith("_py"):
Copy link
Member

Choose a reason for hiding this comment

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

There are static helper functions in ceval.c that are used by opcodes and not Py prefixed, and some of these can raise. E.g. match_class. We can special case the existing ones, but someone could add a new one at any time. So I don't think this heuristic is safe, and I'm not sure I see any possible heuristic that would be safe in the face of possible future changes.

Copy link
Member

@carljm carljm Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I guess the match_class case should be covered by ERROR_IF used afterward to check if _PyErr_Occurred. But what if someone calls a non Py prefixed helper function and then uses a manual goto error; rather than ERROR_IF? There are some opcodes that use goto error; directly...

Maybe checking for both ERROR_IF and goto error would be adequate, and we wouldn't even need to check for use of Py prefixed API? For an opcode implementation to handle an error correctly, it seems it needs to goto error or ERROR_IF. Unless someone introduces another new macro that includes goto error, that would break the heuristic.

Existing macros that implicitly goto error include CHECK_EVAL_BREAKER, DECREF_INPUTS_AND_REUSE_FLOAT, and INSTRUMENTED_JUMP.

return False
return True


def main():
"""Parse command line, parse input, analyze, write output."""
args = arg_parser.parse_args() # Prints message and sys.exit(2) on error
Expand Down