Skip to content

validate_list: make flags argument impossible to spell wrongly. #16843

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tim-one merged 1 commit into from
Oct 19, 2019
Merged

validate_list: make flags argument impossible to spell wrongly. #16843

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 37 additions & 16 deletions Modules/gcmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,23 +329,44 @@ append_objects(PyObject *py_list, PyGC_Head *gc_list)
return 0;
}

// Constants for validate_list's flags argument.
enum flagstates {collecting_clear_unreachable_clear,
collecting_clear_unreachable_set,
collecting_set_unreachable_clear,
collecting_set_unreachable_set};

#ifdef GC_DEBUG
// validate_list checks list consistency. And it works as document
// describing when flags are expected to be set / unset.
// `head` must be a doubly-linked gc list, although it's fine (expected!) if
// the prev and next pointers are "polluted" with flags.
// What's checked:
// - The `head` pointers are not polluted.
// - The objects' prev pointers' PREV_MASK_COLLECTING flags are all
// `prev_value` (PREV_MASK_COLLECTING for set, 0 for clear).
// - The objects' next pointers' NEXT_MASK_UNREACHABLE flags are all
// `next_value` (NEXT_MASK_UNREACHABLE for set, 0 for clear).
// - The objects' PREV_MASK_COLLECTING and NEXT_MASK_UNREACHABLE flags are all
// `set or clear, as specified by the 'flags' argument.
// - The prev and next pointers are mutually consistent.
static void
validate_list(PyGC_Head *head, uintptr_t prev_value, uintptr_t next_value)
validate_list(PyGC_Head *head, enum flagstates flags)
{
assert((head->_gc_prev & PREV_MASK_COLLECTING) == 0);
assert((head->_gc_next & NEXT_MASK_UNREACHABLE) == 0);
uintptr_t prev_value = 0, next_value = 0;
switch (flags) {
case collecting_clear_unreachable_clear:
break;
case collecting_set_unreachable_clear:
prev_value = PREV_MASK_COLLECTING;
break;
case collecting_clear_unreachable_set:
next_value = NEXT_MASK_UNREACHABLE;
break;
case collecting_set_unreachable_set:
prev_value = PREV_MASK_COLLECTING;
next_value = NEXT_MASK_UNREACHABLE;
break;
default:
assert(! "bad internal flags argument");
}
PyGC_Head *prev = head;
PyGC_Head *gc = GC_NEXT(head);
while (gc != head) {
Expand All @@ -361,7 +382,7 @@ validate_list(PyGC_Head *head, uintptr_t prev_value, uintptr_t next_value)
assert(prev == GC_PREV(head));
}
#else
#define validate_list(x, y, z) do{}while(0)
#define validate_list(x, y) do{}while(0)
#endif

/*** end of list stuff ***/
Expand Down Expand Up @@ -657,7 +678,7 @@ clear_unreachable_mask(PyGC_Head *unreachable)
gc->_gc_next &= ~NEXT_MASK_UNREACHABLE;
next = (PyGC_Head*)gc->_gc_next;
}
validate_list(unreachable, PREV_MASK_COLLECTING, 0);
validate_list(unreachable, collecting_set_unreachable_clear);
}

/* A traversal callback for move_legacy_finalizer_reachable. */
Expand Down Expand Up @@ -1050,7 +1071,7 @@ by a call to 'move_legacy_finalizers'), the 'unreachable' list is not a normal
list and we can not use most gc_list_* functions for it. */
static inline void
deduce_unreachable(PyGC_Head *base, PyGC_Head *unreachable) {
validate_list(base, 0, 0);
validate_list(base, collecting_clear_unreachable_clear);
/* Using ob_refcnt and gc_refs, calculate which objects in the
* container set are reachable from outside the set (i.e., have a
* refcount greater than 0 when all the references within the
Expand All @@ -1067,8 +1088,8 @@ deduce_unreachable(PyGC_Head *base, PyGC_Head *unreachable) {
*/
gc_list_init(unreachable);
move_unreachable(base, unreachable); // gc_prev is pointer again
validate_list(base, 0, 0);
validate_list(unreachable, PREV_MASK_COLLECTING, NEXT_MASK_UNREACHABLE);
validate_list(base, collecting_clear_unreachable_clear);
validate_list(unreachable, collecting_set_unreachable_set);
}

/* Handle objects that may have resurrected after a call to 'finalize_garbage', moving
Expand Down Expand Up @@ -1145,7 +1166,7 @@ collect(struct _gc_runtime_state *state, int generation,
old = GEN_HEAD(state, generation+1);
else
old = young;
validate_list(old, 0, 0);
validate_list(old, collecting_clear_unreachable_clear);

deduce_unreachable(young, &unreachable);

Expand Down Expand Up @@ -1178,8 +1199,8 @@ collect(struct _gc_runtime_state *state, int generation,
*/
move_legacy_finalizer_reachable(&finalizers);

validate_list(&finalizers, 0, 0);
validate_list(&unreachable, PREV_MASK_COLLECTING, 0);
validate_list(&finalizers, collecting_clear_unreachable_clear);
validate_list(&unreachable, collecting_set_unreachable_clear);

/* Print debugging information. */
if (state->debug & DEBUG_COLLECTABLE) {
Expand All @@ -1191,8 +1212,8 @@ collect(struct _gc_runtime_state *state, int generation,
/* Clear weakrefs and invoke callbacks as necessary. */
m += handle_weakrefs(&unreachable, old);

validate_list(old, 0, 0);
validate_list(&unreachable, PREV_MASK_COLLECTING, 0);
validate_list(old, collecting_clear_unreachable_clear);
validate_list(&unreachable, collecting_set_unreachable_clear);

/* Call tp_finalize on objects which have one. */
finalize_garbage(&unreachable);
Expand Down Expand Up @@ -1230,7 +1251,7 @@ collect(struct _gc_runtime_state *state, int generation,
* this if they insist on creating this type of structure.
*/
handle_legacy_finalizers(state, &finalizers, old);
validate_list(old, 0, 0);
validate_list(old, collecting_clear_unreachable_clear);

/* Clear free list only during the collection of the highest
* generation */
Expand Down