I'm working on a branch to implement PEP-649:

https://github.com/larryhastings/co_annotations/

Inada Naoki discovered a crash in that branch, discussed here, including steps to reproduce:

larryhastings/co_annotations#10

valgrind showed me what the problem was. stackeffect() allocates a "stack" variable, used to store pushed/popped context while iterating over the basic blocks of the function being assembled. Most of the time, the stack is way bigger than it needs to be--we allocate 4 or 5 entries and it only uses 1 or 2. But, somehow, in the co_annotations branch, the "stack" was occasionally *way too small*. As in, it allocated 66 entries (!) but used 150 (!!).

I don't understand exactly how stackeffect works, so I don't know under what circumstances it would go so deep, much less what would cause it to so severely underestimate how many entries it needed. I *did* make modifications to code generation in compile.c, so it *could* be my bug--but my changes were all much earlier in the process, and AFAIK I never touched any of the code under assemble().

Well, not until I worked around this problem, anyway. My fix: if "stack" is too small, double the size and realloc(). Certainly it makes the problem go away. That's checked in to my branch here:

larryhastings/co_annotations@63b415c

But it doesn't address the underlying bug, whatever it is.

If anybody who understands stackeffect() could take a look and figure it out? That would be neat-o keen.

(Sorry, the name of the function is stackdepth(), not stackeffect().)

Larry, I checked out your branch to test my hypothesis but I was unable to build it. If you still can build it, then perhaps you can try adding the assertion I suggest below to see if it trips.

The size of the stack is the number of blocks, and the intention is that each block is pushed at most once. The only way I can see blocks being pushed more than once is if in stackdepth_push we have b->b_startdepth < 0 && depth < 0 && b->b_startdepth < depth. By why would depth ever be <0?

It is possible that this happened if your changes caused the bytecode to be what we call "invalid", such that new_depth is negative. Unlike the other places where we calculate a depth, there is no assertion that new_depth is >= 0. Then we have depth = new_depth, and if we loop again we reach the assertion on depth >= 0. But if this is the last iteration, then we don't check depth, we just call stackdepth_push(&sp, next, depth); after the loop ends.

If next is unvisited, then next->b_startdepth is MIN_INT. So it will be smaller than some small-negative depth.

In summary, I think it is a bug in your code, which would have been easier to find if we had
assert(new_depth >= 0); /* invalid code or bug in stackdepth() */ just after new_depth is calculated.

I will make a patch to add this assertion.

Could it be related to the discussion here about potentially switching "visited" to mean "pushed" instead of the current meaning, "popped"? This could cause overflow if multiple instructions have the same target, and so things get pushed twice before they're popped.

Never mind, I don't think that's the issue -- stackdepth already uses "visited"="pushed".

That was my first thought as well, but here depth >= 0 seems to be used as a proxy for ‘visited’. So I think the only way we could insert something more than once is if its depth is negative, but not MIN_INT.