`op_free()` silently does nothing if `OPf_KIDS` but `op_first == NULL`

[leonerd]

Found while debugging a test failure of feature-class branch.

If you want to free the former container of an optree during reshaping, it is insufficient simply to save its child and free the parent by doing this:

OP *kid = cUNOPo->op_first;
cUNOPo->op_first = NULL;
op_free(o);

What happens here is that the op still has the OPf_KIDS flag set, which causes Perl_op_free() to want to recurse into the children. It sets up the pointer to be the ->op_first of the op, then goes around the while loop again. The loop finds that the pointer is now NULL, decides its job is complete, and the whole function returns having done nothing.

While this is technically the caller's fault (because no op should ever have the OPf_KIDS flag while its op_first is NULL), I feel that op_free() ought to do something about it. Just unsure quite what.

[leonerd]

First thought: Just insert an assert() somewhere. Either on entry to the loop, assert that every op that has the flag also has an op_first:

assert(!(o->op_flags & OPf_KIDS) || cUNOPo->op_first);

or else, within the body of the block already guarded by the if condition, assert that

assert(cUNOPo->op_first);

before extracting it and continueing the loop.

This solution has the advantage of not changing any observed behaviour, but simply throwing a debugging assertion failure if the caller passes a bad value.

[leonerd]

Second thought: Maybe the if condition should additionally check that the op_first is not NULL. While technically that means the caller screwed up, in practice we probably don't care hugely if the op is slightly malformed while we're freeing it - probably because the caller is doing some tree reshaping. It just saves the caller from having to care about that flag reset in the first place.

Doing this solution would mean my original code wasn't wrong in the first place.

[demerphq]

I think just documenting this clearly in op_free (or maybe create a perlopguts or something to collect information like this) would be a step forward, especially if the technical solutions don't pan out or are incomplete solutions.

[leonerd]

It could well be mentioned in the documentation but I don't think that would have helped me find this bug at all. I wouldn't think to read the op_free documentation (besides, "it frees an optree", I already know what it does so I'd be unlikely to read it again)

Whereas, adding the assert doesn't cause any core tests to fail, and would have easily picked up my bug (or rather prevented it from happening in the first place). Tested in https://github.com/Perl/perl5/pull/20765

[demerphq]

It could well be mentioned in the documentation but I don't think that would have helped me find this bug at all. I wouldn't think to read the op_free documentation (besides, "it frees an optree", I already know what it does so I'd be unlikely to read it again)

If you take that argument to its logical conclusion it implies there is no point in updating any documentation at all, ever, as it wouldn't benefit the person updating the docs as they know it already. Which is nonsensical, the point of docs is to share the accumulated knowledge about something so that new developers don't have to repeat the pain that older devs already went through. That you would not have read the docs for op_free() does not mean others new to the opcode logic would not. So you should document what you learned.

Also consider that human memory is fallible. You might be the "new developer" in a few years and find your own words again and be thankful you wrote down the analysis in a place where you could find it again. I routinely find such things I wrote years ago that I had completely forgotten about. Eventually you will start forgetting stuff too, it is an inevitable part of growing older.

So I think you should add docs on how op_free() is meant to be used based on your experience. Had someone else had done so then you would have known about this when you did the read the docs the first time, and you wouldn't have had this issue.

Whereas, adding the assert doesn't cause any core tests to fail, and would have easily picked up my bug (or rather prevented it from happening in the first place). Tested in https://github.com/Perl/perl5/pull/20765

The assert is helpful, but the assert combined with documentation is even more helpful. Add some docs too. I do it all the time, and people often quote docs I wrote years ago back to me. Not that long ago @leont quoted my docs on sv_gets() back to me. I spent hours debugging how that function worked, and added a ton of comments to explain what was going on so that future developers didn't have to redo that analysis (See sv.c:8797). I have long since forgotten most of it. But since its in the code as docs/comments it is there for me to refresh myself, and for others to benefit from. You should do the same. If someone as smart as you spent hours or days debugging this issue then some docs would help everybody else.

I see the "assert" as "give a man a fish", and i see the docs as "teach a man to fish", and while it is generally agreed it is "better to teach a man to fish than it is to give them a fish", doing both is even better still.

[leonerd]

So, TL;DR "add both docs and assert"? Sure I can do that.

[karenetheridge]

does this need a delta?