bpo-24340: Fix estimation of the code stack size.#5076
bpo-24340: Fix estimation of the code stack size.#5076serhiy-storchaka merged 10 commits intopython:masterfrom
Conversation
|
I'd rather see https://bugs.python.org/issue17611 finished than review another bytecode PR. |
|
I prefer to fix separate issues by separate commits. Fixing this issue will make patches for bpo-17611 simpler. Currently they contain changes for fixing this issue (not completely). |
|
In particularly this could limit changes in PyCompile_OpcodeStackEffect by only changes related to new and changed opcodes. |
|
OTOH reviewing two bytecode and compiler PRs is more work than reviewing a single one. And I would like to see how Mark's approach works with this. |
|
Isn't reviewing reviewing two separate PRs for two issues easier than reviewing the single PR that mixes fixes of two issues in the same code? Or rather reviewing three PRs every of them fixing the same two issues by slightly different code? |
|
Well, those three PRs will have to be regenerated because of conflicts after this PR gets merged. In any case, if you want to go forward with this, I would like to see more comments (especially on opcodes with a variable stack effect). |
|
What information do you want be added? |
Python/compile.c
Outdated
| maxdepth = target_depth; | ||
| instr->i_opcode == SETUP_EXCEPT) | ||
| { | ||
| depth = depth - 6; |
There was a problem hiding this comment.
It's not obvious why that is. SETUP_FINALLY and SETUP_EXCEPT merely call PyFrame_BlockSetup. Can you add a comment?
There was a problem hiding this comment.
PyFrame_BlockSetup doesn't affect the stack of values. The stack effect of these opcodes is 0 in normal flow.
There was a problem hiding this comment.
Can you add comments for all this? (why -6?)
There was a problem hiding this comment.
+6 (jump) - 6 = 0 (not jump)
Do you want to duplicate comments from PyCompile_OpcodeStackEffect()? Or just add a reference to PyCompile_OpcodeStackEffect()?
There was a problem hiding this comment.
Duplicating comments is fine. The file is large anyway.
There was a problem hiding this comment.
In any case I'm going to rewrite this code in bpo-32455 to something like:
new_depth = depth + PyCompile_OpcodeStackEffectEx(opcode, oparg, 0);
if (new_depth > maxdepth)
maxdepth = new_depth;
if (isjump) {
target_depth = depth + PyCompile_OpcodeStackEffectEx(opcode, oparg, 1);
maxdepth = stackdepth_walk(c, instr->i_target, target_depth, maxdepth);
}
depth = new_depth;And all comments will be in a single place.
Python/compile.c
Outdated
| else if (instr->i_opcode == SETUP_WITH || | ||
| instr->i_opcode == SETUP_ASYNC_WITH) | ||
| { | ||
| depth = depth - 5; |
There was a problem hiding this comment.
Same as above. See comments in PyCompile_OpcodeStackEffect().
Python/compile.c
Outdated
| } | ||
| if (instr->i_opcode == FOR_ITER) { | ||
| target_depth = depth-2; | ||
| target_depth = depth - 2; |
There was a problem hiding this comment.
Not sure why. At best, FOR_ITER calls STACKADJ(-1) (when the iterator is exhausted).
There was a problem hiding this comment.
See PyCompile_OpcodeStackEffect(). The effect is +1 in normal flow and -1 when jumps.
|
Could you please take a look again @pitrou? All stack effects are now defined in a single place. |
|
This looks ok to me. I can't guarantee that the compiler changes for |
4 participants
Tests based on patch by Antoine Pitrou (GH #2827).
https://bugs.python.org/issue24340