From 9883d8e818feba112935676eb5aa4ce211d7779c Mon Sep 17 00:00:00 2001 From: Damien George Date: Mon, 27 Jul 2020 23:52:38 +1000 Subject: [PATCH] py/persistentcode: Maintain root ptr list of imported native .mpy code. On ports where normal heap memory can contain executable code (eg ARM-based ports such as stm32), native code loaded from an .mpy file may be reclaimed by the GC because there's no reference to the very start of the native machine code block that is reachable from root pointers (only pointers to internal parts of the machine code block are reachable, but that doesn't help the GC find the memory). This commit fixes this issue by maintaining an explicit list of root pointers pointing to native code that is loaded from an .mpy file. This is not needed for all ports so is selectable by the new configuration option MICROPY_PERSISTENT_CODE_TRACK_RELOC_CODE. It's enabled by default if a port does not specify any special functions to allocate or commit executable memory. A test is included to test that native code loaded from an .mpy file does not get reclaimed by the GC. Fixes #6045. Signed-off-by: Damien George --- py/mpconfig.h | 12 +++ py/mpstate.h | 5 + py/persistentcode.c | 14 ++- py/runtime.c | 4 + tests/micropython/import_mpy_native_gc.py | 91 +++++++++++++++++++ tests/micropython/import_mpy_native_gc.py.exp | 1 + 6 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 tests/micropython/import_mpy_native_gc.py create mode 100644 tests/micropython/import_mpy_native_gc.py.exp diff --git a/py/mpconfig.h b/py/mpconfig.h index 287b15aaef..3d9ce8bb55 100644 --- a/py/mpconfig.h +++ b/py/mpconfig.h @@ -351,6 +351,18 @@ // Convenience definition for whether any native or inline assembler emitter is enabled #define MICROPY_EMIT_MACHINE_CODE (MICROPY_EMIT_NATIVE || MICROPY_EMIT_INLINE_ASM) +// Whether native relocatable code loaded from .mpy files is explicitly tracked +// so that the GC cannot reclaim it. Needed on architectures that allocate +// executable memory on the MicroPython heap and don't explicitly track this +// data some other way. +#ifndef MICROPY_PERSISTENT_CODE_TRACK_RELOC_CODE +#if !MICROPY_EMIT_MACHINE_CODE || defined(MP_PLAT_ALLOC_EXEC) || defined(MP_PLAT_COMMIT_EXEC) +#define MICROPY_PERSISTENT_CODE_TRACK_RELOC_CODE (0) +#else +#define MICROPY_PERSISTENT_CODE_TRACK_RELOC_CODE (1) +#endif +#endif + /*****************************************************************************/ /* Compiler configuration */ diff --git a/py/mpstate.h b/py/mpstate.h index 5f6cf55936..2519c77e2d 100644 --- a/py/mpstate.h +++ b/py/mpstate.h @@ -167,6 +167,11 @@ typedef struct _mp_state_vm_t { mp_obj_dict_t *mp_module_builtins_override_dict; #endif + #if MICROPY_PERSISTENT_CODE_TRACK_RELOC_CODE + // An mp_obj_list_t that tracks relocated native code to prevent the GC from reclaiming them. + mp_obj_t track_reloc_code_list; + #endif + // include any root pointers defined by a port MICROPY_PORT_ROOT_POINTERS diff --git a/py/persistentcode.c b/py/persistentcode.c index 386ea49477..da3234a5fe 100644 --- a/py/persistentcode.c +++ b/py/persistentcode.c @@ -3,7 +3,7 @@ * * The MIT License (MIT) * - * Copyright (c) 2013-2016 Damien P. George + * Copyright (c) 2013-2020 Damien P. George * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -516,6 +516,18 @@ STATIC mp_raw_code_t *load_raw_code(mp_reader_t *reader, qstr_window_t *qw) { fun_data = MP_PLAT_COMMIT_EXEC(fun_data, fun_data_len, opt_ri); #else if (prelude.scope_flags & MP_SCOPE_FLAG_VIPERRELOC) { + #if MICROPY_PERSISTENT_CODE_TRACK_RELOC_CODE + // If native code needs relocations then it's not guaranteed that a pointer to + // the head of `buf` (containing the machine code) will be retained for the GC + // to trace. This is because native functions can start inside `buf` and so + // it's possible that the only GC-reachable pointers are pointers inside `buf`. + // So put this `buf` on a list of reachable root pointers. + if (MP_STATE_PORT(track_reloc_code_list) == MP_OBJ_NULL) { + MP_STATE_PORT(track_reloc_code_list) = mp_obj_new_list(0, NULL); + } + mp_obj_list_append(MP_STATE_PORT(track_reloc_code_list), MP_OBJ_FROM_PTR(fun_data)); + #endif + // Do the relocations. mp_native_relocate(&ri, fun_data, (uintptr_t)fun_data); } #endif diff --git a/py/runtime.c b/py/runtime.c index 78da386919..38da2f4538 100644 --- a/py/runtime.c +++ b/py/runtime.c @@ -106,6 +106,10 @@ void mp_init(void) { MP_STATE_VM(mp_module_builtins_override_dict) = NULL; #endif + #if MICROPY_PERSISTENT_CODE_TRACK_RELOC_CODE + MP_STATE_VM(track_reloc_code_list) = MP_OBJ_NULL; + #endif + #if MICROPY_PY_OS_DUPTERM for (size_t i = 0; i < MICROPY_PY_OS_DUPTERM; ++i) { MP_STATE_VM(dupterm_objs[i]) = MP_OBJ_NULL; diff --git a/tests/micropython/import_mpy_native_gc.py b/tests/micropython/import_mpy_native_gc.py new file mode 100644 index 0000000000..e8fac8f179 --- /dev/null +++ b/tests/micropython/import_mpy_native_gc.py @@ -0,0 +1,91 @@ +# Test that native code loaded from a .mpy file is retained after a GC. + +try: + import gc, sys, uio, uos + + sys.implementation.mpy + uio.IOBase + uos.mount +except (ImportError, AttributeError): + print("SKIP") + raise SystemExit + + +class UserFile(uio.IOBase): + def __init__(self, data): + self.data = memoryview(data) + self.pos = 0 + + def readinto(self, buf): + n = min(len(buf), len(self.data) - self.pos) + buf[:n] = self.data[self.pos : self.pos + n] + self.pos += n + return n + + def ioctl(self, req, arg): + return 0 + + +class UserFS: + def __init__(self, files): + self.files = files + + def mount(self, readonly, mksfs): + pass + + def umount(self): + pass + + def stat(self, path): + if path in self.files: + return (32768, 0, 0, 0, 0, 0, 0, 0, 0, 0) + raise OSError + + def open(self, path, mode): + return UserFile(self.files[path]) + + +# Pre-compiled examples/natmod/features0 example for various architectures, keyed +# by the required value of sys.implementation.mpy. +features0_file_contents = { + # -march=x64 -mcache-lookup-bc + 0xB05: b'M\x05\x0b\x1f \x84b\xe9/\x00\x00\x00SH\x8b\x1ds\x00\x00\x00\xbe\x02\x00\x00\x00\xffS\x18\xbf\x01\x00\x00\x00H\x85\xc0u\x0cH\x8bC \xbe\x02\x00\x00\x00[\xff\xe0H\x0f\xaf\xf8H\xff\xc8\xeb\xe6ATUSH\x8b\x1dA\x00\x00\x00H\x8b\x7f\x08L\x8bc(A\xff\xd4H\x8d5\x1f\x00\x00\x00H\x89\xc5H\x8b\x05-\x00\x00\x00\x0f\xb78\xffShH\x89\xefA\xff\xd4H\x8b\x03[]A\\\xc3\x00\x00\x00\x00\x00\x00\x00\x00\x00\x05\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x90\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01\x84@\x12factorial\x10\x00\x00\r \x01"\x9f\x1c\x01\x1e\xff', + # -march=armv7m + 0x1605: b"M\x05\x16\x1f \x84\x12\x1a\xe0\x00\x00\x13\xb5\nK\nJ{D\x9cX\x02!\xe3h\x98G\x03F\x01 3\xb9\x02!#i\x01\x93\x02\xb0\xbd\xe8\x10@\x18GXC\x01;\xf4\xe7\x00\xbfj\x00\x00\x00\x00\x00\x00\x00\xf8\xb5\tN\tK~D\xf4X@hgi\xb8G\x05F\x07K\x07I\xf2XyD\x10\x88ck\x98G(F\xb8G h\xf8\xbd6\x00\x00\x00\x00\x00\x00\x00\x04\x00\x00\x00\x1c\x00\x00\x00\x00\x00\x00\x00\x05\x00\x00\x00\x00\x00\x00\x00\x80\x00\x00\x00\x00\x00\x00\x00\x01\x84\x00\x12factorial\x10\x00\x00\r<\x01>\x9f8\x01:\xff", +} + +# Populate other armv7m-derived archs based on armv7m. +for arch in (0x1A05, 0x1E05, 0x2205): + features0_file_contents[arch] = features0_file_contents[0x1605] + +if sys.implementation.mpy not in features0_file_contents: + print("SKIP") + raise SystemExit + +# These are the test .mpy files. +user_files = {"/features0.mpy": features0_file_contents[sys.implementation.mpy]} + +# Create and mount a user filesystem. +uos.mount(UserFS(user_files), "/userfs") +sys.path.append("/userfs") + +# Import the native function. +gc.collect() +from features0 import factorial + +# Free the module that contained the function. +del sys.modules["features0"] + +# Run a GC cycle which should reclaim the module but not the function. +gc.collect() + +# Allocate lots of fragmented memory to overwrite anything that was just freed by the GC. +for i in range(1000): + [] + +# Run the native function, it should not have been freed or overwritten. +print(factorial(10)) + +# Unmount and undo path addition. +uos.umount("/userfs") +sys.path.pop() diff --git a/tests/micropython/import_mpy_native_gc.py.exp b/tests/micropython/import_mpy_native_gc.py.exp new file mode 100644 index 0000000000..3fbd4a8698 --- /dev/null +++ b/tests/micropython/import_mpy_native_gc.py.exp @@ -0,0 +1 @@ +3628800