Implement object address discovery from an interior pointer in the public MPS

[thejayps]

Fixes #216

Progress towards resolving #110

Part of the plan to meet Configura's requirements by separating and reviewing each implementation.

This branch is a manual reimplementation of code written for mps-2022 which was informally released to Configura.

[thejayps]

Executing proc.review.entry

1. Start time 22:48
2. proc.review.entry.criteria:
   applying entry.universal, entry.design, entry.impl.
3. entry.universal.author: This work was done by re-coding mps_addr_object as re-implemented by @thejayps for Configura in their informal release "mps-2022", in turn implemented by examining their custom implementation of mps_addr_object in their in-production MPS build. As author I request and permit review. Configura have given us permission to do this work: https://info.ravenbrook.com/mail/2022/01/18/12-53-38/0/
4. entry.universal.source-available: 2 mail threads: The story behind "Ravenbrook MPS sources are out of sync with
   Configura's sources" and Gathering information on Configura specific functionality describe the approach to this work. (Links to the base email only, p4 grep can find all the subsequent responses).
5. entry.universal.source-approved: source documents have in their entirety not been subject to the processes required to allow them to have "passed exit". We should consider the source documents unreviewed if in doubt, yet bear in mind the main purpose of this change is to merge code and functionality, not reinvent it.
6. entry.design.rfc this PR introduces interior pointer resolution to base pointer functionality. Some design issues have been identified and discussed informally before review has taken place, but @rptb1 advised that design improvements should be avoided for release 1.118, where we are aiming to merge functionality. I believe that this creates an as yet not formally identified constraint on the review that could be identified as a "proc.review.consideration" or an attribute to the change itself eg "change.purpose".
7. exit.universal applies
8. Entry passed
9. Entry completed at 23:19
10. Entry took 31 mins

[thejayps]

Executing proc.review.plan

1. Planning started at 23:22
2. The review is expected to take place at 1400 BST on 2023-05-18 with @UNAA008 @rptb1 and @thejayps available for roles.
3. The change consists of ~+200loc testbench, ~140loc main ~+40 lines of documentation
4. (as yet not in procedure - see Review planning does not require the review leader to define what is to be reviewed #221 ) Review is expected to be constrained to the changes in this PR and local code, and appropriate source docs. There are believed to be no hidden e.g. dormant areas of product document that need examining.
5. proc.review.role.check.correctness .consistency and .source are most important
6. all reviewers needs to consider the dual purpose of this change: implement a new feature and merge capabilities.
7. roles:
   .correctness @rptb1 is invited to examine correctness: 340 loc - 35/40mins.
   .sources @thejayps will perform .source review
   .consistency @UNAA008 is invited to consider consistency of the code (140loc) and documentation (40 lines). The addrobj.c testbench should be checked last in this role. ~20-30 mins on main code and docs, ~10-15 mins on testbench.
8. Individual checking will run for 45 mins.
9. Ensure needed documents available to checkers: TODO
10. Plan finished at 23:51
11. Plan took 29 mins

[thejayps]

@thejayps @UNAA008 @rptb1

The source documents for this review are:
Issues:
#110
#216
Mails:
https://info.ravenbrook.com/mail/2023/01/13/14-45-37/0/
https://info.ravenbrook.com/mail/2023/03/23/16-08-02/0/
Other
https://www.ravenbrook.com/project/mps/custom/cet/main/manual/html/custom/cet/index.html
(for consistency, this provides documentation for other custom features that have so far been documented)

What is to be reviewed: The new code and documentation in this pull for the most part represents the product document. There are no other major areas of code or documentation identified for review.

Rules
https://github.com/Ravenbrook/mps/blob/586336a7021c2276a3f798ad10e6e4ab5f438226/procedure/rule.code.rst
https://github.com/Ravenbrook/mps/blob/586336a7021c2276a3f798ad10e6e4ab5f438226/procedure/rule.code.style.rst
https://github.com/Ravenbrook/mps/blob/586336a7021c2276a3f798ad10e6e4ab5f438226/procedure/rule.generic.rst
https://github.com/Ravenbrook/mps/blob/586336a7021c2276a3f798ad10e6e4ab5f438226/procedure/rule.style.rst
apply.

[thejayps]

Begin proc.review.ko

Start 1404
@rptb1 is checking .correctness and will start with the testbench
@thejayps is checking .source
@UNAA008 is checking .consistency and will start away from the testbench
Logging will take place at 1505

[thejayps]

begin checking:

1. M. " a8fd8c1 252 3 3 code/poolamc.c Cherry-pick change 192595 from custom/cet/main, removing mps_addr_object. This function failed to solve the problem of decoding the stack on 64-bit Windows, because the stack may need to be decoded after an MPS assertion failure, in which case the arena lock is held and mps_addr_object cannot be called. We eventually solved the problem in a different way (using mps_arena_postmortem) and mps_addr_object is no longer used." appears as a commit in our document assessing the risk to Configura. It possibly explains why mps_addr_object was believed not to be used by Configura. However:

• any client may wish to do stacktracing upon errors constrained to their own code space.
• Configura want to have the functionality of mps_addr_object implemented.
• it is not clear how mps_arena_postmortem performs the capabilities of mps_addr_object

2. M. comments in source documents describe important constraints to the use of the original function. The comments are are not present in the new implementation. From the mps-preview branch at Configura [mps_addr_object is not present in custom/cet/main@199182. RB 2023-06-07] [These constraints do not appear in the manual on this PR's branch. RB 2023-06-07] [The comment about the stack root is an example of something that works for Configura (where the stack is a root) but indicates a problem with the implementation, and a constraint on the use that we should document). RB 2023-06-07]:

mps_res_t mps_addr_object(mps_addr_t *p_o, mps_arena_t arena, mps_addr_t addr)
{
  Res res;
  Addr p;

  AVER(p_o != NULL);

  /* This function cannot be called while walking the heap, unlike
   * mps_arena_has_addr(). This is because it is designed to be called
   * with an active mutator, so takes the arena lock. This is in order
   * that it sees a consistent view of MPS structures and the heap,
   * and can peek behind the barrier.
   */
  ArenaEnter(arena);
  AVERT(Arena, arena);
  res = ArenaAddrObject(&p, arena, (Addr)addr);
  ArenaLeave(arena);
  /* We require the object to be ambiguously referenced (hence pinned)
   * so that p doesn't become invalid before it is written to *p_o.
   * (We can't simply put this write before the ArenaLeave(), because
   * p_o could point to MPS-managed memory that is behind a barrier.)
   */
  if(res == ResOK) {
    *p_o = (mps_addr_t)p;
  }
  return res;
}

It is likely that the new implementation does not properly document the constraints which may lead to defects for MPS clients.

See also in AMCAddrObject():

if (SegBuffer(seg) != NULL) {
    /* We use BufferGetInit here (and not BufferScanLimit) because we
     * want to be able to find objects that have been allocated and
     * committed since the last flip. These objects lie between the
     * addresses returned by BufferScanLimit (which returns the value
     * of init at the last flip) and BufferGetInit.
     *
     * Strictly speaking we only need a limit that is at least the
     * maximum of the objects on the segments. This is because addr
      must* point inside a live object and we stop skipping once we
     * have found it. The init pointer serves this purpose.
     */
    limit = BufferGetInit(SegBuffer(seg));
  } else {
    limit = SegLimit(seg);
  }

3. m. It is not clearly written what Configura's requirements are

[rptb1]

Executing proc.review.check

1. Start time 14:20.
2. Checking addrobj.c: 8m, 3C
3. C. make testci does not run the test on Linux.
4. m. Test prints no output. Should say "no defects found" at least, and probably a lot more.
5. 14:45. Checking manual. Building manual. 3M, 2m.
6. M. Missing design. rule.code.design.
7. Only skimmed the rest before logging meeting at 15:05.
8. 4C 13m 4M.
9. Logging rescheduled to 16:00.
10. End time 15:15.

[UNAA008]

My role in this review was consistency checking.

I Ambiguous use of "Return" as both the function return value and a value placed in a provided pointer. I suggest the latter should be described by "Provide" rather than "Return".

m Copyright dates in many of the files should be updated.

I Can we make it so that the copyright dates are only in one place in
the source files? Eg have a header comment to say
"See end of file for Copyright and License." ?

m The difference between Res and mps_res_t was not obvious to me and I'm recording this so that we can check it is well explained in the documentation to do with internal and external pointers. Is it easy to find the equivalences?

m In poolamc.c amcAddrObjectSearch line 1896
There is no commentary about the intent of this function.

[rptb1]

Further checking during proc.review.log

1. Started 16:00.
2. nI. I have implemented a copyright date checker for CI somewhere [It's now visible in Automatically checking copyright dates in copyright notices #231 . RB 2023-06-07]. We should deploy it. [It's in my repo on Kiwi in branch/2023-03-23/check-copyright-dates. RB 2023-05-19]
3. Finished 16:55.

[thejayps]

proc.review.brainstorm

Start 1230
From @thejayps M1

1. Search for relevant tokens during development to understand history
2. We don't have a description of configura's requirements. We don't have a place to point to for this.
3. Entry criteria should possibly be strengthened with a statement about requirements. Requirements weren't clear at the start of the review.
4. Requirements documents should start as soon as a requirement exists, and should be iterated as requirements are updated.

From @thejayps M2
5) For merging and porting of requirements, always trace the original requirements

From @rptb1 Item 6
6) Don't assume that because something has been done a certain way before means it was done correctly. This should be part of #170

From @rptb1 in #223 (comment)
7) Documentation should always be written in a way that tries to help someone trying to use it.
8) Ensure we have adequate documentation rules

Brainstorm took 1h

[rptb1]

Executing proc.review.edit with @thejayps

1. Start time 13:40.
2. Reading the review log.
3. Implement object address discovery from an interior pointer in the public MPS #223 (comment) item 1. This is not a defect but a question. Answer: This is resolved by a conversation with Configura where they assert that they do require mps_addr_object. No edit to document.
4. Implement object address discovery from an interior pointer in the public MPS #223 (comment) item 1, Pass: @thejayps .
5. Implement object address discovery from an interior pointer in the public MPS #223 (comment) item 3. Reject: We've since discovered the jobs, etc. and linked them from issue Implement object address discovery from an interior pointer in the public MPS #223 .
6. Implement object address discovery from an interior pointer in the public MPS #223 (review) item 3. Fixed: Added addrobj test to Xcode and enabled on all platforms in 7e0289f .

Handing over edits to @thejayps . (handing over believed to have taken place at ~1615)

[thejayps]

Executing proc.review.edit

[rptb1]

I Ambiguous use of "Return" as both the function return value and a value placed in a provided pointer. I suggest the latter should be described by "Provide" rather than "Return".

Fixed: Used language consistent with the rest of the manual in 160c87d

m Copyright dates in many of the files should be updated.

Raise: Will be considered in #231 (comment)

I Can we make it so that the copyright dates are only in one place in the source files? Eg have a header comment to say "See end of file for Copyright and License." ?

Raise: Will be considered in #231 (comment)

m The difference between Res and mps_res_t was not obvious to me and I'm recording this so that we can check it is well explained in the documentation to do with internal and external pointers. Is it easy to find the equivalences?

Answer: design.mps.interface

m In poolamc.c amcAddrObjectSearch line 1896 There is no commentary about the intent of this function.

Fixed: Added comment about this and the relationship with AMCAddrObject in c4213cb

[thejayps]

2. M. comments in source documents describe important constraints to the use of the original function. The comments are are not present in the new implementation.  From the mps-preview branch at Configura [mps_addr_object is not present in custom/cet/main@199182.  RB 2023-06-07] [These constraints do not appear in the manual on this PR's branch.  RB 2023-06-07] [The comment about the stack root is an example of something that works for Configura (where the stack is a root) but indicates a problem with the implementation, and a constraint on the use that we should document). RB 2023-06-07]:

mps_res_t mps_addr_object(mps_addr_t *p_o, mps_arena_t arena, mps_addr_t addr)
{
  Res res;
  Addr p;

  AVER(p_o != NULL);

  /* This function cannot be called while walking the heap, unlike
   * mps_arena_has_addr(). This is because it is designed to be called
   * with an active mutator, so takes the arena lock. This is in order
   * that it sees a consistent view of MPS structures and the heap,
   * and can peek behind the barrier.
   */
  ArenaEnter(arena);
  AVERT(Arena, arena);
  res = ArenaAddrObject(&p, arena, (Addr)addr);
  ArenaLeave(arena);
  /* We require the object to be ambiguously referenced (hence pinned)
   * so that p doesn't become invalid before it is written to *p_o.
   * (We can't simply put this write before the ArenaLeave(), because
   * p_o could point to MPS-managed memory that is behind a barrier.)
   */
  if(res == ResOK) {
    *p_o = (mps_addr_t)p;
  }
  return res;
}

It is likely that the new implementation does not properly document the constraints which may lead to defects for MPS clients.

See also in AMCAddrObject():

if (SegBuffer(seg) != NULL) {
    /* We use BufferGetInit here (and not BufferScanLimit) because we
     * want to be able to find objects that have been allocated and
     * committed since the last flip. These objects lie between the
     * addresses returned by BufferScanLimit (which returns the value
     * of init at the last flip) and BufferGetInit.
     *
     * Strictly speaking we only need a limit that is at least the
     * maximum of the objects on the segments. This is because addr
      must* point inside a live object and we stop skipping once we
     * have found it. The init pointer serves this purpose.
     */
    limit = BufferGetInit(SegBuffer(seg));
  } else {
    limit = SegLimit(seg);
  }

Fix - added these comments again in 4f3e8db

[thejayps]

4. m. Test prints no output.  Should say "no defects found" at least, and probably a lot more.

(from #223 (review))

Fix: made more verbose in 4956861

[thejayps]

6. M. Missing design. rule.code.design.

from #223 (review)

Raise: #244

[thejayps]

Ready for exit

[thejayps]

Executing proc.review.exit

1. Start time 1440
2. Some checks found to be failing, re running checks.
3. Checks now pass
4. Transgression - no estimation of defects remaining. Doing now: 6 Major defects found, 1 subsequently found to be not a defect: 5 / 0.75 = 6.7 ~ 1or 2 major defects estimated to remain.
5. exit.pass
6. review.exit.calc:
   ~20 person hours (not all edit time logged) for 5 defects = 50 person hours
   Saved 30 hours
   1 or 2 Major defects remaining
7. End time 1515 - exit took 35 mins

[thejayps]

executing proc.merge.pull-request (currently in #228)

1. Start time 1746
2. Pre merge checklist - all "yes"
3. End time 1801 - took 15 mins