Skip to content

Commit 66ae8c9

Browse files
dpgeorgedpgeorge
committed
py: Tidy up variables in VM, probably fixes subtle bugs.
Things get tricky when using the nlr code to catch exceptions. Need to ensure that the variables (stack layout) in the exception handler are the same as in the bit protected by the exception handler. Prior to this patch there were a few bugs. 1) The constant mp_const_MemoryError_obj was being preloaded to a specific location on the stack at the start of the function. But this location on the stack was being overwritten in the opcode loop (since it didn't think that variable would ever be referenced again), and so when an exception occurred, the variable holding the address of MemoryError was corrupt. 2) The FOR_ITER opcode detection in the exception handler used sp, which may or may not contain the right value coming out of the main opcode loop. With this patch there is a clear separation of variables used in the opcode loop and in the exception handler (should fix issue (2) above). Furthermore, nlr_raise is no longer used in the opcode loop. Instead, it jumps directly into the exception handler. This tells the C compiler more about the possible code flow, and means that it should have the same stack layout for the exception handler. This should fix issue (1) above. Indeed, the generated (ARM) assembler has been checked explicitly, and with 'goto exception_handler', the problem with &MemoryError is fixed. This may now fix problems with rge-sm, and probably many other subtle bugs yet to show themselves. Incidentally, rge-sm now passes on pyboard (with a reduced range of integration)! Main lesson: nlr is tricky. Don't use nlr_push unless you know what you are doing! Luckily, it's not used in many places. Using nlr_raise/jump is fine.
1 parent 8bcb986 commit 66ae8c9

1 file changed

Lines changed: 41 additions & 25 deletions

File tree

‎py/vm.c‎

Lines changed: 41 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -165,8 +165,6 @@ mp_vm_return_kind_t mp_execute_byte_code_2(const byte *code_info, const byte **i
165165
mp_obj_t *fastn, mp_obj_t **sp_in_out,
166166
mp_exc_stack_t *exc_stack, mp_exc_stack_t **exc_sp_in_out,
167167
volatile mp_obj_t inject_exc) {
168-
// careful: be sure to declare volatile any variables read in the exception handler (written is ok, I think)
169-
170168
#if MICROPY_USE_COMPUTED_GOTOS
171169
#include "vmentrytable.h"
172170
#define DISPATCH() do { \
@@ -182,32 +180,43 @@ mp_vm_return_kind_t mp_execute_byte_code_2(const byte *code_info, const byte **i
182180
#define ENTRY_DEFAULT default
183181
#endif
184182

185-
int op = 0;
186-
const byte *ip = *ip_in_out;
187-
mp_obj_t *sp = *sp_in_out;
188-
machine_uint_t unum;
189-
qstr qst;
190-
mp_obj_t obj1, obj2;
191-
nlr_buf_t nlr;
183+
// nlr_raise needs to be implemented as a goto, so that the C compiler's flow analyser
184+
// sees that it's possible for us to jump from the dispatch loop to the exception
185+
// handler. Without this, the code may have a different stack layout in the dispatch
186+
// loop and the exception handler, leading to very obscure bugs.
187+
#define RAISE(o) do { nlr_pop(); nlr.ret_val = o; goto exception_handler; } while(0)
192188

189+
// variables that are visible to the exception handler (declared volatile)
193190
volatile bool currently_in_except_block = MP_TAGPTR_TAG(*exc_sp_in_out); // 0 or 1, to detect nested exceptions
194191
mp_exc_stack_t *volatile exc_sp = MP_TAGPTR_PTR(*exc_sp_in_out); // stack grows up, exc_sp points to top of stack
195-
const byte *volatile save_ip = ip; // this is so we can access ip in the exception handler without making ip volatile (which means the compiler can't keep it in a register in the main loop)
192+
const byte *volatile save_ip = *ip_in_out; // this is so we can access ip in the exception handler without making ip volatile (which means the compiler can't keep it in a register in the main loop)
193+
mp_obj_t *volatile save_sp = *sp_in_out; // this is so we can access sp in the exception handler when needed
196194

197195
// outer exception handling loop
198196
for (;;) {
197+
nlr_buf_t nlr;
199198
outer_dispatch_loop:
200199
if (nlr_push(&nlr) == 0) {
200+
// local variables that are not visible to the exception handler
201+
byte op = 0;
202+
const byte *ip = *ip_in_out;
203+
mp_obj_t *sp = *sp_in_out;
204+
machine_uint_t unum;
205+
qstr qst;
206+
mp_obj_t obj1, obj2;
207+
201208
// If we have exception to inject, now that we finish setting up
202209
// execution context, raise it. This works as if RAISE_VARARGS
203210
// bytecode was executed.
204211
// Injecting exc into yield from generator is a special case,
205212
// handled by MP_BC_YIELD_FROM itself
206213
if (inject_exc != MP_OBJ_NULL && *ip != MP_BC_YIELD_FROM) {
207-
mp_obj_t t = inject_exc;
214+
obj1 = inject_exc;
208215
inject_exc = MP_OBJ_NULL;
209-
nlr_raise(mp_make_raise_obj(t));
216+
obj1 = mp_make_raise_obj(obj1);
217+
RAISE(obj1);
210218
}
219+
211220
// loop to execute byte code
212221
for (;;) {
213222
dispatch_loop:
@@ -297,7 +306,8 @@ mp_vm_return_kind_t mp_execute_byte_code_2(const byte *code_info, const byte **i
297306
load_check:
298307
if (obj1 == MP_OBJ_NULL) {
299308
local_name_error:
300-
nlr_raise(mp_obj_new_exception_msg(&mp_type_NameError, "local variable referenced before assignment"));
309+
obj1 = mp_obj_new_exception_msg(&mp_type_NameError, "local variable referenced before assignment");
310+
RAISE(obj1);
301311
}
302312
PUSH(obj1);
303313
DISPATCH();
@@ -580,7 +590,7 @@ mp_vm_return_kind_t mp_execute_byte_code_2(const byte *code_info, const byte **i
580590
// if TOS is an integer, does something else
581591
// else error
582592
if (mp_obj_is_exception_type(TOP())) {
583-
nlr_raise(sp[-1]);
593+
RAISE(sp[-1]);
584594
}
585595
if (TOP() == mp_const_none) {
586596
sp--;
@@ -606,6 +616,7 @@ mp_vm_return_kind_t mp_execute_byte_code_2(const byte *code_info, const byte **i
606616

607617
ENTRY(MP_BC_FOR_ITER):
608618
DECODE_ULABEL; // the jump offset if iteration finishes; for labels are always forward
619+
save_sp = sp;
609620
obj1 = mp_iternext_allow_raise(TOP());
610621
if (obj1 == MP_OBJ_NULL) {
611622
--sp; // pop the exhausted iterator
@@ -829,12 +840,14 @@ mp_vm_return_kind_t mp_execute_byte_code_2(const byte *code_info, const byte **i
829840
}
830841
}
831842
if (obj1 == MP_OBJ_NULL) {
832-
nlr_raise(mp_obj_new_exception_msg(&mp_type_RuntimeError, "No active exception to reraise"));
843+
obj1 = mp_obj_new_exception_msg(&mp_type_RuntimeError, "No active exception to reraise");
844+
RAISE(obj1);
833845
}
834846
} else {
835847
obj1 = POP();
836848
}
837-
nlr_raise(mp_make_raise_obj(obj1));
849+
obj1 = mp_make_raise_obj(obj1);
850+
RAISE(obj1);
838851

839852
ENTRY(MP_BC_YIELD_VALUE):
840853
yield:
@@ -847,7 +860,7 @@ mp_vm_return_kind_t mp_execute_byte_code_2(const byte *code_info, const byte **i
847860
ENTRY(MP_BC_YIELD_FROM): {
848861
//#define EXC_MATCH(exc, type) MP_OBJ_IS_TYPE(exc, type)
849862
#define EXC_MATCH(exc, type) mp_obj_exception_match(exc, type)
850-
#define GENERATOR_EXIT_IF_NEEDED(t) if (t != MP_OBJ_NULL && EXC_MATCH(t, &mp_type_GeneratorExit)) { nlr_raise(t); }
863+
#define GENERATOR_EXIT_IF_NEEDED(t) if (t != MP_OBJ_NULL && EXC_MATCH(t, &mp_type_GeneratorExit)) { RAISE(t); }
851864
mp_vm_return_kind_t ret_kind;
852865
obj1 = POP();
853866
mp_obj_t t_exc = MP_OBJ_NULL;
@@ -890,7 +903,7 @@ mp_vm_return_kind_t mp_execute_byte_code_2(const byte *code_info, const byte **i
890903
GENERATOR_EXIT_IF_NEEDED(t_exc);
891904
DISPATCH();
892905
} else {
893-
nlr_raise(obj2);
906+
RAISE(obj2);
894907
}
895908
}
896909
}
@@ -912,25 +925,27 @@ mp_vm_return_kind_t mp_execute_byte_code_2(const byte *code_info, const byte **i
912925
DISPATCH();
913926

914927
ENTRY_DEFAULT:
915-
printf("code %p, byte code 0x%02x not implemented\n", ip, op);
916928
obj1 = mp_obj_new_exception_msg(&mp_type_NotImplementedError, "byte code not implemented");
917929
nlr_pop();
918930
fastn[0] = obj1;
919931
return MP_VM_RETURN_EXCEPTION;
932+
920933
#if !MICROPY_USE_COMPUTED_GOTOS
921934
} // switch
922935
#endif
923-
}
936+
} // for loop
924937

925938
} else {
939+
exception_handler:
926940
// exception occurred
927941

928942
// check if it's a StopIteration within a for block
929943
if (*save_ip == MP_BC_FOR_ITER && mp_obj_is_subclass_fast(mp_obj_get_type(nlr.ret_val), &mp_type_StopIteration)) {
930-
ip = save_ip + 1;
944+
const byte *ip = save_ip + 1;
945+
machine_uint_t unum;
931946
DECODE_ULABEL; // the jump offset if iteration finishes; for labels are always forward
932-
--sp; // pop the exhausted iterator
933-
ip += unum; // jump to after for-block
947+
*ip_in_out = ip + unum; // jump to after for-block
948+
*sp_in_out = save_sp - 1; // pop the exhausted iterator
934949
goto outer_dispatch_loop; // continue with dispatch loop
935950
}
936951

@@ -969,14 +984,15 @@ mp_vm_return_kind_t mp_execute_byte_code_2(const byte *code_info, const byte **i
969984
currently_in_except_block = 1;
970985

971986
// catch exception and pass to byte code
972-
sp = MP_TAGPTR_PTR(exc_sp->val_sp);
973-
ip = exc_sp->handler;
987+
*ip_in_out = exc_sp->handler;
988+
mp_obj_t *sp = MP_TAGPTR_PTR(exc_sp->val_sp);
974989
// save this exception in the stack so it can be used in a reraise, if needed
975990
exc_sp->prev_exc = nlr.ret_val;
976991
// push(traceback, exc-val, exc-type)
977992
PUSH(mp_const_none);
978993
PUSH(nlr.ret_val);
979994
PUSH(mp_obj_get_type(nlr.ret_val));
995+
*sp_in_out = sp;
980996

981997
} else {
982998
// propagate exception to higher level

0 commit comments

Comments
 (0)