From 991905087f81892016f210cbb3d74fe2600ac95c Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Thu, 13 Apr 2023 10:07:58 +0100 Subject: [PATCH 01/22] Part of making extension callbacks part of the public MPS. * Move type and macro declarations to the public header mps.h. * Move documentation to appropriate sections of manual. (cherry picked from commit b928fa236178fb1bdbe20442c3f53b8e8a545a4b) --- code/mps.h | 13 +++++ manual/source/release.rst | 6 +++ manual/source/topic/arena.rst | 93 +++++++++++++++++++++++++++++++++-- 3 files changed, 109 insertions(+), 3 deletions(-) diff --git a/code/mps.h b/code/mps.h index 4c56265c2e..a2940aa1ac 100644 --- a/code/mps.h +++ b/code/mps.h @@ -120,6 +120,13 @@ typedef mps_addr_t (*mps_fmt_isfwd_t)(mps_addr_t); typedef void (*mps_fmt_pad_t)(mps_addr_t, size_t); typedef mps_addr_t (*mps_fmt_class_t)(mps_addr_t); +/* Callbacks indicating that the arena has extended or contracted. + * These are used to register chunks with RtlInstallFunctionTableCallback + * + * so that the client can unwind the stack through functions in the arena. + */ +typedef void (*mps_arena_extended_t)(mps_arena_t, mps_addr_t, size_t); +typedef void (*mps_arena_contracted_t)(mps_arena_t, mps_addr_t, size_t); /* Keyword argument lists */ @@ -171,6 +178,12 @@ extern const struct mps_key_s _mps_key_ARENA_SIZE; extern const struct mps_key_s _mps_key_ARENA_ZONED; #define MPS_KEY_ARENA_ZONED (&_mps_key_ARENA_ZONED) #define MPS_KEY_ARENA_ZONED_FIELD b +extern const struct mps_key_s _mps_key_arena_extended; +#define MPS_KEY_ARENA_EXTENDED (&_mps_key_arena_extended) +#define MPS_KEY_ARENA_EXTENDED_FIELD fun +extern const struct mps_key_s _mps_key_arena_contracted; +#define MPS_KEY_ARENA_CONTRACTED (&_mps_key_arena_contracted) +#define MPS_KEY_ARENA_CONTRACTED_FIELD fun extern const struct mps_key_s _mps_key_FORMAT; #define MPS_KEY_FORMAT (&_mps_key_FORMAT) #define MPS_KEY_FORMAT_FIELD format diff --git a/manual/source/release.rst b/manual/source/release.rst index cba65bcc3a..6674cf691c 100644 --- a/manual/source/release.rst +++ b/manual/source/release.rst @@ -47,6 +47,12 @@ New features :ref:`topic-scanning-protocol`. This allows the client program to safely update references in the visited objects. +#. A :term:`virtual memory arena` can now be configured to call + functions when it acquires a new chunk of :term:`address space`, + and when it returns a chunk of address space to the operation + system. This is intended to support dynamic function tables in + Windows. See :ref:`topic-arena-extension`. + Interface changes ................. diff --git a/manual/source/topic/arena.rst b/manual/source/topic/arena.rst index 520c8179d1..d87978de8d 100644 --- a/manual/source/topic/arena.rst +++ b/manual/source/topic/arena.rst @@ -139,7 +139,7 @@ Client arenas * :c:macro:`MPS_KEY_ARENA_SIZE` (type :c:type:`size_t`) is its size. - It also accepts three optional keyword arguments: + It also accepts five optional keyword arguments: * :c:macro:`MPS_KEY_COMMIT_LIMIT` (type :c:type:`size_t`) is the maximum amount of memory, in :term:`bytes (1)`, that the MPS @@ -159,6 +159,17 @@ Client arenas may pause the :term:`client program` for. See :c:func:`mps_arena_pause_time_set` for details. + * :c:macro:`MPS_KEY_ARENA_EXTENDED` (type :c:type:`mps_fun_t`) is + a function that will be called when the arena is *extended*: + that is, when it acquires a new chunk of address space from the + operating system. See :ref:`topic-arena-extension` for details. + + * :c:macro:`MPS_KEY_ARENA_CONTRACTED` (type :c:type:`mps_fun_t`) + is a function that will be called when the arena is + *contracted*: that is, when it finishes with a chunk of address + space and returns it to the operating system. See + :ref:`topic-arena-extension` for details. + For example:: MPS_ARGS_BEGIN(args) { @@ -983,8 +994,8 @@ Arena introspection and debugging from MPS-managed memory, then it may attempt to re-enter the MPS, which will fail as the MPS is not re-entrant. - .. |RtlInstallFunctionTableCallback| replace:: ``RtlInstallFunctionTableCallback()`` - .. _RtlInstallFunctionTableCallback: https://msdn.microsoft.com/en-us/library/windows/desktop/ms680595(v=vs.85).aspx + .. |RtlInstallFunctionTableCallback| replace:: :c:func:`RtlInstallFunctionTableCallback` + .. _RtlInstallFunctionTableCallback: https://docs.microsoft.com/en-gb/windows/win32/api/winnt/nf-winnt-rtlinstallfunctiontablecallback If this happens, in order to allow the debugger to finish decoding the call stack, the only remedy is to put the arena @@ -1040,3 +1051,79 @@ Arena introspection and debugging :c:func:`mps_addr_pool`, and to find out which :term:`object format` describes the object at the address, use :c:func:`mps_addr_fmt`. + + +.. index:: + single: arena extension callbacks; introduction + single: extension callbacks; introduction + single: arena contraction callbacks; introduction + single: contraction callbacks; introduction + +.. _topic-arena-extension: + +Arena extension callbacks +------------------------- + +There are situations in which the :term:`client program` needs to be +informed about the chunks of address space that an :term:`arena` is +managing. To support this, the MPS allows the client program to +specify two callback functions when creating a :term:`virtual memory +arena`: one function is called when the arena is *extended* (that is, +when it acquires a new chunk of address space from the operating +system), and the other when the arena is *contracted* (that is, when +it returns a chunk of address space to the operating system). + +The use case that this feature is designed to support is debugging of +dynamically generated code in 64-bit Windows. Microsoft's +documentation for |RtlInstallFunctionTableCallback|_ says: + + Function tables are used on 64-bit Windows to determine how to + unwind or walk the stack. These tables are usually generated by + the compiler and stored as part of the image. However, + applications must provide the function table for dynamically + generated code. + +An application may install a dynamic function table by calling +|RtlInstallFunctionTableCallback|_, passing the region of memory in +which the dynamically generated functions can be found, and may later +delete the table by calling |RtlDeleteFunctionTable|_. + +.. |RtlDeleteFunctionTable| replace:: :c:func:`RtlDeleteFunctionTable` +.. _RtlDeleteFunctionTable: https://docs.microsoft.com/en-gb/windows/win32/api/winnt/nf-winnt-rtldeletefunctiontable + +So if the client program is storing dynamically generated functions in +MPS-managed memory, then it could define callback functions that +install and delete the function table callback for the dynamically +generated code, like this:: + + void arena_extended(mps_arena_t arena, void *base, size_t size) + { + RtlInstallFunctionTableCallback(...); + } + + void arena_contracted(mps_arena_t arena, void *base, size_t size) + { + RtlDeleteFunctionTable(...); + } + +and then pass these two functions using :term:`keyword arguments` to +:c:func:`mps_arena_create_k`:: + + MPS_ARGS_BEGIN(args) { + MPS_ARGS_ADD(args, MPS_KEY_ARENA_EXTENDED, (mps_fun_t)arena_extended); + MPS_ARGS_ADD(args, MPS_KEY_ARENA_CONTRACTED, (mps_fun_t)arena_contracted); + /* ... other keyword arguments ... */ + res = mps_arena_create_k(&arena, mps_arena_class_vm(), args); + } MPS_ARGS_END(args); + +The callback functions receive three arguments: ``arena`` (the arena +being extended or contracted), ``base`` (the base address of the chunk +of address space that has just been acquired from, or is about to be +returned to, the operating system), and ``size`` (the size of the +chunk, in bytes). They must not call any function in the MPS, and must +not access any memory managed by the MPS. + +.. note:: + + Arena extension callbacks are only supported by :term:`virtual + memory arenas`. From fd040edff1076302c97e23b593650e85c9db18de Mon Sep 17 00:00:00 2001 From: Jonathan Holburn Date: Thu, 13 Apr 2023 12:25:15 +0100 Subject: [PATCH 02/22] Add test for arena extend and contract callbacks --- code/commpost.nmk | 3 + code/commpre.nmk | 1 + code/extcon.c | 373 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 377 insertions(+) create mode 100644 code/extcon.c diff --git a/code/commpost.nmk b/code/commpost.nmk index 470db2519b..497f2806c8 100644 --- a/code/commpost.nmk +++ b/code/commpost.nmk @@ -225,6 +225,9 @@ $(PFM)\$(VARIETY)\cvmicv.exe: $(PFM)\$(VARIETY)\cvmicv.obj \ $(PFM)\$(VARIETY)\djbench.exe: $(PFM)\$(VARIETY)\djbench.obj \ $(TESTLIBOBJ) $(TESTTHROBJ) +$(PFM)\$(VARIETY)\extcon.exe: $(PFM)\$(VARIETY)\extcon.obj \ + $(PFM)\$(VARIETY)\mps.lib $(TESTLIBOBJ) + $(PFM)\$(VARIETY)\finalcv.exe: $(PFM)\$(VARIETY)\finalcv.obj \ $(PFM)\$(VARIETY)\mps.lib $(FMTTESTOBJ) $(TESTLIBOBJ) diff --git a/code/commpre.nmk b/code/commpre.nmk index 236e08ad2e..4a41ea5641 100644 --- a/code/commpre.nmk +++ b/code/commpre.nmk @@ -73,6 +73,7 @@ TEST_TARGETS=\ btcv.exe \ bttest.exe \ djbench.exe \ + extcon.exe \ finalcv.exe \ finaltest.exe \ fotest.exe \ diff --git a/code/extcon.c b/code/extcon.c new file mode 100644 index 0000000000..e370928b76 --- /dev/null +++ b/code/extcon.c @@ -0,0 +1,373 @@ +/* extcon.c: ARENA EXTENDED AND CONTRACTED CALLBACK TEST + * + * $Id$ + * Copyright (c) 2014-2022 Ravenbrook Limited. See end of file for license. + * + * .overview: This test case allocates a bunch of large objects, of a size + * similar to the size of the arena, to force the arena to extend. It then + * discards the base pointers to those objects, and forces a collection. + * + * .limitations: This test checks only that the EXTENDED and CONTRACTED + * callbacks were called at least once. It does not check that the + * extensions and contractions themselves were performed correctly, nor + * does it check that an appropriate number of extensions and contractions + * took place, nor does it check that they took place at sensible times. + */ + +#include "mps.h" +#include "testlib.h" +#include "mpsavm.h" +#include "mpscamc.h" +#include +#include +#include + +/* Number of test objects to allocate */ +#define N_TESTOBJ 10 +/* Number of integers in each test object */ +#define N_INT_TESTOBJ 10000 +/* This is the difference in size between */ +#define SIZEDIFF 10 + +/* Set alignment to mps_word_ts */ +#define ALIGNMENT sizeof(mps_word_t) + +/* Align size upwards to the next multiple of the word size. */ +#define ALIGN_WORD(size) \ + (((size) + ALIGNMENT - 1) & ~(ALIGNMENT - 1)) + +/* Object TYPE macro */ +#define TYPE(obj) ((obj)->type.type) + +/* Global objects*/ +static mps_arena_t arena; /* the arena */ +static mps_pool_t obj_pool; /* pool for ordinary objects */ +static mps_ap_t obj_ap; /* allocation point used to allocate objects */ + +/* Count of number of arena contractions and extensions */ +static int n_contract = 0; +static int n_extend = 0; + +/* Union of all object types */ +typedef int type_t; +enum { + TYPE_INTBOX, + TYPE_FWD2, /* two-word forwarding object */ + TYPE_FWD, /* three words and up forwarding object */ + TYPE_PAD1, /* one-word padding object */ + TYPE_PAD /* two words and up padding object */ +}; + +typedef struct type_s { + type_t type; +} type_s; + +typedef union obj_u *obj_t; + +typedef struct fwd2_s { + type_t type; /* TYPE_FWD2 */ + obj_t fwd; /* forwarded object */ +} fwd2_s; + +typedef struct fwd_s { + type_t type; /* TYPE_FWD */ + obj_t fwd; /* forwarded object */ + size_t size; /* total size of this object */ +} fwd_s; + +/* Align size upwards to the next multiple of the word size, and + * additionally ensure that it's big enough to store a forwarding + * pointer. Evaluates its argument twice. */ +#define ALIGN_OBJ(size) \ + (ALIGN_WORD(size) >= ALIGN_WORD(sizeof(fwd_s)) \ + ? ALIGN_WORD(size) \ + : ALIGN_WORD(sizeof(fwd_s))) + +typedef struct pad1_s { + type_t type; /* TYPE_PAD1 */ +} pad1_s; + +typedef struct pad_s { + type_t type; /* TYPE_PAD */ + size_t size; /* total size of this object */ +} pad_s; + + +typedef struct test_alloc_obj +{ + type_t type; + size_t size; + int int_array[N_INT_TESTOBJ]; +} test_alloc_obj_s; + + +typedef union obj_u { + type_s type; + test_alloc_obj_s int_box; + fwd_s fwd; + fwd2_s fwd2; + pad1_s pad1; + pad_s pad; +} obj_s; + +/* Callback functions for arena extension and contraction */ +void arena_extended_cb(mps_arena_t arena_in, mps_addr_t addr, size_t size) +{ + testlib_unused(arena_in); + testlib_unused(addr); + testlib_unused(size); + n_extend++; +} + +void arena_contracted_cb(mps_arena_t arena_in, mps_addr_t addr, size_t size) +{ + testlib_unused(arena_in); + testlib_unused(addr); + testlib_unused(size); + n_contract++; +} + + +/* Format functions */ + +mps_addr_t obj_skip(mps_addr_t addr) +{ + obj_t obj = addr; + + switch (TYPE(obj)) + { + case TYPE_INTBOX: + addr = (char *)addr + ALIGN_OBJ(sizeof(test_alloc_obj_s)); + break; + case TYPE_FWD2: + addr = (char *)addr + ALIGN_WORD(sizeof(fwd2_s)); + break; + case TYPE_FWD: + addr = (char *)addr + ALIGN_WORD(obj->fwd.size); + break; + case TYPE_PAD1: + addr = (char *)addr + ALIGN_WORD(sizeof(pad1_s)); + break; + case TYPE_PAD: + addr = (char *)addr + ALIGN_WORD(obj->pad.size); + break; + default: + printf("invalid type"); + assert(0); + break; + } + + return addr; +} + +void obj_pad(mps_addr_t addr, size_t size) +{ + + obj_t obj = addr; + + assert(size >= ALIGN_WORD(sizeof(pad1_s))); + if (size == ALIGN_WORD(sizeof(pad1_s))) { + TYPE(obj) = TYPE_PAD1; + } else { + TYPE(obj) = TYPE_PAD; + obj->pad.size = size; + } +} + +void obj_fwd(mps_addr_t old, mps_addr_t new) +{ + obj_t obj = old; + mps_addr_t limit = obj_skip(old); + size_t size = (size_t)((char*)limit - (char*)old); + + assert(size >= ALIGN_WORD(sizeof(fwd2_s))); + if (size == ALIGN_WORD(sizeof(fwd2_s))) { + TYPE(obj) = TYPE_FWD2; + obj->fwd2.fwd = new; + } else { + TYPE(obj) = TYPE_FWD; + obj->fwd.fwd = new; + obj->fwd.size = size; + } +} + +mps_addr_t obj_isfwd(mps_addr_t addr) +{ + obj_t obj = addr; + switch (TYPE(obj)) { + case TYPE_FWD2: + return obj->fwd2.fwd; + case TYPE_FWD: + return obj->fwd.fwd; + } + return NULL; +} + +int main(int argc, char* argv[]) +{ + mps_res_t res; + mps_fmt_t obj_fmt; + + mps_thr_t thread; + mps_root_t stack_root; + size_t arena_size, obj_size; + + /* Cold end of stack pointer */ + void *cold = &cold; + + /* [FIXME: do we need a trampoline? is p a root?] */ + mps_addr_t p; + + int i; + test_alloc_obj_s *testobj[N_TESTOBJ]; + + testlib_init(argc, argv); + + /* Make initial arena size slightly bigger than the test object size to force and extension as early as possible */ + obj_size = ALIGN_OBJ(sizeof(test_alloc_obj_s)); + arena_size = ALIGN_OBJ(obj_size + SIZEDIFF); + + /* Create arena and register callbacks */ + MPS_ARGS_BEGIN(args) { + MPS_ARGS_ADD(args, MPS_KEY_ARENA_SIZE, arena_size); + MPS_ARGS_ADD(args, MPS_KEY_ARENA_EXTENDED, (mps_fun_t)&arena_extended_cb); + MPS_ARGS_ADD(args, MPS_KEY_ARENA_CONTRACTED, (mps_fun_t)&arena_contracted_cb); + res = mps_arena_create_k(&arena, mps_arena_class_vm(), args); + } MPS_ARGS_END(args); + + /* Create new fmt */ + MPS_ARGS_BEGIN(args) { + MPS_ARGS_ADD(args, MPS_KEY_FMT_ALIGN, ALIGNMENT); + MPS_ARGS_ADD(args, MPS_KEY_FMT_SKIP, obj_skip); + MPS_ARGS_ADD(args, MPS_KEY_FMT_FWD, obj_fwd); + MPS_ARGS_ADD(args, MPS_KEY_FMT_ISFWD, obj_isfwd); + MPS_ARGS_ADD(args, MPS_KEY_FMT_PAD, obj_pad); + res = mps_fmt_create_k(&obj_fmt, arena, args); + } MPS_ARGS_END(args); + + /* Create new pool */ + MPS_ARGS_BEGIN(args) { + MPS_ARGS_ADD(args, MPS_KEY_FORMAT, obj_fmt); + die(mps_pool_create_k(&obj_pool, arena, mps_class_amcz(), args), + "mps_pool_create_k"); + } MPS_ARGS_END(args); + + /* Register thread */ + die(mps_thread_reg(&thread, arena), "Thread reg"); + + /* Register stack roots */ + die(mps_root_create_thread(&stack_root, arena, thread, cold), "Create Stack root"); + + /* Create allocation point */ + die(mps_ap_create_k(&obj_ap, obj_pool, mps_args_none), "Create Allocation point"); + + /* Allocate objects and force arena extension */ + for (i = 0; i < N_TESTOBJ; i++) { + int j; + test_alloc_obj_s* p_test_obj; + do { + res = mps_reserve(&p, obj_ap, obj_size); + if (res != MPS_RES_OK) + exit(EXIT_FAILURE); + + /* p is now an ambiguous reference to the reserved block */ + testobj[i] = p; + + /* initialize obj */ + p_test_obj = testobj[i]; + p_test_obj->type = TYPE_INTBOX; + p_test_obj->size = obj_size; + + for (j = 0; j < N_INT_TESTOBJ; ++j) { + p_test_obj->int_array[j] = j; + } + } while (!mps_commit(obj_ap, p, obj_size)); /* see note 2 [FIXME:what? where?] */ + /* obj is now valid and managed by the MPS */ + + /* Destroy the local reference to test object*/ + p_test_obj = NULL; + p = NULL; + } + + /* destroy all the references to the objects*/ + for (i = 0; i < N_TESTOBJ; i++) { + + /* !!! JPH bonus test of mps_addr_object - move to separate testbench */ +#if 0 + /* Comment this out as mps_addr_object is unavailable */ + mps_addr_t out; + assert(N_TESTOBJ <= N_INT_TESTOBJ); + + /* [FIXME:explain this, especially use of i] */ + die(mps_addr_object(&out, arena, &(testobj[i])->int_array[i]), "Address object"); + + assert(out == testobj[i]); + + /* end piggy back testbench */ + +#endif + /* now overwrite the ref */ + testobj[i] = NULL; + } + + /* Collect */ + mps_arena_collect(arena); + + printf("Arena extended %d times\n", n_extend); + printf("Arena contracted %d times\n", n_contract); + + /* Clean up */ + /* [FIXME:is park needed] */ + mps_arena_park(arena); + + mps_root_destroy(stack_root); + mps_thread_dereg(thread); + mps_ap_destroy(obj_ap); + mps_pool_destroy(obj_pool); + mps_fmt_destroy(obj_fmt); + mps_arena_destroy(arena); + + + if (n_extend == 0) + printf("%s: No callbacks received upon arena extended!\n", argv[0]); + if (n_contract == 0) + printf("%s: No callbacks received upon arena contracted!\n", argv[0]); + + if (n_contract == 0 || n_extend == 0) + { + printf("%s: Conclusion: Test failed.\n", argv[0]); + return 1; + } + + printf("%s: Conclusion: Failed to find any defects.\n", argv[0]); + return 0; +} + +/* C. COPYRIGHT AND LICENSE + * + * Copyright (C) 2022-2023 Ravenbrook Limited . + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the + * distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS + * IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ From d252cbd7589688c816d0a930840531e95b9da705 Mon Sep 17 00:00:00 2001 From: Jonathan Holburn Date: Thu, 13 Apr 2023 12:34:58 +0100 Subject: [PATCH 03/22] Add extcon.c to testci target so it runs during CI --- tool/testcases.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tool/testcases.txt b/tool/testcases.txt index b19de92d12..6a8e035085 100644 --- a/tool/testcases.txt +++ b/tool/testcases.txt @@ -16,6 +16,7 @@ awlutth =T btcv bttest =N interactive djbench =N benchmark +extcon finalcv =P finaltest =P forktest =X From b4d548562a7226978982c51df51bad473b314585 Mon Sep 17 00:00:00 2001 From: Jonathan Holburn Date: Fri, 14 Apr 2023 10:10:18 +0100 Subject: [PATCH 04/22] Fix test to ensure that cold pointer exists in colder stack frame. This avoids the issue https://github.com/Ravenbrook/mps/issues/210 Also increase the number of test objects by *10 to make it more likely the arena will decide to contract. Also comment out some printfs in the interrupt context, to avoid messy output. Also fix some typos in comments --- code/extcon.c | 69 +++++++++++++++++++++++++++++---------------------- 1 file changed, 39 insertions(+), 30 deletions(-) diff --git a/code/extcon.c b/code/extcon.c index e370928b76..6fab18d579 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -23,7 +23,7 @@ #include /* Number of test objects to allocate */ -#define N_TESTOBJ 10 +#define N_TESTOBJ 100 /* Number of integers in each test object */ #define N_INT_TESTOBJ 10000 /* This is the difference in size between */ @@ -116,6 +116,7 @@ void arena_extended_cb(mps_arena_t arena_in, mps_addr_t addr, size_t size) testlib_unused(arena_in); testlib_unused(addr); testlib_unused(size); + /* printf("Arena extended by %zd bytes\n", size); */ n_extend++; } @@ -124,12 +125,11 @@ void arena_contracted_cb(mps_arena_t arena_in, mps_addr_t addr, size_t size) testlib_unused(arena_in); testlib_unused(addr); testlib_unused(size); + /* printf("Arena contracted by %zd bytes\n", size); */ n_contract++; } - /* Format functions */ - mps_addr_t obj_skip(mps_addr_t addr) { obj_t obj = addr; @@ -203,27 +203,18 @@ mps_addr_t obj_isfwd(mps_addr_t addr) return NULL; } -int main(int argc, char* argv[]) +void test_main(void *cold_stack_end) { mps_res_t res; mps_fmt_t obj_fmt; - mps_thr_t thread; mps_root_t stack_root; size_t arena_size, obj_size; - - /* Cold end of stack pointer */ - void *cold = &cold; - - /* [FIXME: do we need a trampoline? is p a root?] */ mps_addr_t p; - int i; test_alloc_obj_s *testobj[N_TESTOBJ]; - testlib_init(argc, argv); - - /* Make initial arena size slightly bigger than the test object size to force and extension as early as possible */ + /* Make initial arena size slightly bigger than the test object size to force an extension as early as possible */ obj_size = ALIGN_OBJ(sizeof(test_alloc_obj_s)); arena_size = ALIGN_OBJ(obj_size + SIZEDIFF); @@ -256,7 +247,7 @@ int main(int argc, char* argv[]) die(mps_thread_reg(&thread, arena), "Thread reg"); /* Register stack roots */ - die(mps_root_create_thread(&stack_root, arena, thread, cold), "Create Stack root"); + die(mps_root_create_thread(&stack_root, arena, thread, cold_stack_end), "Create Stack root"); /* Create allocation point */ die(mps_ap_create_k(&obj_ap, obj_pool, mps_args_none), "Create Allocation point"); @@ -265,11 +256,15 @@ int main(int argc, char* argv[]) for (i = 0; i < N_TESTOBJ; i++) { int j; test_alloc_obj_s* p_test_obj; + printf("Reserving memory for object %d: ", i); do { res = mps_reserve(&p, obj_ap, obj_size); if (res != MPS_RES_OK) exit(EXIT_FAILURE); + /* Each "*" indicates a single attempt to reserve memory */ + printf("*"); + /* p is now an ambiguous reference to the reserved block */ testobj[i] = p; @@ -281,31 +276,35 @@ int main(int argc, char* argv[]) for (j = 0; j < N_INT_TESTOBJ; ++j) { p_test_obj->int_array[j] = j; } - } while (!mps_commit(obj_ap, p, obj_size)); /* see note 2 [FIXME:what? where?] */ - /* obj is now valid and managed by the MPS */ + } while (!mps_commit(obj_ap, p, obj_size)); + /* testobj[i] is now valid and managed by the MPS */ + + printf("...committed.\n"); - /* Destroy the local reference to test object*/ + /* Overwrite the local references to test objects*/ p_test_obj = NULL; p = NULL; } - /* destroy all the references to the objects*/ + /* overwrite all the references to the objects*/ for (i = 0; i < N_TESTOBJ; i++) { - /* !!! JPH bonus test of mps_addr_object - move to separate testbench */ -#if 0 - /* Comment this out as mps_addr_object is unavailable */ + /* bonus test of mps_addr_object */ +#if 0 /* Comment this out as mps_addr_object is unavailable */ mps_addr_t out; assert(N_TESTOBJ <= N_INT_TESTOBJ); - /* [FIXME:explain this, especially use of i] */ + /* use "i" to as a convenient way to generate different interior pointers + To guarantee the i index will give us an interior pointer the number of test + objects must be <= the number of integers in each object */ + assert(N_TESTOBJ <= N_INT_TESTOBJ); die(mps_addr_object(&out, arena, &(testobj[i])->int_array[i]), "Address object"); assert(out == testobj[i]); /* end piggy back testbench */ - #endif + /* now overwrite the ref */ testobj[i] = NULL; } @@ -329,20 +328,30 @@ int main(int argc, char* argv[]) if (n_extend == 0) - printf("%s: No callbacks received upon arena extended!\n", argv[0]); + printf("No callbacks received upon arena extended!\n"); if (n_contract == 0) - printf("%s: No callbacks received upon arena contracted!\n", argv[0]); + printf("No callbacks received upon arena contracted!\n"); if (n_contract == 0 || n_extend == 0) { - printf("%s: Conclusion: Test failed.\n", argv[0]); - return 1; + exit(EXIT_FAILURE); } +} + +int main(int argc, char* argv[]) +{ + void *stack_marker = &stack_marker; - printf("%s: Conclusion: Failed to find any defects.\n", argv[0]); - return 0; + testlib_init(argc, argv); + + test_main(stack_marker); + + printf("%s: Conclusion: Failed to find any defects.\n", argv[0]); + + return 0; } + /* C. COPYRIGHT AND LICENSE * * Copyright (C) 2022-2023 Ravenbrook Limited . From 7e45b2efd1ad466dff16d80d7e7a474f2a9a2e65 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 14 Apr 2023 10:46:56 +0100 Subject: [PATCH 05/22] Adding extcon extension/contraction test to Posix builds. Fixing warnings in extcon.c. --- code/comm.gmk | 4 ++++ code/extcon.c | 21 +++++++++++---------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/code/comm.gmk b/code/comm.gmk index a2cd68baab..e18e500b91 100644 --- a/code/comm.gmk +++ b/code/comm.gmk @@ -267,6 +267,7 @@ TEST_TARGETS=\ btcv \ bttest \ djbench \ + extcon \ finalcv \ finaltest \ forktest \ @@ -487,6 +488,9 @@ $(PFM)/$(VARIETY)/bttest: $(PFM)/$(VARIETY)/bttest.o \ $(PFM)/$(VARIETY)/djbench: $(PFM)/$(VARIETY)/djbench.o \ $(TESTLIBOBJ) $(TESTTHROBJ) +$(PFM)/$(VARIETY)/extcon: $(PFM)/$(VARIETY)/extcon.o \ + $(TESTLIBOBJ) $(PFM)/$(VARIETY)/mps.a + $(PFM)/$(VARIETY)/finalcv: $(PFM)/$(VARIETY)/finalcv.o \ $(FMTDYTSTOBJ) $(TESTLIBOBJ) $(PFM)/$(VARIETY)/mps.a diff --git a/code/extcon.c b/code/extcon.c index 6fab18d579..392b1b3c37 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -111,7 +111,7 @@ typedef union obj_u { } obj_s; /* Callback functions for arena extension and contraction */ -void arena_extended_cb(mps_arena_t arena_in, mps_addr_t addr, size_t size) +static void arena_extended_cb(mps_arena_t arena_in, mps_addr_t addr, size_t size) { testlib_unused(arena_in); testlib_unused(addr); @@ -120,7 +120,7 @@ void arena_extended_cb(mps_arena_t arena_in, mps_addr_t addr, size_t size) n_extend++; } -void arena_contracted_cb(mps_arena_t arena_in, mps_addr_t addr, size_t size) +static void arena_contracted_cb(mps_arena_t arena_in, mps_addr_t addr, size_t size) { testlib_unused(arena_in); testlib_unused(addr); @@ -130,7 +130,7 @@ void arena_contracted_cb(mps_arena_t arena_in, mps_addr_t addr, size_t size) } /* Format functions */ -mps_addr_t obj_skip(mps_addr_t addr) +static mps_addr_t obj_skip(mps_addr_t addr) { obj_t obj = addr; @@ -160,7 +160,7 @@ mps_addr_t obj_skip(mps_addr_t addr) return addr; } -void obj_pad(mps_addr_t addr, size_t size) +static void obj_pad(mps_addr_t addr, size_t size) { obj_t obj = addr; @@ -174,7 +174,7 @@ void obj_pad(mps_addr_t addr, size_t size) } } -void obj_fwd(mps_addr_t old, mps_addr_t new) +static void obj_fwd(mps_addr_t old, mps_addr_t new) { obj_t obj = old; mps_addr_t limit = obj_skip(old); @@ -191,19 +191,20 @@ void obj_fwd(mps_addr_t old, mps_addr_t new) } } -mps_addr_t obj_isfwd(mps_addr_t addr) +static mps_addr_t obj_isfwd(mps_addr_t addr) { obj_t obj = addr; switch (TYPE(obj)) { case TYPE_FWD2: - return obj->fwd2.fwd; + return obj->fwd2.fwd; case TYPE_FWD: - return obj->fwd.fwd; + return obj->fwd.fwd; + default: + return NULL; } - return NULL; } -void test_main(void *cold_stack_end) +static void test_main(void *cold_stack_end) { mps_res_t res; mps_fmt_t obj_fmt; From 32e21e336ff68f1f9facb5a0593a82641388c7a9 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 14 Apr 2023 10:52:08 +0100 Subject: [PATCH 06/22] Detabifying extcon.c. --- code/extcon.c | 378 +++++++++++++++++++++++++------------------------- 1 file changed, 189 insertions(+), 189 deletions(-) diff --git a/code/extcon.c b/code/extcon.c index 392b1b3c37..d22701bdb8 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -23,7 +23,7 @@ #include /* Number of test objects to allocate */ -#define N_TESTOBJ 100 +#define N_TESTOBJ 1000 /* Number of integers in each test object */ #define N_INT_TESTOBJ 10000 /* This is the difference in size between */ @@ -51,28 +51,28 @@ static int n_extend = 0; /* Union of all object types */ typedef int type_t; enum { - TYPE_INTBOX, - TYPE_FWD2, /* two-word forwarding object */ - TYPE_FWD, /* three words and up forwarding object */ - TYPE_PAD1, /* one-word padding object */ - TYPE_PAD /* two words and up padding object */ + TYPE_INTBOX, + TYPE_FWD2, /* two-word forwarding object */ + TYPE_FWD, /* three words and up forwarding object */ + TYPE_PAD1, /* one-word padding object */ + TYPE_PAD /* two words and up padding object */ }; typedef struct type_s { - type_t type; + type_t type; } type_s; typedef union obj_u *obj_t; typedef struct fwd2_s { - type_t type; /* TYPE_FWD2 */ - obj_t fwd; /* forwarded object */ + type_t type; /* TYPE_FWD2 */ + obj_t fwd; /* forwarded object */ } fwd2_s; typedef struct fwd_s { - type_t type; /* TYPE_FWD */ - obj_t fwd; /* forwarded object */ - size_t size; /* total size of this object */ + type_t type; /* TYPE_FWD */ + obj_t fwd; /* forwarded object */ + size_t size; /* total size of this object */ } fwd_s; /* Align size upwards to the next multiple of the word size, and @@ -84,30 +84,30 @@ typedef struct fwd_s { : ALIGN_WORD(sizeof(fwd_s))) typedef struct pad1_s { - type_t type; /* TYPE_PAD1 */ + type_t type; /* TYPE_PAD1 */ } pad1_s; typedef struct pad_s { - type_t type; /* TYPE_PAD */ - size_t size; /* total size of this object */ + type_t type; /* TYPE_PAD */ + size_t size; /* total size of this object */ } pad_s; typedef struct test_alloc_obj { - type_t type; - size_t size; - int int_array[N_INT_TESTOBJ]; + type_t type; + size_t size; + int int_array[N_INT_TESTOBJ]; } test_alloc_obj_s; typedef union obj_u { - type_s type; - test_alloc_obj_s int_box; - fwd_s fwd; - fwd2_s fwd2; - pad1_s pad1; - pad_s pad; + type_s type; + test_alloc_obj_s int_box; + fwd_s fwd; + fwd2_s fwd2; + pad1_s pad1; + pad_s pad; } obj_s; /* Callback functions for arena extension and contraction */ @@ -126,217 +126,217 @@ static void arena_contracted_cb(mps_arena_t arena_in, mps_addr_t addr, size_t si testlib_unused(addr); testlib_unused(size); /* printf("Arena contracted by %zd bytes\n", size); */ - n_contract++; + n_contract++; } /* Format functions */ static mps_addr_t obj_skip(mps_addr_t addr) { - obj_t obj = addr; - - switch (TYPE(obj)) - { - case TYPE_INTBOX: - addr = (char *)addr + ALIGN_OBJ(sizeof(test_alloc_obj_s)); - break; - case TYPE_FWD2: - addr = (char *)addr + ALIGN_WORD(sizeof(fwd2_s)); - break; - case TYPE_FWD: - addr = (char *)addr + ALIGN_WORD(obj->fwd.size); - break; - case TYPE_PAD1: - addr = (char *)addr + ALIGN_WORD(sizeof(pad1_s)); - break; - case TYPE_PAD: - addr = (char *)addr + ALIGN_WORD(obj->pad.size); - break; - default: - printf("invalid type"); - assert(0); - break; - } - - return addr; + obj_t obj = addr; + + switch (TYPE(obj)) + { + case TYPE_INTBOX: + addr = (char *)addr + ALIGN_OBJ(sizeof(test_alloc_obj_s)); + break; + case TYPE_FWD2: + addr = (char *)addr + ALIGN_WORD(sizeof(fwd2_s)); + break; + case TYPE_FWD: + addr = (char *)addr + ALIGN_WORD(obj->fwd.size); + break; + case TYPE_PAD1: + addr = (char *)addr + ALIGN_WORD(sizeof(pad1_s)); + break; + case TYPE_PAD: + addr = (char *)addr + ALIGN_WORD(obj->pad.size); + break; + default: + printf("invalid type"); + assert(0); + break; + } + + return addr; } static void obj_pad(mps_addr_t addr, size_t size) { - obj_t obj = addr; + obj_t obj = addr; - assert(size >= ALIGN_WORD(sizeof(pad1_s))); - if (size == ALIGN_WORD(sizeof(pad1_s))) { - TYPE(obj) = TYPE_PAD1; - } else { - TYPE(obj) = TYPE_PAD; - obj->pad.size = size; - } + assert(size >= ALIGN_WORD(sizeof(pad1_s))); + if (size == ALIGN_WORD(sizeof(pad1_s))) { + TYPE(obj) = TYPE_PAD1; + } else { + TYPE(obj) = TYPE_PAD; + obj->pad.size = size; + } } static void obj_fwd(mps_addr_t old, mps_addr_t new) { - obj_t obj = old; - mps_addr_t limit = obj_skip(old); - size_t size = (size_t)((char*)limit - (char*)old); - - assert(size >= ALIGN_WORD(sizeof(fwd2_s))); - if (size == ALIGN_WORD(sizeof(fwd2_s))) { - TYPE(obj) = TYPE_FWD2; - obj->fwd2.fwd = new; - } else { - TYPE(obj) = TYPE_FWD; - obj->fwd.fwd = new; - obj->fwd.size = size; - } + obj_t obj = old; + mps_addr_t limit = obj_skip(old); + size_t size = (size_t)((char*)limit - (char*)old); + + assert(size >= ALIGN_WORD(sizeof(fwd2_s))); + if (size == ALIGN_WORD(sizeof(fwd2_s))) { + TYPE(obj) = TYPE_FWD2; + obj->fwd2.fwd = new; + } else { + TYPE(obj) = TYPE_FWD; + obj->fwd.fwd = new; + obj->fwd.size = size; + } } static mps_addr_t obj_isfwd(mps_addr_t addr) { - obj_t obj = addr; - switch (TYPE(obj)) { - case TYPE_FWD2: - return obj->fwd2.fwd; - case TYPE_FWD: - return obj->fwd.fwd; - default: - return NULL; - } + obj_t obj = addr; + switch (TYPE(obj)) { + case TYPE_FWD2: + return obj->fwd2.fwd; + case TYPE_FWD: + return obj->fwd.fwd; + default: + return NULL; + } } static void test_main(void *cold_stack_end) { - mps_res_t res; - mps_fmt_t obj_fmt; - mps_thr_t thread; - mps_root_t stack_root; - size_t arena_size, obj_size; - mps_addr_t p; - int i; - test_alloc_obj_s *testobj[N_TESTOBJ]; - - /* Make initial arena size slightly bigger than the test object size to force an extension as early as possible */ - obj_size = ALIGN_OBJ(sizeof(test_alloc_obj_s)); - arena_size = ALIGN_OBJ(obj_size + SIZEDIFF); - - /* Create arena and register callbacks */ - MPS_ARGS_BEGIN(args) { - MPS_ARGS_ADD(args, MPS_KEY_ARENA_SIZE, arena_size); - MPS_ARGS_ADD(args, MPS_KEY_ARENA_EXTENDED, (mps_fun_t)&arena_extended_cb); - MPS_ARGS_ADD(args, MPS_KEY_ARENA_CONTRACTED, (mps_fun_t)&arena_contracted_cb); - res = mps_arena_create_k(&arena, mps_arena_class_vm(), args); - } MPS_ARGS_END(args); - - /* Create new fmt */ - MPS_ARGS_BEGIN(args) { - MPS_ARGS_ADD(args, MPS_KEY_FMT_ALIGN, ALIGNMENT); - MPS_ARGS_ADD(args, MPS_KEY_FMT_SKIP, obj_skip); - MPS_ARGS_ADD(args, MPS_KEY_FMT_FWD, obj_fwd); - MPS_ARGS_ADD(args, MPS_KEY_FMT_ISFWD, obj_isfwd); - MPS_ARGS_ADD(args, MPS_KEY_FMT_PAD, obj_pad); - res = mps_fmt_create_k(&obj_fmt, arena, args); - } MPS_ARGS_END(args); - - /* Create new pool */ - MPS_ARGS_BEGIN(args) { - MPS_ARGS_ADD(args, MPS_KEY_FORMAT, obj_fmt); - die(mps_pool_create_k(&obj_pool, arena, mps_class_amcz(), args), - "mps_pool_create_k"); - } MPS_ARGS_END(args); - - /* Register thread */ - die(mps_thread_reg(&thread, arena), "Thread reg"); - - /* Register stack roots */ - die(mps_root_create_thread(&stack_root, arena, thread, cold_stack_end), "Create Stack root"); - - /* Create allocation point */ - die(mps_ap_create_k(&obj_ap, obj_pool, mps_args_none), "Create Allocation point"); - - /* Allocate objects and force arena extension */ - for (i = 0; i < N_TESTOBJ; i++) { - int j; - test_alloc_obj_s* p_test_obj; + mps_res_t res; + mps_fmt_t obj_fmt; + mps_thr_t thread; + mps_root_t stack_root; + size_t arena_size, obj_size; + mps_addr_t p; + int i; + test_alloc_obj_s *testobj[N_TESTOBJ]; + + /* Make initial arena size slightly bigger than the test object size to force an extension as early as possible */ + obj_size = ALIGN_OBJ(sizeof(test_alloc_obj_s)); + arena_size = ALIGN_OBJ(obj_size + SIZEDIFF); + + /* Create arena and register callbacks */ + MPS_ARGS_BEGIN(args) { + MPS_ARGS_ADD(args, MPS_KEY_ARENA_SIZE, arena_size); + MPS_ARGS_ADD(args, MPS_KEY_ARENA_EXTENDED, (mps_fun_t)&arena_extended_cb); + MPS_ARGS_ADD(args, MPS_KEY_ARENA_CONTRACTED, (mps_fun_t)&arena_contracted_cb); + res = mps_arena_create_k(&arena, mps_arena_class_vm(), args); + } MPS_ARGS_END(args); + + /* Create new fmt */ + MPS_ARGS_BEGIN(args) { + MPS_ARGS_ADD(args, MPS_KEY_FMT_ALIGN, ALIGNMENT); + MPS_ARGS_ADD(args, MPS_KEY_FMT_SKIP, obj_skip); + MPS_ARGS_ADD(args, MPS_KEY_FMT_FWD, obj_fwd); + MPS_ARGS_ADD(args, MPS_KEY_FMT_ISFWD, obj_isfwd); + MPS_ARGS_ADD(args, MPS_KEY_FMT_PAD, obj_pad); + res = mps_fmt_create_k(&obj_fmt, arena, args); + } MPS_ARGS_END(args); + + /* Create new pool */ + MPS_ARGS_BEGIN(args) { + MPS_ARGS_ADD(args, MPS_KEY_FORMAT, obj_fmt); + die(mps_pool_create_k(&obj_pool, arena, mps_class_amcz(), args), + "mps_pool_create_k"); + } MPS_ARGS_END(args); + + /* Register thread */ + die(mps_thread_reg(&thread, arena), "Thread reg"); + + /* Register stack roots */ + die(mps_root_create_thread(&stack_root, arena, thread, cold_stack_end), "Create Stack root"); + + /* Create allocation point */ + die(mps_ap_create_k(&obj_ap, obj_pool, mps_args_none), "Create Allocation point"); + + /* Allocate objects and force arena extension */ + for (i = 0; i < N_TESTOBJ; i++) { + int j; + test_alloc_obj_s* p_test_obj; printf("Reserving memory for object %d: ", i); - do { - res = mps_reserve(&p, obj_ap, obj_size); - if (res != MPS_RES_OK) - exit(EXIT_FAILURE); + do { + res = mps_reserve(&p, obj_ap, obj_size); + if (res != MPS_RES_OK) + exit(EXIT_FAILURE); /* Each "*" indicates a single attempt to reserve memory */ printf("*"); - /* p is now an ambiguous reference to the reserved block */ - testobj[i] = p; + /* p is now an ambiguous reference to the reserved block */ + testobj[i] = p; - /* initialize obj */ - p_test_obj = testobj[i]; - p_test_obj->type = TYPE_INTBOX; - p_test_obj->size = obj_size; + /* initialize obj */ + p_test_obj = testobj[i]; + p_test_obj->type = TYPE_INTBOX; + p_test_obj->size = obj_size; - for (j = 0; j < N_INT_TESTOBJ; ++j) { - p_test_obj->int_array[j] = j; - } - } while (!mps_commit(obj_ap, p, obj_size)); - /* testobj[i] is now valid and managed by the MPS */ + for (j = 0; j < N_INT_TESTOBJ; ++j) { + p_test_obj->int_array[j] = j; + } + } while (!mps_commit(obj_ap, p, obj_size)); + /* testobj[i] is now valid and managed by the MPS */ printf("...committed.\n"); /* Overwrite the local references to test objects*/ - p_test_obj = NULL; - p = NULL; - } + p_test_obj = NULL; + p = NULL; + } - /* overwrite all the references to the objects*/ - for (i = 0; i < N_TESTOBJ; i++) { + /* overwrite all the references to the objects*/ + for (i = 0; i < N_TESTOBJ; i++) { - /* bonus test of mps_addr_object */ + /* bonus test of mps_addr_object */ #if 0 /* Comment this out as mps_addr_object is unavailable */ mps_addr_t out; - assert(N_TESTOBJ <= N_INT_TESTOBJ); + assert(N_TESTOBJ <= N_INT_TESTOBJ); - /* use "i" to as a convenient way to generate different interior pointers + /* use "i" to as a convenient way to generate different interior pointers To guarantee the i index will give us an interior pointer the number of test objects must be <= the number of integers in each object */ assert(N_TESTOBJ <= N_INT_TESTOBJ); - die(mps_addr_object(&out, arena, &(testobj[i])->int_array[i]), "Address object"); + die(mps_addr_object(&out, arena, &(testobj[i])->int_array[i]), "Address object"); - assert(out == testobj[i]); + assert(out == testobj[i]); - /* end piggy back testbench */ + /* end piggy back testbench */ #endif - /* now overwrite the ref */ - testobj[i] = NULL; - } - - /* Collect */ - mps_arena_collect(arena); - - printf("Arena extended %d times\n", n_extend); - printf("Arena contracted %d times\n", n_contract); - - /* Clean up */ - /* [FIXME:is park needed] */ - mps_arena_park(arena); - - mps_root_destroy(stack_root); - mps_thread_dereg(thread); - mps_ap_destroy(obj_ap); - mps_pool_destroy(obj_pool); - mps_fmt_destroy(obj_fmt); - mps_arena_destroy(arena); - - - if (n_extend == 0) - printf("No callbacks received upon arena extended!\n"); - if (n_contract == 0) - printf("No callbacks received upon arena contracted!\n"); - - if (n_contract == 0 || n_extend == 0) - { - exit(EXIT_FAILURE); - } + /* now overwrite the ref */ + testobj[i] = NULL; + } + + /* Collect */ + mps_arena_collect(arena); + + printf("Arena extended %d times\n", n_extend); + printf("Arena contracted %d times\n", n_contract); + + /* Clean up */ + /* [FIXME:is park needed] */ + mps_arena_park(arena); + + mps_root_destroy(stack_root); + mps_thread_dereg(thread); + mps_ap_destroy(obj_ap); + mps_pool_destroy(obj_pool); + mps_fmt_destroy(obj_fmt); + mps_arena_destroy(arena); + + + if (n_extend == 0) + printf("No callbacks received upon arena extended!\n"); + if (n_contract == 0) + printf("No callbacks received upon arena contracted!\n"); + + if (n_contract == 0 || n_extend == 0) + { + exit(EXIT_FAILURE); + } } int main(int argc, char* argv[]) From 24e4c8614d8d4f956c6332335b6e6bf0e56305b3 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 14 Apr 2023 15:08:27 +0100 Subject: [PATCH 07/22] Adding an assertion that forces gcc not to reorder locals, working around GitHub issue #210 . --- code/extcon.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/code/extcon.c b/code/extcon.c index d22701bdb8..c822800224 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -23,7 +23,7 @@ #include /* Number of test objects to allocate */ -#define N_TESTOBJ 1000 +#define N_TESTOBJ 100 /* Number of integers in each test object */ #define N_INT_TESTOBJ 10000 /* This is the difference in size between */ @@ -215,6 +215,14 @@ static void test_main(void *cold_stack_end) int i; test_alloc_obj_s *testobj[N_TESTOBJ]; + /* The testobj array must be below (on all current Posix platforms) + the cold end of the stack in order for the MPS to scan it. We + have observed a Heisenbug where GCC will inline test_main into + main and lose this condition if the expression below is removed. + This is a problem we are analysing in GitHub issue #210 + . */ + Insist((void *)&testobj[N_TESTOBJ] < cold_stack_end); + /* Make initial arena size slightly bigger than the test object size to force an extension as early as possible */ obj_size = ALIGN_OBJ(sizeof(test_alloc_obj_s)); arena_size = ALIGN_OBJ(obj_size + SIZEDIFF); From 627ded3fc9eb253d2eba8075762be8daced3608e Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 14 Apr 2023 15:32:10 +0100 Subject: [PATCH 08/22] Adding an attribute to test_main to prevent clang 14 from inlining it an reordering locals, causing stack roots to become lost. Working around GitHub issue #210 . --- code/extcon.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/code/extcon.c b/code/extcon.c index c822800224..5ad002b361 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -204,6 +204,7 @@ static mps_addr_t obj_isfwd(mps_addr_t addr) } } +ATTRIBUTE_NOINLINE static void test_main(void *cold_stack_end) { mps_res_t res; @@ -221,7 +222,7 @@ static void test_main(void *cold_stack_end) main and lose this condition if the expression below is removed. This is a problem we are analysing in GitHub issue #210 . */ - Insist((void *)&testobj[N_TESTOBJ] < cold_stack_end); + Insist((void *)&testobj[N_TESTOBJ] <= cold_stack_end); /* Make initial arena size slightly bigger than the test object size to force an extension as early as possible */ obj_size = ALIGN_OBJ(sizeof(test_alloc_obj_s)); From 921a639e58fc5805fa28b03c0177698fe3cf93cb Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 14 Apr 2023 15:32:37 +0100 Subject: [PATCH 09/22] Removing unnecessary arena_park. --- code/extcon.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/code/extcon.c b/code/extcon.c index 5ad002b361..3606839e19 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -326,9 +326,6 @@ static void test_main(void *cold_stack_end) printf("Arena contracted %d times\n", n_contract); /* Clean up */ - /* [FIXME:is park needed] */ - mps_arena_park(arena); - mps_root_destroy(stack_root); mps_thread_dereg(thread); mps_ap_destroy(obj_ap); From 3f7be39ab918cfee4d2a0a2fb9ca9689a4c6485d Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 14 Apr 2023 17:33:16 +0100 Subject: [PATCH 10/22] Adding detailed output to extcon about memory reservation and GC cycles, to try to diagnose intermittent failures in CI. --- code/extcon.c | 69 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 59 insertions(+), 10 deletions(-) diff --git a/code/extcon.c b/code/extcon.c index 3606839e19..fd457d6c2d 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -116,7 +116,7 @@ static void arena_extended_cb(mps_arena_t arena_in, mps_addr_t addr, size_t size testlib_unused(arena_in); testlib_unused(addr); testlib_unused(size); - /* printf("Arena extended by %zd bytes\n", size); */ + printf("Arena extended by %"PRIuLONGEST" bytes\n", (ulongest_t)size); n_extend++; } @@ -125,7 +125,7 @@ static void arena_contracted_cb(mps_arena_t arena_in, mps_addr_t addr, size_t si testlib_unused(arena_in); testlib_unused(addr); testlib_unused(size); - /* printf("Arena contracted by %zd bytes\n", size); */ + printf("Arena contracted by %"PRIuLONGEST" bytes\n", (ulongest_t)size); n_contract++; } @@ -204,6 +204,45 @@ static mps_addr_t obj_isfwd(mps_addr_t addr) } } + +static void print_messages(void) +{ + mps_message_type_t type; + + while (mps_message_queue_type(&type, arena)) { + mps_message_t message; + + cdie(mps_message_get(&message, arena, type), + "get"); + + switch(type) { + case mps_message_type_gc_start(): + printf("GC start at %"PRIuLONGEST": %s\n", + (ulongest_t)mps_message_clock(arena, message), + mps_message_gc_start_why(arena, message)); + break; + + case mps_message_type_gc(): + printf("GC end at %"PRIuLONGEST" " + "condemned %"PRIuLONGEST" " + "not condemned %"PRIuLONGEST" " + "live %"PRIuLONGEST"\n", + (ulongest_t)mps_message_clock(arena, message), + (ulongest_t)mps_message_gc_condemned_size(arena, message), + (ulongest_t)mps_message_gc_not_condemned_size(arena, message), + (ulongest_t)mps_message_gc_live_size(arena, message)); + break; + + default: + cdie(0, "message type"); + break; + } + + mps_message_discard(arena, message); + } +} + + ATTRIBUTE_NOINLINE static void test_main(void *cold_stack_end) { @@ -236,6 +275,8 @@ static void test_main(void *cold_stack_end) res = mps_arena_create_k(&arena, mps_arena_class_vm(), args); } MPS_ARGS_END(args); + printf("Initial reservation %"PRIuLONGEST".\n", (ulongest_t)mps_arena_reserved(arena)); + /* Create new fmt */ MPS_ARGS_BEGIN(args) { MPS_ARGS_ADD(args, MPS_KEY_FMT_ALIGN, ALIGNMENT); @@ -262,19 +303,21 @@ static void test_main(void *cold_stack_end) /* Create allocation point */ die(mps_ap_create_k(&obj_ap, obj_pool, mps_args_none), "Create Allocation point"); + mps_message_type_enable(arena, mps_message_type_gc_start()); + mps_message_type_enable(arena, mps_message_type_gc()); + /* Allocate objects and force arena extension */ for (i = 0; i < N_TESTOBJ; i++) { int j; test_alloc_obj_s* p_test_obj; - printf("Reserving memory for object %d: ", i); + do { + printf("Reserving memory for object %d\n", i); + res = mps_reserve(&p, obj_ap, obj_size); if (res != MPS_RES_OK) exit(EXIT_FAILURE); - /* Each "*" indicates a single attempt to reserve memory */ - printf("*"); - /* p is now an ambiguous reference to the reserved block */ testobj[i] = p; @@ -289,11 +332,16 @@ static void test_main(void *cold_stack_end) } while (!mps_commit(obj_ap, p, obj_size)); /* testobj[i] is now valid and managed by the MPS */ - printf("...committed.\n"); + printf("Object %d committed. " + "Arena reserved: %"PRIuLONGEST".\n", + i, + (ulongest_t)mps_arena_reserved(arena)); /* Overwrite the local references to test objects*/ p_test_obj = NULL; p = NULL; + + print_messages(); } /* overwrite all the references to the objects*/ @@ -317,11 +365,15 @@ static void test_main(void *cold_stack_end) /* now overwrite the ref */ testobj[i] = NULL; + + print_messages(); } /* Collect */ mps_arena_collect(arena); + print_messages(); + printf("Arena extended %d times\n", n_extend); printf("Arena contracted %d times\n", n_contract); @@ -332,7 +384,6 @@ static void test_main(void *cold_stack_end) mps_pool_destroy(obj_pool); mps_fmt_destroy(obj_fmt); mps_arena_destroy(arena); - if (n_extend == 0) printf("No callbacks received upon arena extended!\n"); @@ -340,9 +391,7 @@ static void test_main(void *cold_stack_end) printf("No callbacks received upon arena contracted!\n"); if (n_contract == 0 || n_extend == 0) - { exit(EXIT_FAILURE); - } } int main(int argc, char* argv[]) From a4190fab5f7141fd1a02154e7045640def81ac25 Mon Sep 17 00:00:00 2001 From: Jonathan Holburn Date: Wed, 26 Apr 2023 18:01:53 +0100 Subject: [PATCH 11/22] add output to aid cold end of stack debugging --- code/extcon.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/code/extcon.c b/code/extcon.c index fd457d6c2d..f1e515b971 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -263,6 +263,16 @@ static void test_main(void *cold_stack_end) . */ Insist((void *)&testobj[N_TESTOBJ] <= cold_stack_end); + if ((void *)&testobj[N_TESTOBJ] > cold_stack_end) + { + printf("Cold stack marker invalid!\n"); + } else { + printf("Cold stack marker probably valid.\n"); + } + + + //assert((void *)&testobj[N_TESTOBJ] <= cold_stack_end); + /* Make initial arena size slightly bigger than the test object size to force an extension as early as possible */ obj_size = ALIGN_OBJ(sizeof(test_alloc_obj_s)); arena_size = ALIGN_OBJ(obj_size + SIZEDIFF); @@ -384,7 +394,10 @@ static void test_main(void *cold_stack_end) mps_pool_destroy(obj_pool); mps_fmt_destroy(obj_fmt); mps_arena_destroy(arena); - +#if 0 + printf("&testobj[N_TESTOBJ] = %p\n", (void *)&testobj[N_TESTOBJ]); + printf("cold_stack_end = %p\n", cold_stack_end); +#endif if (n_extend == 0) printf("No callbacks received upon arena extended!\n"); if (n_contract == 0) From f737421b582aa0dd5941c047f49dd40a4c506ec0 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 2 May 2023 15:40:26 +0100 Subject: [PATCH 12/22] Moving the root of objects into a static to avoid problems with the cold end of the stack for now, deferring solution of GitHub Issue #210 . Convertin asserts to Insists so that they are present in hot builds. --- code/extcon.c | 44 +++++++++++++++++++++++++------------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/code/extcon.c b/code/extcon.c index f1e515b971..af1b4fe14d 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -18,7 +18,6 @@ #include "testlib.h" #include "mpsavm.h" #include "mpscamc.h" -#include #include #include @@ -153,7 +152,7 @@ static mps_addr_t obj_skip(mps_addr_t addr) break; default: printf("invalid type"); - assert(0); + Insist(0); break; } @@ -165,7 +164,7 @@ static void obj_pad(mps_addr_t addr, size_t size) obj_t obj = addr; - assert(size >= ALIGN_WORD(sizeof(pad1_s))); + Insist(size >= ALIGN_WORD(sizeof(pad1_s))); if (size == ALIGN_WORD(sizeof(pad1_s))) { TYPE(obj) = TYPE_PAD1; } else { @@ -180,7 +179,7 @@ static void obj_fwd(mps_addr_t old, mps_addr_t new) mps_addr_t limit = obj_skip(old); size_t size = (size_t)((char*)limit - (char*)old); - assert(size >= ALIGN_WORD(sizeof(fwd2_s))); + Insist(size >= ALIGN_WORD(sizeof(fwd2_s))); if (size == ALIGN_WORD(sizeof(fwd2_s))) { TYPE(obj) = TYPE_FWD2; obj->fwd2.fwd = new; @@ -249,11 +248,16 @@ static void test_main(void *cold_stack_end) mps_res_t res; mps_fmt_t obj_fmt; mps_thr_t thread; - mps_root_t stack_root; + mps_root_t stack_root, testobj_root; size_t arena_size, obj_size; mps_addr_t p; int i; - test_alloc_obj_s *testobj[N_TESTOBJ]; + /* In the original version of extcon this was a stack root, but we + observed unreliable failures to do with registering the cold end + of the stack. See GitHub issue #210 + . For now, we + declare this as a separate root. */ + static mps_addr_t testobj[N_TESTOBJ]; /* The testobj array must be below (on all current Posix platforms) the cold end of the stack in order for the MPS to scan it. We @@ -263,15 +267,11 @@ static void test_main(void *cold_stack_end) . */ Insist((void *)&testobj[N_TESTOBJ] <= cold_stack_end); + /* The Insist above ought to prevent an invalid marker message below. */ if ((void *)&testobj[N_TESTOBJ] > cold_stack_end) - { printf("Cold stack marker invalid!\n"); - } else { + else printf("Cold stack marker probably valid.\n"); - } - - - //assert((void *)&testobj[N_TESTOBJ] <= cold_stack_end); /* Make initial arena size slightly bigger than the test object size to force an extension as early as possible */ obj_size = ALIGN_OBJ(sizeof(test_alloc_obj_s)); @@ -310,6 +310,12 @@ static void test_main(void *cold_stack_end) /* Register stack roots */ die(mps_root_create_thread(&stack_root, arena, thread, cold_stack_end), "Create Stack root"); + /* Register ambiguous array of object roots. */ + die(mps_root_create_table(&testobj_root, arena, + mps_rank_ambig(), (mps_rm_t)0, + &testobj[0], N_TESTOBJ), + "root_create_table(testobj)"); + /* Create allocation point */ die(mps_ap_create_k(&obj_ap, obj_pool, mps_args_none), "Create Allocation point"); @@ -319,7 +325,7 @@ static void test_main(void *cold_stack_end) /* Allocate objects and force arena extension */ for (i = 0; i < N_TESTOBJ; i++) { int j; - test_alloc_obj_s* p_test_obj; + test_alloc_obj_s *p_test_obj; do { printf("Reserving memory for object %d\n", i); @@ -336,9 +342,8 @@ static void test_main(void *cold_stack_end) p_test_obj->type = TYPE_INTBOX; p_test_obj->size = obj_size; - for (j = 0; j < N_INT_TESTOBJ; ++j) { + for (j = 0; j < N_INT_TESTOBJ; ++j) p_test_obj->int_array[j] = j; - } } while (!mps_commit(obj_ap, p, obj_size)); /* testobj[i] is now valid and managed by the MPS */ @@ -358,17 +363,17 @@ static void test_main(void *cold_stack_end) for (i = 0; i < N_TESTOBJ; i++) { /* bonus test of mps_addr_object */ -#if 0 /* Comment this out as mps_addr_object is unavailable */ +#if 0 /* Comment this out until mps_addr_object becomes available. */ mps_addr_t out; - assert(N_TESTOBJ <= N_INT_TESTOBJ); + Insist(N_TESTOBJ <= N_INT_TESTOBJ); /* use "i" to as a convenient way to generate different interior pointers To guarantee the i index will give us an interior pointer the number of test objects must be <= the number of integers in each object */ - assert(N_TESTOBJ <= N_INT_TESTOBJ); + Insist(N_TESTOBJ <= N_INT_TESTOBJ); die(mps_addr_object(&out, arena, &(testobj[i])->int_array[i]), "Address object"); - assert(out == testobj[i]); + Insist(out == testobj[i]); /* end piggy back testbench */ #endif @@ -388,6 +393,7 @@ static void test_main(void *cold_stack_end) printf("Arena contracted %d times\n", n_contract); /* Clean up */ + mps_root_destroy(testobj_root); mps_root_destroy(stack_root); mps_thread_dereg(thread); mps_ap_destroy(obj_ap); From 874cb1bd4c7f48c593efd90e390bd951108b9c47 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 2 May 2023 16:02:48 +0100 Subject: [PATCH 13/22] Disabling the Insist comparing the testobj array to the cold end of the stack, since the testobj array is now a static and not comparible. --- code/extcon.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/code/extcon.c b/code/extcon.c index af1b4fe14d..2e62e64497 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -264,14 +264,16 @@ static void test_main(void *cold_stack_end) have observed a Heisenbug where GCC will inline test_main into main and lose this condition if the expression below is removed. This is a problem we are analysing in GitHub issue #210 - . */ + . For now, we + disable this Insist to allow the test to run with a static + testobj array. */ +#if 0 Insist((void *)&testobj[N_TESTOBJ] <= cold_stack_end); - - /* The Insist above ought to prevent an invalid marker message below. */ if ((void *)&testobj[N_TESTOBJ] > cold_stack_end) printf("Cold stack marker invalid!\n"); else printf("Cold stack marker probably valid.\n"); +#endif /* Make initial arena size slightly bigger than the test object size to force an extension as early as possible */ obj_size = ALIGN_OBJ(sizeof(test_alloc_obj_s)); From b4e5256d603b4ce1c06d5a40d3f5a493be443dba Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 2 May 2023 16:48:22 +0100 Subject: [PATCH 14/22] Disable extcon test except on Windows to workaround our inability to maintain the Xcode project for macOS. See GitHub issue #217 . --- tool/testcases.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tool/testcases.txt b/tool/testcases.txt index 6a8e035085..bf5c4e258c 100644 --- a/tool/testcases.txt +++ b/tool/testcases.txt @@ -16,7 +16,7 @@ awlutth =T btcv bttest =N interactive djbench =N benchmark -extcon +extcon =W TODO: Enable when we can update Xcode project. See GitHub issue #217 . finalcv =P finaltest =P forktest =X From 90d54f1e0da23d85748a76b269ac05ac4a48c495 Mon Sep 17 00:00:00 2001 From: Jonathan Holburn Date: Wed, 10 May 2023 13:51:39 +0100 Subject: [PATCH 15/22] remove homebrew format from extcon testbench and replace with dylan fmt. Also disable code relating to stack roots. Also replaces deprecated root_table. Add comments --- code/commpost.nmk | 2 +- code/extcon.c | 217 ++++++---------------------------------------- 2 files changed, 28 insertions(+), 191 deletions(-) diff --git a/code/commpost.nmk b/code/commpost.nmk index 497f2806c8..a94c87b17e 100644 --- a/code/commpost.nmk +++ b/code/commpost.nmk @@ -226,7 +226,7 @@ $(PFM)\$(VARIETY)\djbench.exe: $(PFM)\$(VARIETY)\djbench.obj \ $(TESTLIBOBJ) $(TESTTHROBJ) $(PFM)\$(VARIETY)\extcon.exe: $(PFM)\$(VARIETY)\extcon.obj \ - $(PFM)\$(VARIETY)\mps.lib $(TESTLIBOBJ) + $(PFM)\$(VARIETY)\mps.lib $(FMTTESTOBJ) $(TESTLIBOBJ) $(PFM)\$(VARIETY)\finalcv.exe: $(PFM)\$(VARIETY)\finalcv.obj \ $(PFM)\$(VARIETY)\mps.lib $(FMTTESTOBJ) $(TESTLIBOBJ) diff --git a/code/extcon.c b/code/extcon.c index 2e62e64497..d407649a6c 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -16,6 +16,8 @@ #include "mps.h" #include "testlib.h" +#include "fmtdy.h" +#include "fmtdytst.h" #include "mpsavm.h" #include "mpscamc.h" #include @@ -23,9 +25,9 @@ /* Number of test objects to allocate */ #define N_TESTOBJ 100 -/* Number of integers in each test object */ -#define N_INT_TESTOBJ 10000 -/* This is the difference in size between */ +/* Number of dylan "slots" in each test object */ +#define N_SLOT_TESTOBJ 10000 +/* This is the number of bytes the initial arena is bigger than the test object size */ #define SIZEDIFF 10 /* Set alignment to mps_word_ts */ @@ -33,82 +35,17 @@ /* Align size upwards to the next multiple of the word size. */ #define ALIGN_WORD(size) \ - (((size) + ALIGNMENT - 1) & ~(ALIGNMENT - 1)) - -/* Object TYPE macro */ -#define TYPE(obj) ((obj)->type.type) + (((size) + ALIGNMENT - 1) & ~(ALIGNMENT - 1)) /* Global objects*/ static mps_arena_t arena; /* the arena */ -static mps_pool_t obj_pool; /* pool for ordinary objects */ +static mps_pool_t obj_pool; /* pool for test objects */ static mps_ap_t obj_ap; /* allocation point used to allocate objects */ /* Count of number of arena contractions and extensions */ static int n_contract = 0; static int n_extend = 0; -/* Union of all object types */ -typedef int type_t; -enum { - TYPE_INTBOX, - TYPE_FWD2, /* two-word forwarding object */ - TYPE_FWD, /* three words and up forwarding object */ - TYPE_PAD1, /* one-word padding object */ - TYPE_PAD /* two words and up padding object */ -}; - -typedef struct type_s { - type_t type; -} type_s; - -typedef union obj_u *obj_t; - -typedef struct fwd2_s { - type_t type; /* TYPE_FWD2 */ - obj_t fwd; /* forwarded object */ -} fwd2_s; - -typedef struct fwd_s { - type_t type; /* TYPE_FWD */ - obj_t fwd; /* forwarded object */ - size_t size; /* total size of this object */ -} fwd_s; - -/* Align size upwards to the next multiple of the word size, and - * additionally ensure that it's big enough to store a forwarding - * pointer. Evaluates its argument twice. */ -#define ALIGN_OBJ(size) \ - (ALIGN_WORD(size) >= ALIGN_WORD(sizeof(fwd_s)) \ - ? ALIGN_WORD(size) \ - : ALIGN_WORD(sizeof(fwd_s))) - -typedef struct pad1_s { - type_t type; /* TYPE_PAD1 */ -} pad1_s; - -typedef struct pad_s { - type_t type; /* TYPE_PAD */ - size_t size; /* total size of this object */ -} pad_s; - - -typedef struct test_alloc_obj -{ - type_t type; - size_t size; - int int_array[N_INT_TESTOBJ]; -} test_alloc_obj_s; - - -typedef union obj_u { - type_s type; - test_alloc_obj_s int_box; - fwd_s fwd; - fwd2_s fwd2; - pad1_s pad1; - pad_s pad; -} obj_s; - /* Callback functions for arena extension and contraction */ static void arena_extended_cb(mps_arena_t arena_in, mps_addr_t addr, size_t size) { @@ -128,82 +65,7 @@ static void arena_contracted_cb(mps_arena_t arena_in, mps_addr_t addr, size_t si n_contract++; } -/* Format functions */ -static mps_addr_t obj_skip(mps_addr_t addr) -{ - obj_t obj = addr; - - switch (TYPE(obj)) - { - case TYPE_INTBOX: - addr = (char *)addr + ALIGN_OBJ(sizeof(test_alloc_obj_s)); - break; - case TYPE_FWD2: - addr = (char *)addr + ALIGN_WORD(sizeof(fwd2_s)); - break; - case TYPE_FWD: - addr = (char *)addr + ALIGN_WORD(obj->fwd.size); - break; - case TYPE_PAD1: - addr = (char *)addr + ALIGN_WORD(sizeof(pad1_s)); - break; - case TYPE_PAD: - addr = (char *)addr + ALIGN_WORD(obj->pad.size); - break; - default: - printf("invalid type"); - Insist(0); - break; - } - - return addr; -} - -static void obj_pad(mps_addr_t addr, size_t size) -{ - - obj_t obj = addr; - - Insist(size >= ALIGN_WORD(sizeof(pad1_s))); - if (size == ALIGN_WORD(sizeof(pad1_s))) { - TYPE(obj) = TYPE_PAD1; - } else { - TYPE(obj) = TYPE_PAD; - obj->pad.size = size; - } -} - -static void obj_fwd(mps_addr_t old, mps_addr_t new) -{ - obj_t obj = old; - mps_addr_t limit = obj_skip(old); - size_t size = (size_t)((char*)limit - (char*)old); - - Insist(size >= ALIGN_WORD(sizeof(fwd2_s))); - if (size == ALIGN_WORD(sizeof(fwd2_s))) { - TYPE(obj) = TYPE_FWD2; - obj->fwd2.fwd = new; - } else { - TYPE(obj) = TYPE_FWD; - obj->fwd.fwd = new; - obj->fwd.size = size; - } -} - -static mps_addr_t obj_isfwd(mps_addr_t addr) -{ - obj_t obj = addr; - switch (TYPE(obj)) { - case TYPE_FWD2: - return obj->fwd2.fwd; - case TYPE_FWD: - return obj->fwd.fwd; - default: - return NULL; - } -} - - +/* Messages for testbench debugging */ static void print_messages(void) { mps_message_type_t type; @@ -241,7 +103,8 @@ static void print_messages(void) } } - +/* Disabling inlining is necessary (but perhaps not sufficient) if using stack roots. + See comment below with link to GitHub issue*/ ATTRIBUTE_NOINLINE static void test_main(void *cold_stack_end) { @@ -250,14 +113,13 @@ static void test_main(void *cold_stack_end) mps_thr_t thread; mps_root_t stack_root, testobj_root; size_t arena_size, obj_size; - mps_addr_t p; int i; /* In the original version of extcon this was a stack root, but we observed unreliable failures to do with registering the cold end of the stack. See GitHub issue #210 . For now, we declare this as a separate root. */ - static mps_addr_t testobj[N_TESTOBJ]; + static mps_word_t testobj[N_TESTOBJ]; /* The testobj array must be below (on all current Posix platforms) the cold end of the stack in order for the MPS to scan it. We @@ -276,8 +138,9 @@ static void test_main(void *cold_stack_end) #endif /* Make initial arena size slightly bigger than the test object size to force an extension as early as possible */ - obj_size = ALIGN_OBJ(sizeof(test_alloc_obj_s)); - arena_size = ALIGN_OBJ(obj_size + SIZEDIFF); + /* See definition of make_dylan_vector() in fmtdytst.c for calculation of vector size */ + obj_size = ALIGN_WORD((N_SLOT_TESTOBJ + 2) * sizeof(mps_word_t)); + arena_size = ALIGN_WORD(obj_size + SIZEDIFF); /* Create arena and register callbacks */ MPS_ARGS_BEGIN(args) { @@ -289,15 +152,7 @@ static void test_main(void *cold_stack_end) printf("Initial reservation %"PRIuLONGEST".\n", (ulongest_t)mps_arena_reserved(arena)); - /* Create new fmt */ - MPS_ARGS_BEGIN(args) { - MPS_ARGS_ADD(args, MPS_KEY_FMT_ALIGN, ALIGNMENT); - MPS_ARGS_ADD(args, MPS_KEY_FMT_SKIP, obj_skip); - MPS_ARGS_ADD(args, MPS_KEY_FMT_FWD, obj_fwd); - MPS_ARGS_ADD(args, MPS_KEY_FMT_ISFWD, obj_isfwd); - MPS_ARGS_ADD(args, MPS_KEY_FMT_PAD, obj_pad); - res = mps_fmt_create_k(&obj_fmt, arena, args); - } MPS_ARGS_END(args); + die(dylan_fmt(&obj_fmt, arena), "dylan_fmt()"); /* Create new pool */ MPS_ARGS_BEGIN(args) { @@ -310,13 +165,19 @@ static void test_main(void *cold_stack_end) die(mps_thread_reg(&thread, arena), "Thread reg"); /* Register stack roots */ + /* Since this testbench is currently not using a stack root, #IF 0 this out */ + testlib_unused(cold_stack_end); + testlib_unused(stack_root); +#if 0 die(mps_root_create_thread(&stack_root, arena, thread, cold_stack_end), "Create Stack root"); +#endif /* Register ambiguous array of object roots. */ - die(mps_root_create_table(&testobj_root, arena, + die(mps_root_create_area(&testobj_root, arena, mps_rank_ambig(), (mps_rm_t)0, - &testobj[0], N_TESTOBJ), - "root_create_table(testobj)"); + &testobj[0], &testobj[N_TESTOBJ], + mps_scan_area, NULL), + "root_create_area(testobj)"); /* Create allocation point */ die(mps_ap_create_k(&obj_ap, obj_pool, mps_args_none), "Create Allocation point"); @@ -326,38 +187,14 @@ static void test_main(void *cold_stack_end) /* Allocate objects and force arena extension */ for (i = 0; i < N_TESTOBJ; i++) { - int j; - test_alloc_obj_s *p_test_obj; - - do { - printf("Reserving memory for object %d\n", i); - res = mps_reserve(&p, obj_ap, obj_size); - if (res != MPS_RES_OK) - exit(EXIT_FAILURE); - - /* p is now an ambiguous reference to the reserved block */ - testobj[i] = p; - - /* initialize obj */ - p_test_obj = testobj[i]; - p_test_obj->type = TYPE_INTBOX; - p_test_obj->size = obj_size; - - for (j = 0; j < N_INT_TESTOBJ; ++j) - p_test_obj->int_array[j] = j; - } while (!mps_commit(obj_ap, p, obj_size)); - /* testobj[i] is now valid and managed by the MPS */ + die(make_dylan_vector(&testobj[i], obj_ap, N_SLOT_TESTOBJ), "make_dylan_vector"); printf("Object %d committed. " "Arena reserved: %"PRIuLONGEST".\n", i, (ulongest_t)mps_arena_reserved(arena)); - /* Overwrite the local references to test objects*/ - p_test_obj = NULL; - p = NULL; - print_messages(); } @@ -381,7 +218,7 @@ static void test_main(void *cold_stack_end) #endif /* now overwrite the ref */ - testobj[i] = NULL; + testobj[i] = (mps_word_t)NULL; print_messages(); } @@ -396,7 +233,7 @@ static void test_main(void *cold_stack_end) /* Clean up */ mps_root_destroy(testobj_root); - mps_root_destroy(stack_root); + /* mps_root_destroy(stack_root);*/ mps_thread_dereg(thread); mps_ap_destroy(obj_ap); mps_pool_destroy(obj_pool); From 612c1a363b97d4ec237e1470da55dcaa207a2782 Mon Sep 17 00:00:00 2001 From: Jonathan Holburn Date: Wed, 10 May 2023 13:57:26 +0100 Subject: [PATCH 16/22] add dylan test object to comm.gmk for extcon, missing from last commit --- code/comm.gmk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/comm.gmk b/code/comm.gmk index e18e500b91..3a241d84ff 100644 --- a/code/comm.gmk +++ b/code/comm.gmk @@ -489,7 +489,7 @@ $(PFM)/$(VARIETY)/djbench: $(PFM)/$(VARIETY)/djbench.o \ $(TESTLIBOBJ) $(TESTTHROBJ) $(PFM)/$(VARIETY)/extcon: $(PFM)/$(VARIETY)/extcon.o \ - $(TESTLIBOBJ) $(PFM)/$(VARIETY)/mps.a + $(FMTDYTSTOBJ) $(TESTLIBOBJ) $(PFM)/$(VARIETY)/mps.a $(PFM)/$(VARIETY)/finalcv: $(PFM)/$(VARIETY)/finalcv.o \ $(FMTDYTSTOBJ) $(TESTLIBOBJ) $(PFM)/$(VARIETY)/mps.a From d9087b1e32f104e2fe2a6b9a1524f8d3e68372bd Mon Sep 17 00:00:00 2001 From: Jonathan Holburn Date: Wed, 10 May 2023 14:05:36 +0100 Subject: [PATCH 17/22] consistently use die() in extcon.c --- code/extcon.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/code/extcon.c b/code/extcon.c index d407649a6c..b8de4db751 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -108,7 +108,6 @@ static void print_messages(void) ATTRIBUTE_NOINLINE static void test_main(void *cold_stack_end) { - mps_res_t res; mps_fmt_t obj_fmt; mps_thr_t thread; mps_root_t stack_root, testobj_root; @@ -147,7 +146,7 @@ static void test_main(void *cold_stack_end) MPS_ARGS_ADD(args, MPS_KEY_ARENA_SIZE, arena_size); MPS_ARGS_ADD(args, MPS_KEY_ARENA_EXTENDED, (mps_fun_t)&arena_extended_cb); MPS_ARGS_ADD(args, MPS_KEY_ARENA_CONTRACTED, (mps_fun_t)&arena_contracted_cb); - res = mps_arena_create_k(&arena, mps_arena_class_vm(), args); + die(mps_arena_create_k(&arena, mps_arena_class_vm(), args), "mps_arena_create_k"); } MPS_ARGS_END(args); printf("Initial reservation %"PRIuLONGEST".\n", (ulongest_t)mps_arena_reserved(arena)); From 74d77c80f639f384f799506fc19c2dc15f09b94b Mon Sep 17 00:00:00 2001 From: Jonathan Holburn Date: Tue, 23 May 2023 02:15:02 +0100 Subject: [PATCH 18/22] executing proc.review.edit --- code/extcon.c | 3 ++- code/mps.h | 4 ++-- manual/source/topic/arena.rst | 28 ++++++++++++++++++++++------ 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/code/extcon.c b/code/extcon.c index b8de4db751..3040488df3 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -232,12 +232,13 @@ static void test_main(void *cold_stack_end) /* Clean up */ mps_root_destroy(testobj_root); - /* mps_root_destroy(stack_root);*/ + /* mps_root_destroy(stack_root);*/ /*commented out while not using stack root */ mps_thread_dereg(thread); mps_ap_destroy(obj_ap); mps_pool_destroy(obj_pool); mps_fmt_destroy(obj_fmt); mps_arena_destroy(arena); + /* comment out some diagnostics for investigating issue #210 mentioned above */ #if 0 printf("&testobj[N_TESTOBJ] = %p\n", (void *)&testobj[N_TESTOBJ]); printf("cold_stack_end = %p\n", cold_stack_end); diff --git a/code/mps.h b/code/mps.h index a2940aa1ac..646bcdfb1a 100644 --- a/code/mps.h +++ b/code/mps.h @@ -125,8 +125,8 @@ typedef mps_addr_t (*mps_fmt_class_t)(mps_addr_t); * * so that the client can unwind the stack through functions in the arena. */ -typedef void (*mps_arena_extended_t)(mps_arena_t, mps_addr_t, size_t); -typedef void (*mps_arena_contracted_t)(mps_arena_t, mps_addr_t, size_t); +typedef void (*mps_arena_extendedar_t)(mps_arena_t, void *, size_t); +typedef void (*mps_arena_contracted_t)(mps_arena_t, void *, size_t); /* Keyword argument lists */ diff --git a/manual/source/topic/arena.rst b/manual/source/topic/arena.rst index d87978de8d..50e8ae29f3 100644 --- a/manual/source/topic/arena.rst +++ b/manual/source/topic/arena.rst @@ -160,14 +160,15 @@ Client arenas :c:func:`mps_arena_pause_time_set` for details. * :c:macro:`MPS_KEY_ARENA_EXTENDED` (type :c:type:`mps_fun_t`) is - a function that will be called when the arena is *extended*: - that is, when it acquires a new chunk of address space from the - operating system. See :ref:`topic-arena-extension` for details. + a function that will be called immediately after the arena is + *extended*: that is, just after it acquires a new chunk of address + space from the operating system. See :ref:`topic-arena-extension` + for details. * :c:macro:`MPS_KEY_ARENA_CONTRACTED` (type :c:type:`mps_fun_t`) - is a function that will be called when the arena is - *contracted*: that is, when it finishes with a chunk of address - space and returns it to the operating system. See + is a function that will be called immediately before the arena is + *contracted*: that is, just before it finishes with a chunk of + address space and returns it to the operating system. See :ref:`topic-arena-extension` for details. For example:: @@ -1125,5 +1126,20 @@ not access any memory managed by the MPS. .. note:: + The extenstion callback is also called immediately after the arena + is created, in other words, the creation of the arena is treated as + a special example of an extension of the arena. + + Unlike the extension callback, the contraction callback is not called + when the arena is destroyed. + + Every contraction of the arena will match one-to-one with the arena + extensions that have already taken place. After creation, any + contractions performed by the arena will be the same size as the + extensions that have already taken place. Contractions never occur as + amalgamations nor as fractions of previous arena extensions. + If an arena has never extended beyond its original size, it will never + call the contraction callback. + Arena extension callbacks are only supported by :term:`virtual memory arenas`. From b8ac0d1a8c339c8867db03340d81d14ca34bc693 Mon Sep 17 00:00:00 2001 From: Jonathan Holburn Date: Tue, 23 May 2023 02:20:51 +0100 Subject: [PATCH 19/22] correction to last commit --- code/mps.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/mps.h b/code/mps.h index 646bcdfb1a..96214c8966 100644 --- a/code/mps.h +++ b/code/mps.h @@ -125,7 +125,7 @@ typedef mps_addr_t (*mps_fmt_class_t)(mps_addr_t); * * so that the client can unwind the stack through functions in the arena. */ -typedef void (*mps_arena_extendedar_t)(mps_arena_t, void *, size_t); +typedef void (*mps_arena_extended_t)(mps_arena_t, void *, size_t); typedef void (*mps_arena_contracted_t)(mps_arena_t, void *, size_t); /* Keyword argument lists */ From fb93d8616d8b5db175037d201230c6bbc28ec067 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 9 Jun 2023 14:29:55 +0100 Subject: [PATCH 20/22] Adding arena contraction callback to all chunk deallocations, so that it is called when the arena is destroyed. --- code/arenavm.c | 19 ++++++++++++------- code/extcon.c | 11 ++++++++--- manual/source/topic/arena.rst | 6 ++---- 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/code/arenavm.c b/code/arenavm.c index 19bc8d82a5..50708e9563 100644 --- a/code/arenavm.c +++ b/code/arenavm.c @@ -428,6 +428,10 @@ static Bool vmChunkDestroy(Tree tree, void *closure) { Chunk chunk; VMChunk vmChunk; + Arena arena; + Addr base; + Size size; + VMArena vmArena; AVERT(Tree, tree); AVER(closure == UNUSED_POINTER); @@ -437,8 +441,14 @@ static Bool vmChunkDestroy(Tree tree, void *closure) AVERT(Chunk, chunk); vmChunk = Chunk2VMChunk(chunk); AVERT(VMChunk, vmChunk); + arena = ChunkArena(chunk); + vmArena = MustBeA(VMArena, arena); + base = chunk->base; + size = ChunkSize(chunk); - (void)vmArenaUnmapSpare(ChunkArena(chunk), ChunkSize(chunk), chunk); + (*vmArena->contracted)(arena, base, size); + + (void)vmArenaUnmapSpare(arena, size, chunk); SparseArrayFinish(&vmChunk->pages); @@ -778,6 +788,7 @@ static void VMArenaDestroy(Arena arena) * */ arena->primary = NULL; TreeTraverseAndDelete(&arena->chunkTree, vmChunkDestroy, UNUSED_POINTER); + AVER(arena->chunkTree == TreeEMPTY); /* Must wait until the chunks are destroyed, since vmChunkDestroy calls vmArenaUnmapSpare which uses the spare land. */ @@ -1223,7 +1234,6 @@ static Bool vmChunkCompact(Tree tree, void *closure) { Chunk chunk; Arena arena = closure; - VMArena vmArena = MustBeA(VMArena, arena); AVERT(Tree, tree); @@ -1232,11 +1242,6 @@ static Bool vmChunkCompact(Tree tree, void *closure) if(chunk != arena->primary && BTIsResRange(chunk->allocTable, 0, chunk->pages)) { - Addr base = chunk->base; - Size size = ChunkSize(chunk); - /* Callback before destroying the chunk, as the arena is (briefly) - invalid afterwards. See job003893. */ - (*vmArena->contracted)(arena, base, size); vmChunkDestroy(tree, UNUSED_POINTER); return TRUE; } else { diff --git a/code/extcon.c b/code/extcon.c index 3040488df3..4cd410d27a 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -227,9 +227,6 @@ static void test_main(void *cold_stack_end) print_messages(); - printf("Arena extended %d times\n", n_extend); - printf("Arena contracted %d times\n", n_contract); - /* Clean up */ mps_root_destroy(testobj_root); /* mps_root_destroy(stack_root);*/ /*commented out while not using stack root */ @@ -238,6 +235,14 @@ static void test_main(void *cold_stack_end) mps_pool_destroy(obj_pool); mps_fmt_destroy(obj_fmt); mps_arena_destroy(arena); + + /* Destroying the arena should cause contraction callbacks on all + remaining chunks, even if they had contents. */ + Insist(n_extend == n_contract); + + printf("Arena extended %d times\n", n_extend); + printf("Arena contracted %d times\n", n_contract); + /* comment out some diagnostics for investigating issue #210 mentioned above */ #if 0 printf("&testobj[N_TESTOBJ] = %p\n", (void *)&testobj[N_TESTOBJ]); diff --git a/manual/source/topic/arena.rst b/manual/source/topic/arena.rst index 50e8ae29f3..5ca0dbbd0c 100644 --- a/manual/source/topic/arena.rst +++ b/manual/source/topic/arena.rst @@ -1130,16 +1130,14 @@ not access any memory managed by the MPS. is created, in other words, the creation of the arena is treated as a special example of an extension of the arena. - Unlike the extension callback, the contraction callback is not called - when the arena is destroyed. + The contraction callback is called on all remaining chunks when + the arena is destroyed. There will be at least one callback. Every contraction of the arena will match one-to-one with the arena extensions that have already taken place. After creation, any contractions performed by the arena will be the same size as the extensions that have already taken place. Contractions never occur as amalgamations nor as fractions of previous arena extensions. - If an arena has never extended beyond its original size, it will never - call the contraction callback. Arena extension callbacks are only supported by :term:`virtual memory arenas`. From 2833d4505593271d6ddc6052d5871c2d7c936b58 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 9 Jun 2023 14:43:41 +0100 Subject: [PATCH 21/22] Improving comments in response to review . --- code/extcon.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/code/extcon.c b/code/extcon.c index 4cd410d27a..430a4259a4 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -1,17 +1,21 @@ /* extcon.c: ARENA EXTENDED AND CONTRACTED CALLBACK TEST * * $Id$ - * Copyright (c) 2014-2022 Ravenbrook Limited. See end of file for license. + * Copyright (c) 2022-2023 Ravenbrook Limited. See end of file for license. * * .overview: This test case allocates a bunch of large objects, of a size * similar to the size of the arena, to force the arena to extend. It then * discards the base pointers to those objects, and forces a collection. * - * .limitations: This test checks only that the EXTENDED and CONTRACTED - * callbacks were called at least once. It does not check that the - * extensions and contractions themselves were performed correctly, nor - * does it check that an appropriate number of extensions and contractions - * took place, nor does it check that they took place at sensible times. + * .limitations: This test checks that the EXTENDED and CONTRACTED + * callbacks were called at least once, and that they are called the + * same number of times. It does not check that the extensions and + * contractions themselves were performed correctly, nor does it check + * that an appropriate number of extensions and contractions took + * place, nor does it check that they took place at sensible times. + * + * .dylan: This test uses Dylan format objects in common with most + * other tests for convenience and brevity. */ #include "mps.h" From a20116a9a28fbd06e2772214d483287e571e9529 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 9 Jun 2023 14:49:32 +0100 Subject: [PATCH 22/22] Clarifying comments in response to review --- code/extcon.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/code/extcon.c b/code/extcon.c index 430a4259a4..4bd04a14b2 100644 --- a/code/extcon.c +++ b/code/extcon.c @@ -29,9 +29,10 @@ /* Number of test objects to allocate */ #define N_TESTOBJ 100 -/* Number of dylan "slots" in each test object */ +/* The number of slots determines the size of each object */ #define N_SLOT_TESTOBJ 10000 -/* This is the number of bytes the initial arena is bigger than the test object size */ +/* The initial arena size is requested to be bigger the test object by + this many bytes */ #define SIZEDIFF 10 /* Set alignment to mps_word_ts */