From 3f6647f3ae44fb9467703d4979d05e9ffaec90f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89milie=20Feral?= Date: Thu, 14 May 2020 16:22:51 +0200 Subject: [PATCH] [ion][python] Implement an architecture-dependant collect_registers. setjmp is not guaranteed to collect all registers without modification on all platforms. This fixes the following bug: when the pointer of a newly allocated object on the Python heap is stored in rpb registers on x86_64 arch, it was not collected by the garbarge collector. --- ion/include/ion.h | 4 ++ ion/src/device/Makefile | 2 + ion/src/shared/collect_registers.cpp | 15 ++++++ ion/src/simulator/android/Makefile | 2 + ion/src/simulator/ios/Makefile | 2 + ion/src/simulator/linux/Makefile | 2 + ion/src/simulator/macos/Makefile | 2 + .../simulator/shared/collect_registers.cpp | 17 +++++++ .../shared/collect_registers_x86_64.s | 25 +++++++++ ion/src/simulator/web/Makefile | 2 + ion/src/simulator/windows/Makefile | 2 + liba/include/setjmp.h | 3 +- python/port/port.cpp | 51 ++++++++----------- 13 files changed, 98 insertions(+), 31 deletions(-) create mode 100644 ion/src/shared/collect_registers.cpp create mode 100644 ion/src/simulator/shared/collect_registers.cpp create mode 100644 ion/src/simulator/shared/collect_registers_x86_64.s diff --git a/ion/include/ion.h b/ion/include/ion.h index ac327bbac..45a8e5559 100644 --- a/ion/include/ion.h +++ b/ion/include/ion.h @@ -17,6 +17,7 @@ #include #include #include +#include /* ION is not your regular library. It is a library you link against, but it * will take care of configuring the whole environment for you. In POSIX terms, @@ -48,6 +49,9 @@ void decompress(const uint8_t * src, uint8_t * dst, int srcSize, int dstSize); // Tells whether the stack pointer is within acceptable bounds bool stackSafe(); +// Collect registers in a buffer and returns the stack pointer +uintptr_t collectRegisters(jmp_buf regs); + } #endif diff --git a/ion/src/device/Makefile b/ion/src/device/Makefile index 4a3a96426..b4f6a6b11 100644 --- a/ion/src/device/Makefile +++ b/ion/src/device/Makefile @@ -10,6 +10,8 @@ ifeq ($(EPSILON_TELEMETRY),1) ion_src += ion/src/shared/telemetry_console.cpp endif +ion_src += ion/src/shared/collect_registers.cpp + ION_DEVICE_SFLAGS = -Iion/src/device/$(MODEL) -Iion/src/device/shared $(call object_for,$(ion_device_src) $(ion_device_flasher_src) $(ion_device_bench_src)): SFLAGS += $(ION_DEVICE_SFLAGS) diff --git a/ion/src/shared/collect_registers.cpp b/ion/src/shared/collect_registers.cpp new file mode 100644 index 000000000..2083f6969 --- /dev/null +++ b/ion/src/shared/collect_registers.cpp @@ -0,0 +1,15 @@ +#include +#include + +namespace Ion { + +/* Forbid inlining to ensure dummy to be at the top of the stack. Otherwise, + * LTO inlining can make regs lower on the stack than some just-allocated + * pointers. */ +__attribute__((noinline))uintptr_t collectRegisters(jmp_buf buf) { + setjmp(buf); + int dummy; + return (uintptr_t)&dummy; +} + +} diff --git a/ion/src/simulator/android/Makefile b/ion/src/simulator/android/Makefile index 9143b3dfe..d3fb36236 100644 --- a/ion/src/simulator/android/Makefile +++ b/ion/src/simulator/android/Makefile @@ -7,6 +7,8 @@ ion_src += $(addprefix ion/src/simulator/shared/, \ dummy/language.cpp \ ) +ion_src += ion/src/shared/collect_registers.cpp + ifeq ($(EPSILON_TELEMETRY),1) ion_src += ion/src/simulator/android/src/cpp/telemetry.cpp endif diff --git a/ion/src/simulator/ios/Makefile b/ion/src/simulator/ios/Makefile index 2d85d1297..334e1d4d1 100644 --- a/ion/src/simulator/ios/Makefile +++ b/ion/src/simulator/ios/Makefile @@ -7,6 +7,8 @@ ion_src += $(addprefix ion/src/simulator/shared/, \ dummy/callback.cpp \ ) +ion_src += ion/src/shared/collect_registers.cpp + $(call object_for,ion/src/simulator/shared/main.cpp) : SFLAGS += -DEPSILON_SDL_FULLSCREEN=1 ifeq ($(EPSILON_TELEMETRY),1) diff --git a/ion/src/simulator/linux/Makefile b/ion/src/simulator/linux/Makefile index f6cae470e..aa8500471 100644 --- a/ion/src/simulator/linux/Makefile +++ b/ion/src/simulator/linux/Makefile @@ -15,6 +15,8 @@ ion_src += $(addprefix ion/src/simulator/linux/, \ ion_src += $(addprefix ion/src/simulator/shared/, \ dummy/callback.cpp \ + collect_registers_x86_64.s \ + collect_registers.cpp \ ) ifeq ($(EPSILON_TELEMETRY),1) diff --git a/ion/src/simulator/macos/Makefile b/ion/src/simulator/macos/Makefile index 81b06f3d4..b00459f75 100644 --- a/ion/src/simulator/macos/Makefile +++ b/ion/src/simulator/macos/Makefile @@ -5,6 +5,8 @@ ion_src += $(addprefix ion/src/simulator/macos/, \ ion_src += $(addprefix ion/src/simulator/shared/, \ apple/language.m \ dummy/callback.cpp \ + collect_registers_x86_64.s \ + collect_registers.cpp \ ) ifeq ($(EPSILON_TELEMETRY),1) diff --git a/ion/src/simulator/shared/collect_registers.cpp b/ion/src/simulator/shared/collect_registers.cpp new file mode 100644 index 000000000..4fa4c86fd --- /dev/null +++ b/ion/src/simulator/shared/collect_registers.cpp @@ -0,0 +1,17 @@ +#include + +extern "C" { + +// define in assembly code +// Force the name as archs (linux/macos) don't mangle C names the same way +extern uintptr_t collect_registers(uintptr_t * regs) asm ("_collect_registers"); +} +namespace Ion { + +// Wrapper to avoid +uintptr_t collectRegisters(jmp_buf buf) { + uintptr_t * regs = (uintptr_t *)buf; + return collect_registers(regs); +} + +} diff --git a/ion/src/simulator/shared/collect_registers_x86_64.s b/ion/src/simulator/shared/collect_registers_x86_64.s new file mode 100644 index 000000000..4740681b4 --- /dev/null +++ b/ion/src/simulator/shared/collect_registers_x86_64.s @@ -0,0 +1,25 @@ +.text + +.global _collect_registers + +_collect_registers: + pushq %r15 + pushq %r14 + pushq %r13 + pushq %r12 + pushq %rbp + pushq %rbx + movq %rbx, (%rdi) + movq %rbp, 8(%rdi) + movq %r12, 16(%rdi) + popq %rbx + popq %rbp + popq %r12 + movq %r13, 24(%rdi) + movq %r14, 32(%rdi) + popq %r13 + movq %r15, 40(%rdi) + popq %r14 + popq %r15 + movq %rsp, %rax + ret diff --git a/ion/src/simulator/web/Makefile b/ion/src/simulator/web/Makefile index 89f41927a..61a7641dc 100644 --- a/ion/src/simulator/web/Makefile +++ b/ion/src/simulator/web/Makefile @@ -22,6 +22,8 @@ ion_src += $(addprefix ion/src/simulator/shared/, \ dummy/language.cpp \ ) +ion_src += ion/src/shared/collect_registers.cpp + ifeq ($(EPSILON_TELEMETRY),1) ion_src += ion/src/simulator/shared/dummy/telemetry_init.cpp ion_src += ion/src/shared/telemetry_console.cpp diff --git a/ion/src/simulator/windows/Makefile b/ion/src/simulator/windows/Makefile index 897db8dca..50c6396ac 100644 --- a/ion/src/simulator/windows/Makefile +++ b/ion/src/simulator/windows/Makefile @@ -8,6 +8,8 @@ ion_src += $(addprefix ion/src/simulator/shared/, \ dummy/callback.cpp \ ) +ion_src += ion/src/shared/collect_registers.cpp + ifeq ($(EPSILON_TELEMETRY),1) ion_src += ion/src/simulator/shared/dummy/telemetry_init.cpp ion_src += ion/src/shared/telemetry_console.cpp diff --git a/liba/include/setjmp.h b/liba/include/setjmp.h index 6a7ccf2e4..826b0cf5a 100644 --- a/liba/include/setjmp.h +++ b/liba/include/setjmp.h @@ -1,6 +1,7 @@ #ifndef LIBA_SETJMP_H #define LIBA_SETJMP_H +#include #include "private/macros.h" /* We are preseving registers: @@ -14,7 +15,7 @@ LIBA_BEGIN_DECLS -typedef int jmp_buf[31]; +typedef uintptr_t jmp_buf[31]; void longjmp(jmp_buf env, int val); int setjmp(jmp_buf env); diff --git a/python/port/port.cpp b/python/port/port.cpp index d0d1f7224..fd86492db 100644 --- a/python/port/port.cpp +++ b/python/port/port.cpp @@ -1,6 +1,6 @@ #include "port.h" -#include +#include #include #include @@ -231,63 +231,54 @@ KDColor MicroPython::ColorParser::ParseColor(mp_obj_t input, ColorMode ColorMode mp_raise_TypeError("Color couldn't be parsed"); } -/* Forbid inlining to ensure regs to be at the top of the stack. Otherwise, - * LTO inlining can make regs lower on the stack than some just-allocated - * pointers. */ -__attribute__((noinline)) void gc_collect(void) { +void gc_collect_regs_and_stack(void) { + // get the registers and the sp + jmp_buf regs; + uintptr_t sp = Ion::collectRegisters(regs); + void * python_stack_top = MP_STATE_THREAD(stack_top); assert(python_stack_top != NULL); - gc_collect_start(); - - modturtle_gc_collect(); - modpyplot_gc_collect(); - - /* get the registers. - * regs is the also the last object on the stack so the stack is bound by - * ®s and python_stack_top. */ - jmp_buf regs; - /* TODO: we use setjmp to get the registers values to look for python heap - * root. However, the 'setjmp' does not guarantee that it gets all registers - * values. We should check our setjmp implementation for the device and - * ensure that it also works for other platforms. */ - setjmp(regs); - - void **regs_ptr = (void**)®s; - /* On the device, the stack is stored in reverse order, but it might not be * the case on a computer. We thus have to take the absolute value of the * addresses difference. */ size_t stackLengthInByte; void ** scanStart; - if ((uintptr_t)python_stack_top > (uintptr_t)regs_ptr) { + if ((uintptr_t)python_stack_top > sp) { /* To compute the stack length: - * regs + * registers * <-----------> * STACK <- ...| | | | | |--|--|--|--| | | | | | | - * ^®s ^python_stack_top + * ^sp ^python_stack_top * */ - stackLengthInByte = (uintptr_t)python_stack_top - (uintptr_t)regs_ptr; - scanStart = regs_ptr; + stackLengthInByte = (uintptr_t)python_stack_top - sp; + scanStart = (void **)sp; } else { /* When computing the stack length, take into account regs' size. - * regs + * registers * <-----------> * STACK -> | | | | | | | | | | | |--|--|--|--| | | |... - * ^python_stack_top ^®s + * ^python_stack_top ^sp * */ - stackLengthInByte = (uintptr_t)regs_ptr - (uintptr_t)python_stack_top + sizeof(regs); + stackLengthInByte = sp - (uintptr_t)python_stack_top + sizeof(regs); scanStart = (void **)python_stack_top; } /* Memory error detectors might find an error here as they might split regs * and stack memory zones. */ MicroPython::collectRootsAtAddress((char *)scanStart, stackLengthInByte); +} + +void gc_collect(void) { + gc_collect_start(); + modturtle_gc_collect(); + modpyplot_gc_collect(); + gc_collect_regs_and_stack(); gc_collect_end(); }