Apply all changes from Configura to the public MPS. #76

gareth-rees · 2022-01-28T19:54:16Z

These changes were originally implemented as custom work for Configura on the custom/cet/main branch. With permission from the customer (see e-mail from Göran Rydqvist) we apply them to the public MPS, and integrate the custom documentation and interfaces into the public manual and headers.

The custom interfaces are as follows:

The Transforms interface provides a mechanism to rewrite references throughout the MPS-managed heap;
The Arena extension/contraction callbacks provide a mechanism for the client to register stack decoding callbacks on Windows managed runtime;
The hash arrays flag for allocation points prevents the MPS from aggressively collecting address-based hash tables.

These changes were originally implemented as custom work for Configura on the custom/cet/main branch. With permission from the customer (see e-mail https://info.ravenbrook.com/mail/2022/01/18/12-53-38/0/ from Göran Rydqvist) we apply them to the public MPS, subject to future work to integrate the custom documentation and interfaces into the public manual and headers. The custom interfaces are as follows: 1. The Transforms interface provides a mechanism to rewrite references throughout the MPS-managed heap; 2. The Arena extension/contraction callbacks provide a mechanism for the client to register stack decoding callbacks on Windows managed runtime; 3. The hash arrays flag for allocation points prevents the MPS from aggressively collecting address-based hash tables.

* Move transforms sources to the core sections of the makefiles. * Move function declarations to the public header mps.h. * Update copyright notices for transforms code. * Remove references to Configura from the comments. * Remove trailing whitespace. * Translate design to reStructuredText. * Move documentation to reference section of manual. * Add warning about unsuitability when ambiguous references may exist.

* Move type and macro declarations to the public header mps.h. * Move documentation to appropriate sections of manual.

gareth-rees

Some review to be going on with.

gareth-rees · 2022-01-28T20:01:31Z

code/protw3.c

@@ -113,15 +113,16 @@ LONG WINAPI ProtSEHfilter(LPEXCEPTION_POINTERS info)

 /* ProtSetup -- set up the protection system */

+static void *vehHandle;


This needs a comment to explain why it's global. I can only guess that this is to assist with debugging? The name should start with prot since it belongs to the prot module.

JPH to change variable name and investigate reason for making global

gareth-rees · 2022-01-28T21:03:08Z

code/mpsitr.c

+  AVER(mps_old_list != NULL);
+  AVER(mps_new_list != NULL);
+  /* count: cannot check */


Should check the public names (starting mps_) or the internal names consistently.

gareth-rees · 2022-01-29T12:33:15Z

code/trans.c

+     destroyed */
+  if (transform->oldToNew != NULL)
+    CHECKD(Table, transform->oldToNew);
+  CHECKL(BoolCheck(transform->aborted));


I think we can also check that an aborted transform has a valid table.

CHECKL(!transform->aborted || transform->oldToNew != NULL);

gareth-rees · 2022-01-29T12:37:26Z

code/trans.c

+                    transformTableAlloc,
+                    transformTableFree,
+                    transform,
+                    0, 1); /* these can't be old references */


I think we should have constants for these values, something like this:

#define TransformUnusedKey ((TableKey)0) #define TransformDeletedKey ((TableKey)1)

gareth-rees · 2022-01-29T12:40:32Z

code/trans.c

+    {
+      Seg seg;
+      AVER(SegOfAddr(&seg, transform->arena, old_list[i]));
+    }


We could assert that the old ref is not equal to TransformUnusedKey or TransformDeletedKey here.

gareth-rees · 2022-01-29T13:00:27Z

code/trans.c

+  /* Condemn generation containing seg if not already condemned. */
+  gen = PoolSegPoolGen(SegPool(seg), seg)->gen;
+  AVERT(GenDesc, gen);
+  if (RingIsSingle(&gen->trace[trace->ti].traceRing))


Use TraceSetIsMember(gen->activeTraces, trace) here to avoid needing to know so many details of the data structures.

gareth-rees · 2022-01-29T13:01:47Z

code/trans.c

+  Arena arena;
+  Globals globals;
+  Trace trace;
+  double mortality;


mortality does not seem to be used anywhere.

gareth-rees · 2022-01-29T13:03:15Z

code/trans.c

+    /* I'm not sure why the interface is defined this way. RB 2012-08-03 */
+    TransformDestroy(transform);


This is an annoying wart on the interface — I think we should fix this before making the interface public, even though this requires the customer to update their code.

gareth-rees · 2022-01-29T13:05:39Z

code/trans.c

+  trace->fix = transformFix;
+  trace->fixClosure = transform;
+
+  res = TraceStart(trace, 1.0, 0.0);


These estimates of the mortality and finishing time do not seem very good! Since this is non-incremental, we don't need accurate estimates, but maybe there is a better way to communicate that to TraceStart

gareth-rees · 2022-01-29T13:06:53Z

code/trans.c

+  /* Force the trace to complete now. */
+  ArenaPark(globals);


Since the arena ends up parked, we need to update the arena state documentation to mentation that mps_transform_apply() parks the arena.

gareth-rees

Some more comments. I haven't looked very closely at ztfm.c since it is a test case.

gareth-rees · 2022-01-29T18:53:04Z

design/transform.txt

+* How the implementation is an add-on to the MPS.
+
+* How the code is kept separate from the mainstream MPS.


These points no longer need to be written.

gareth-rees · 2022-01-29T18:54:59Z

design/transform.txt

+
+* How the code is kept separate from the mainstream MPS.
+
+* Why it has its own hash table implementation (no good reason).


I don't think this is true any more.

gareth-rees · 2022-01-29T18:55:47Z

manual/source/topic/transform.rst

+    If the transform was successfully applied, it is destroyed, as if
+    :c:func:`mps_transform_destroy` had been called.


It would be nice to avoid this (see above).

gareth-rees · 2022-01-29T19:26:30Z

procedure/release-build.rst

-#. Edit the index of releases (``release/index.html``) and add the
+#. Edit the index of releases (``release/index.*``) and add the
   release to the table, in a manner consistent with previous releases.

-#. Edit the index of versions (``version/index.html``) and add the
+#. Edit the index of versions (``version/index.*``) and add the


I don't think this change makes sense — the indexes for the public MPS are still in HTML format.

gareth-rees · 2022-01-29T19:28:22Z

code/ztfm.c

+    mps_transform_destroy(t1);
+    after(myrootExact, perset, 1, -10, 2, +10);
+
+    /* Two transforms, both live [-- not supported yet.  RHSK 2010-12-16] */


It is true that multiple transforms are not supported yet?

JPH investigate

gareth-rees · 2022-01-29T19:31:11Z

code/ztfm.c

+/* testscriptB -- create pools and objects; call testscriptC
+ *
+ * Is called via mps_tramp, so matches mps_tramp_t function prototype,
+ * and use trampDataStruct to pass parameters.
+ */
+
+typedef struct trampDataStruct {
+  mps_arena_t arena;
+  mps_thr_t thr;
+  const char *script;
+} trampDataStruct;


We don't use mps_tramp any more, so this needs to be updated.

gareth-rees · 2022-01-29T19:32:32Z

code/ztfm.c

+  trampData.arena = arena;
+  trampData.thr = thr;
+  trampData.script = script;
+  testscriptB(&trampData, sizeof trampData);


This is the only call of testscriptB so perhaps it could be inlined here.

gareth-rees · 2022-01-29T19:33:35Z

code/ztfm.c

+  /* 1<<19 == 524288 == 1/2 Mebibyte */
+  /* 16<<20 == 16777216 == 16 Mebibyte */
+
+  testscriptA("Arena(size 500000000), "


This is the only call to testscriptA so perhaps it could be inlined here.

thejayps · 2022-03-18T14:43:34Z

code/commpost.nmk

-$(PFM)\$(VARIETY)\cvmicv.exe: $(PFM)\$(VARIETY)\cvmicv.obj \
-	$(PFM)\$(VARIETY)\mps.lib $(FMTTESTOBJ) $(TESTLIBOBJ)


cvmicv.c contains coverage tests for the CVM interface, since the code being tested is now all part of the mainline, should not these coverage tests be migrated as well?

rptb1 · 2023-01-13T13:40:19Z

Do we want to make Configura's MPS synchronized with the public MPS for simplicity, ease of delivery, etc.? I understand from @thejayps that some editing may have occurred in this work and that he had to re-add (without source control) work to do with interior pointers back to the code base.

rptb1 · 2023-01-13T14:14:28Z

We have to be extraordinarily careful about this. It affects thousands of real users. I'm working with @thejayps to locate the ends of all the worms in this particular can.

rptb1 · 2023-02-20T09:06:11Z

Blocked pending work planned in https://info.ravenbrook.com/mail/2023/02/06/10-44-45/0/ , which includes a full risk assessment and double-checking of this work.

thejayps · 2023-02-20T14:39:39Z

We have assigned this as priority "optional" since we are currently researching the associated issue in another way. This pull request could become of higher importance later.

rptb1 · 2023-06-12T13:22:31Z

Added "pending" label because @gareth-rees ' comments require actions as if they were from a formal review.

rptb1 · 2023-11-06T10:26:50Z

This work is subsumed by #213 , #214 , #215 , and #230 and so we will not merge this branch. Postmortem was identified as a requirement but was already implemented in a compatible way in the public MPS.

gareth-rees added 3 commits January 23, 2022 18:03

Make hash arrays and extension callbacks part of the public MPS.

b928fa2

* Move type and macro declarations to the public header mps.h. * Move documentation to appropriate sections of manual.

gareth-rees requested a review from thejayps January 28, 2022 19:55

gareth-rees commented Jan 29, 2022

View reviewed changes

thejayps reviewed Mar 18, 2022

View reviewed changes

rptb1 self-assigned this Jan 11, 2023

rptb1 self-requested a review January 13, 2023 13:15

This was linked to issues Jan 13, 2023

Ravenbrook MPS sources are out of sync with Configura's sources #110

Open

Public MPS source code does not include transforms #111

Closed

rptb1 mentioned this pull request Jan 20, 2023

The public MPS has no process for maintaining confidential extensions #126

Open

rptb1 added blocked Unable to proceed. See comment for reason. high risk This work is or would be of high risk of introducing defects. labels Feb 20, 2023

thejayps added the optional Will cause failures / of benefit. Worth assigning resources. label Feb 20, 2023

rptb1 added this to the version 1.118 milestone Feb 22, 2023

rptb1 mentioned this pull request Apr 13, 2023

Make extension callbacks part of the public MPS #213

Merged

thejayps mentioned this pull request Jun 2, 2023

Allow clients to implement large hash arrays without a collection-rehash feedback loop in the public MPS #230

Merged

rptb1 added the pending Something needs doing, even if closed. label Jun 12, 2023

thejayps mentioned this pull request Jun 15, 2023

Make transforms part of the public MPS #214

Merged

rptb1 closed this Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply all changes from Configura to the public MPS. #76

Apply all changes from Configura to the public MPS. #76

gareth-rees commented Jan 28, 2022

gareth-rees left a comment

gareth-rees Jan 28, 2022 •

edited

Loading

thejayps Mar 18, 2022

gareth-rees Jan 28, 2022

thejayps Mar 18, 2022

gareth-rees Jan 29, 2022

gareth-rees Jan 29, 2022

thejayps Mar 18, 2022

gareth-rees Jan 29, 2022

thejayps Mar 18, 2022

gareth-rees Jan 29, 2022

thejayps Mar 18, 2022

gareth-rees Jan 29, 2022

gareth-rees Jan 29, 2022

gareth-rees Jan 29, 2022 •

edited

Loading

gareth-rees Jan 29, 2022

gareth-rees left a comment

gareth-rees Jan 29, 2022

gareth-rees Jan 29, 2022

gareth-rees Jan 29, 2022

gareth-rees Jan 29, 2022

gareth-rees Jan 29, 2022

thejayps Mar 18, 2022

gareth-rees Jan 29, 2022

gareth-rees Jan 29, 2022

gareth-rees Jan 29, 2022

thejayps Mar 18, 2022

rptb1 commented Jan 13, 2023

rptb1 commented Jan 13, 2023

rptb1 commented Feb 20, 2023 •

edited

Loading

thejayps commented Feb 20, 2023

rptb1 commented Jun 12, 2023

rptb1 commented Nov 6, 2023 •

edited

Loading

		@@ -113,15 +113,16 @@ LONG WINAPI ProtSEHfilter(LPEXCEPTION_POINTERS info)

		/* ProtSetup -- set up the protection system */

		static void *vehHandle;

		/* I'm not sure why the interface is defined this way. RB 2012-08-03 */
		TransformDestroy(transform);

		* How the implementation is an add-on to the MPS.

		* How the code is kept separate from the mainstream MPS.


		* How the code is kept separate from the mainstream MPS.

		* Why it has its own hash table implementation (no good reason).

		If the transform was successfully applied, it is destroyed, as if
		:c:func:`mps_transform_destroy` had been called.

		$(PFM)\$(VARIETY)\cvmicv.exe: $(PFM)\$(VARIETY)\cvmicv.obj \
		$(PFM)\$(VARIETY)\mps.lib $(FMTTESTOBJ) $(TESTLIBOBJ)

Apply all changes from Configura to the public MPS. #76

Apply all changes from Configura to the public MPS. #76

Conversation

gareth-rees commented Jan 28, 2022

gareth-rees left a comment

Choose a reason for hiding this comment

gareth-rees Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gareth-rees Jan 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gareth-rees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rptb1 commented Jan 13, 2023

rptb1 commented Jan 13, 2023

rptb1 commented Feb 20, 2023 • edited Loading

thejayps commented Feb 20, 2023

rptb1 commented Jun 12, 2023

rptb1 commented Nov 6, 2023 • edited Loading

gareth-rees Jan 28, 2022 •

edited

Loading

gareth-rees Jan 29, 2022 •

edited

Loading

rptb1 commented Feb 20, 2023 •

edited

Loading

rptb1 commented Nov 6, 2023 •

edited

Loading