Strange check in arc.mm:emptyPool() on line 174

[cmpwi]

Greetings,

The second check within the while loop seems suspect; the wrapping if statement is only run if stop is not NULL, and within the while loop, stop is never modified. However, a check for stop == NULL is present. Thus, the rest of the if statement (stopPool->previous == NULL) is never evaluated and the loop is never broken in this case.

Reading through the code gives me the impression that simply removing stop == NULL is sufficient, however, I am definitely out of my league with this one. Perhaps the current behavior is indeed expected and desired?

As a thought experiment, if stop is not equal to NULL, and the previous pool happens to be NULL, the second if statement won't catch that. If, by some miracle, the stop location is not found, stopPool then becomes NULL by virtue of stopPool->previous being NULL. The second pass over from the top will then catch that and return. I wouldn't know of any case where all of these suppositions are true at the same time, and I don't think that anything bad/catastrophic is happening as is since the first check handles this possibility, but the logic does seem really wonky to me.

Here is the relevant snippet:

static void emptyPool(struct arc_tls *tls, void *stop)
{
	struct arc_autorelease_pool *stopPool = NULL;
	if (NULL != stop)
	{
		stopPool = tls->pool;
		while (1)
		{
			// Invalid stop location
			if (NULL == stopPool)
			{
				return;
			}
			// NULL is the placeholder for the top-level pool
			if (NULL == stop && stopPool->previous == NULL)
			{
				break;
			}
			// Stop location was found in this pool
			if ((stop >= stopPool->pool) && (stop < &stopPool->pool[POOL_SIZE]))
			{
				break;
			}
			stopPool = stopPool->previous;
		}
	}
/* Routine continues... */

[cmpwi]

Reading the code more carefully now makes it more obvious why it all still works: if stop is not NULL and valid, it's guaranteed to be found in at least the final pool after going though them all, thus breaking appropriately and continuing with the rest of the routine's logic. The whole stopPool->previous == NULL check just seems like an optimization that never gets run; now it makes sense.

Edit: I just realized that if an invalid yet non-NULL stop location is given and stopPool is at the final pool, the while loop will break and the entirety of stopPool will be released in the do-while loop below (edit: which would only happen if stop is some contrived address like 0x1, thus being smaller than tls->pool->insert). Is that desired behavior? Perhaps the entirety of the second check should be removed? I believe the loop should only break if stop is indeed a valid position within a pool, and return if all pools have been exhausted and a position has not been found.