Repository navigation
Error handling for some instructions is incorrect #99298
Description
Activity
Interesting. There are now some instrs that have
JUMPY(<cache size>); CHECK_EVAL_BREAKER();.Once those instrs get cache effects theJUMPBY()would come from the generator; can it safely be moved after the eval_breaker check, or does the generator now need to generate those checks too (presumably based on some one-of flag in the instr definition)?The
JUMPBY(...);needs to happen before theCHECK_EVAL_BREAKER();(as it does now) since we need to dispatch to the next instruction after handling eval breakers.(Note also that error handling for eval breakers is unaffected by this issue, since we can just pretend that any errors raised by eval breaker checks happened as part of the next instruction... basically, we act as if eval breakers are checked just before the next instruction, rather than just after the current one.)
So yeah, we probably need a way to spell this as part of the DSL. But it might be able to wait for now... I think only
CALL,CALL_FUNCTION_EX, andJUMP_BACKWARDare affected (RESUMEis a weird one... it checks the eval breaker only on certain opargs).Another option could be to make
CHECK_EVAL_BREAKERits own instruction, and put it after every call and before every backward jump. Not sure how expensive that would be, though.I'd rather not add more opcodes. These can use the legacy format for now; eventually we'll add some silly little flag to the instruction definition format.
In several places, we have
goto error;branches in bytecode instructions that occur after modifying thenext_instrpointer. This is incorrect, since the error branch will behave as if the error occurred in the new location (most often an adjacent instruction). The result could be as benign as an incorrect location in a traceback, or as problematic as incorrect control flow in or near atry/exceptblock.I tried for a bit to make the compiler emit code that did the wrong thing here, and I wasn't able to. So this is mostly a theoretical concern (but still worth fixing).