| Title | Pool class attributes incorrect, unchecked |
| Status | closed |
| Priority | nice |
| Assigned user | Gareth Rees |
| Organization | Ravenbrook |
| Description | See design/type/ [1] for a description of pool class attributes. Pool classes are supposed to have attributes INCR_RB if they require a read barrier and INCR_WB if they require a write barrier. INCR_WB is not set on any pool classes, but some pool classes do require write barriers; and neither INCR_RB nor INCR_WB is ever checked. Some other attributes could be checked more systematically (or at all). See analysis. |
| Analysis | Problems with attributes: 1. There's no interface for checking them, so modules need to explicitly look up pool->class->attr which seems naughty. [I added PoolHasAddr] 2. Design contradicts itself ("attributes are only used for consistency checking ... it's no longer true that they are only used for consistency checking") [I removed this text, and documented what these attribute are actually used for] 3. MOVINGGC is used but undocumented. [added to design] 4. MOVINGGC must imply GC but that is not checked. [now checked in PoolClassCheck] 5. PM_NO_READ and PM_NO_WRITE are defined and documented, but not used. [removed from design] 6. SCAN should be checked systematically by PoolScan, not opportunistically in TraceQuantum. [moved check] 7. SCAN should be checked systematically in SegCheck (segments with ranks must belong to scannable pools), not opportunistically in TraceStart. [moved check] 8. BUF_RESERVE seems to be redundant with BUF [removed it] 9. BUF_ALLOC is defined and documented but not used [removed it] Problems with pool use of attributes: 1. AMCZ inherits from AMC and so has to clear the SCAN and INCR_RB attributes. This indicates that the inheritance is the wrong way round: AMC should inherit from AMCZ, not vice versa. [swapped] 2. AMCZ resets grey and scan methods but forgets to do the same to the blacken method. This error would not occur if the inheritance were reversed. [swapped] 3. MRG sets the FREE attribute but does not provide a free method. [don't set it] 4. PoolClassMixInCollect sets INCR_RB but this isn't right: you only need a read barrier if there are references as well as collection. [don't set it] 5. AMS sets FMT but does not provide a walk method. [used PoolNoWalk] |
| How found | inspection |
| Evidence | [1] <http://www.ravenbrook.com/project/mps/master/manual/html/design/type.html#Attr> |
| Created by | Gareth Rees |
| Created on | 2014-04-04 15:06:37 |
| Last modified by | Gareth Rees |
| Last modified on | 2016-04-15 19:26:30 |
| History | 2014-04-04 GDR Created. |
| Change | Effect | Date | User | Description |
|---|---|---|---|---|
| 185231 | closed | 2014-04-04 17:05:08 | Gareth Rees | Tidy-up of attributes and pool classes: * Bring design up to date. * New function PoolHasAttr encapsulates attribute checking. * Abstract classes are abstract and mustn't be checked. * The dummy pool class in fotest needs a size. * Abstract pool classes null out methods that they can't provide a generic implementation for, to force subclasses to provide one. * New function PoolTrivFramePopPending provides a generic implementation of that method. * Rename PoolNoFreeWalk to PoolTrivFreeWalk since it has NOOP rather than NOTREACHED. * Check that AttrMOVINGGC implies AttrGC. * Remove unimplemented attributes (BUF_RESERVE, BUF_ALLOC, INCR_RB, INCR_WB, PM) * AMC now inherits from AMCZ instead of the other way round. This is simpler: AMC adds features to AMCZ rather than AMCZ taking features away (and not quite getting it right). * Similarly, LO inherits from AbstractSegBufPoolClass + PoolClassMixInCollect so that it doesn't have to clear AttrSCAN and the scan methods. * Fix bug in MFSCheck -- mustn't check unroundedUnitSize >= UNIT_MIN since small unit sizes are rounded up to UNIT_MIN. * Don't see AttrFREE in MRG (since no free method is supplied). * Check AttrSCAN systematically (in PoolScan and SegCheck) rather than opportunistically in TraceStart and TraceQuantum. |