From b1b3863e8a588aa413d5eb53e76744cf21e6cf40 Mon Sep 17 00:00:00 2001 From: Gareth Rees Date: Mon, 10 Aug 2015 17:22:31 +0100 Subject: [PATCH] Address review comments from rb in Copied from Perforce Change: 188112 ServerID: perforce.ravenbrook.com --- mps/code/ss.c | 17 +++++++----- mps/code/ss.h | 16 ++++++++--- mps/design/ss.txt | 71 ++++++++++++++++++++++++++++++++--------------- 3 files changed, 71 insertions(+), 33 deletions(-) diff --git a/mps/code/ss.c b/mps/code/ss.c index efdad8eda34..c3f61d2d1c4 100644 --- a/mps/code/ss.c +++ b/mps/code/ss.c @@ -8,12 +8,15 @@ * * This is a generic implementation, but it makes assumptions that, * while true on all the platforms we currently (version 1.115) - * support, may not be true on all platforms. + * support, may not be true on all platforms. See + * . * - * .assume.down: The stack grows downwards. + * .assume.desc: The stack is descending (and so stackTop is a lower + * address than stackBot). * - * .assume.bottom: There is no need to scan the word pointed to by - * stackBot. + * .assume.empty: The stack convention is "full" (and so we must scan + * the word pointed to by stackTop but not the word pointed to by + * stackBot). * * .assume.align: Addresses on the stack are aligned to sizeof(Addr). */ @@ -57,12 +60,12 @@ Res StackScan(ScanState ss, Addr *stackBot) AVERT(ScanState, ss); arena = ss->arena; - /* See */ - AVER(arena->scAtArenaEnter); + AVER(arena->scAtArenaEnter != NULL); if (arena->scAtArenaEnter) { res = stackScanInner(arena, ss, stackBot, arena->scAtArenaEnter); } else { - /* Somehow missed saving the context at the entry point: do it now. */ + /* Somehow missed saving the context at the entry point (see + * ): do it now. */ StackContextStruct sc; STACK_CONTEXT_SAVE(&sc); res = stackScanInner(arena, ss, stackBot, &sc); diff --git a/mps/code/ss.h b/mps/code/ss.h index 2eca1104d45..ad7ead0c751 100644 --- a/mps/code/ss.h +++ b/mps/code/ss.h @@ -38,6 +38,7 @@ typedef struct StackContextStruct { /* STACK_CONTEXT_END -- clear context and leave arena */ #define STACK_CONTEXT_END(arena) END; \ + AVER(arena->scAtArenaEnter != NULL); \ arena->scAtArenaEnter = NULL; \ ArenaLeave(arena); \ END @@ -58,9 +59,12 @@ extern Res StackContextScan(ScanState ss, StackContext sc); #if defined(MPS_OS_XC) -/* We call _setjmp rather than setjmp because _setjmp saves only the - * register set and the stack while setjmp also saves the signal mask. - * See _setjmp(2). */ +/* We call _setjmp rather than setjmp because we can be confident what + * it does via the source code at + * , + * and because _setjmp saves only the register set and the stack while + * setjmp also saves the signal mask, which we don't care about. See + * _setjmp(2). */ #define STACK_CONTEXT_SAVE(sc) ((void)_setjmp((sc)->jumpBuffer)) @@ -71,7 +75,11 @@ extern Res StackContextScan(ScanState ss, StackContext sc); #endif /* platform defines */ -/* StackScan -- scan the mutator's stack and registers */ +/* StackScan -- scan the mutator's stack and registers + * + * This must be called between STACK_CONTEXT_BEGIN and + * STACK_CONTEXT_END. + */ extern Res StackScan(ScanState ss, Addr *stackBot); diff --git a/mps/design/ss.txt b/mps/design/ss.txt index b9f5dcae4d9..4786deff432 100644 --- a/mps/design/ss.txt +++ b/mps/design/ss.txt @@ -41,17 +41,30 @@ Requirements _`.req.stack.top`: Must locate the top of the mutator's stack. (This is needed for conservative garbage collection of uncooperative code, -where references might be stored by mutator on its stack, to work.) +where references might be stored by mutator on its stack.) _`.req.stack.bottom.not`: There is no requirement to locate the bottom of the stack. (The mutator supplies this as an argument to ``mps_root_create_reg()``.) +_`.req.stack.platform`: Must implement the platform's stack +conventions. + +_`.req.stack.platform.full-empty`: The implementation must take into +account whether the stack is *full* (the stack pointer points to the +last full location) or *empty* (the stack pointer points to the +first empty location). + +_`.req.stack.platform.desc-asc`: The implementation must take into +account whether the stack is *descending* (top of stack is at a lower +address than bottom of stack) or *ascending* (top of stack is at a +higher address than bottom of stack). + _`.req.registers`: Must locate and scan all references in the mutator's *root registers*, the subset of registers which might contain references that do not also appear on the stack. (This is needed for conservative garbage collection of uncooperative code, -where references might appear in registers, to work.) +where references might appear in registers.) _`.req.entry`: Should save the mutator's context (stack and registers) at the point where it enters the MPS. (This avoids scanning registers @@ -72,7 +85,9 @@ Design ------ _`.sol.entry-points`: To meet `.req.stack.entry`_, the mutator's -registers and stack must be recorded when the mutator enters the MPS. +registers and stack must be recorded when the mutator enters the MPS, +if there is a possibility that the MPS might need to know the mutator +context. _`.sol.entry-points.fragile`: The analysis of which entry points might need to save the context (see `.anal.entry-points`_ below) is fragile. @@ -93,25 +108,34 @@ been spilled onto the stack by the time the MPS is entered, so will be scanned by the stack scan. Floating-point registers and debugging registers do not, as far as we are aware, contain pointers. -_`.sol.setjmp`: The C standard specifies that ``jmp_buf`` -"is an array type suitable for holding the information needed to -restore a calling environment. The environment of a call to the -``setjmp`` macro consists of information sufficient for a call to the -``longjmp`` function to return execution to the correct block and -invocation of that block, were it called recursively." This is what we -need: the jump buffer needs to include the root registers in order to -work as described. (A sufficiently perverse implementation could -defeat this, but it works on all our supported platforms.) +_`.sol.setjmp`: The values in callee-save registers can be found by +calling ``setjmp`` and scanning the ``jmp_buf``. -_`.sol.setjmp.scan`: An implementation can decode the jump -buffer and scan just the root registers, or it can conservatively scan -the whole of the jump buffer. +_`.sol.setjmp.scan`: An implementation can decode the jump buffer and +scan just the root registers, or it can conservatively scan the whole +of the jump buffer. + +_`.sol.setjmp.justify`: The C standard specifies that ``jmp_buf`` "is +an array type suitable for holding the information needed to restore a +calling environment. The environment of a call to the ``setjmp`` macro +consists of information sufficient for a call to the ``longjmp`` +function to return execution to the correct block and invocation of +that block, were it called recursively." We believe that any +reasonable implementation of ``setjmp`` must copy the callee-save +registers into the ``jmp_buf`` in order to work as described. +Otherwise, once the callee-save registers have been overwritten by +other function calls, a ``longjmp`` would result in the callee-save +registers having the wrong values. _`.sol.stack.top`: Similarly, an implementation can decode the jump buffer and get the top of the mutator's stack from the stack pointer register, or it can conservatively use the address of a local variable in the entry point to the MPS. +_`.sol.stack.platform`: As of version 1.115, all supported platforms +are *full* and *descending* so the implementation in ``StackScan`` +assumes this. New platforms must check this assumption. + _`.sol.xc.alternative`: On OS X, we could use ``getcontext()`` from libunwind (see here_), but that produces deprecation warnings and introduces a dependency on that library. @@ -176,9 +200,9 @@ sources at changelevel 187410) showing which entry points might call │ └ArenaCollect │ └mps_arena_collect └TraceStartCollectAll - ├ArenaStep ⤴ - ├ArenaStartCollect ⤴ - └TracePoll ⤴ + ├ArenaStep [see above] + ├ArenaStartCollect [see above] + └TracePoll [see above] So the entry points that need to save the stack context are ``mps_arena_step()``, ``mps_alloc()``, ``mps_ap_fill()``, @@ -212,6 +236,8 @@ Return ``ResOK`` if successful, or another result code if not. See the platform-specific implementation of ``StackContextScan()`` for the exact registers which are scanned. +_`.if.scan.begin-end`: This function must be called between ``STACK_CONTEXT_BEGIN()`` and ``STACK_CONTEXT_END()``. + ``STACK_CONTEXT_SAVE(StackContext sc)`` _`.if.save`: Store the mutator context in the structure ``sc``. @@ -227,8 +253,8 @@ violation of design.mps.config.no-spaghetti_. ``STACK_CONTEXT_BEGIN(Arena arena)`` -_`.if.begin`: Start an MPS operation that needs to know the mutator -context. This macro must be used like this:: +_`.if.begin`: Start an MPS operation that may need to know the mutator +context (see .sol.entry-points). This macro must be used like this:: Res res; STACK_CONTEXT_BEGIN(arena) { @@ -257,8 +283,9 @@ Implementations _`.impl.an`: Generic implementation in ``ssan.c``. Since the C standard does not specify where the callee-save registers appear in -the jump buffer, the whole buffer must be scanned, and the top of the -stack must be taken conservatively from a local variable. +the jump buffer, the whole buffer must be scanned (see +`.sol.setjmp.scan`_), and the top of the stack must be taken +conservatively from a local variable. _`.impl.w3`: Windows implementation in ``ssw3i3mv.c`` and ``ssw3i6mv.c``. We know the layout of the jump buffer used by the