I think the problem here is with the coherence protocols maintaining two invalid states: NP and I.

NP is supposed to be the state when the entry has not been allocated, while in I state, the entry

is allocated but is invalid.  I think this was supposed to be an optimization that helps in avoiding 

memory allocation and deallocation.
Joel, if whatever I have said above is right, I would suggest that you eliminate either of the states and

always allocate and deallocate entries.  I think the greater problem is SLICC's lack of constructors.

Nilay's clarification on the original intent is correct. The problem is that the interface for allocating entries is too loose:
A specific problem here is that the cache entry constructor sets permissions to NotPresent, and the allocation can occur anywhere in a .sm file (this is a memory management issue exposed to the programmer :/). As a result, there's no way of knowing how a protocol author might use the entry (e.g. placing an entry in a TBE, and later moving it into the cache, and the permission may not get updated). This allows the cache to contain allocated, NotPresent lines that can be leaked in allocate().
We'd need to make a much broader change to address sources of this leak, and that could affect a lot of downstream code. I've validated this simple fix, and it will work with any downstream code.

Ah, this makes sense to me. Thanks for clarifying. I like this fix as is.

I am in opposition to this change.  Joel, I think protocols should be fixed.  I am not asking you to fix

them but I don't think we should go with this change either.  In fact we should have an assert in the

allocate() function that says (set[i] == nullptr || set[i]->permission != not present || set[i] == entry).

What you are suggesting would make the optimization meaningless.

@Nilay: I agree it would be nice to include such an assertion, but I don't feel it makes sense to do so. Since we can just delete the entry (the way this patch suggests), we don't need to assertion fail if allocate() finds a NotPresent line that is not equal to the passed entry. If we wanted to add this assertion, we'd have to do pretty broad testing that this assertion doesn't get triggered with any of the mainline protocols; The average gem5 user shouldn't be responsible for dealing with this assertion. I don't feel we should be any more aggressive with what this change tries to enforce.
As a compromise, would you be satisfied if I added a warn_once that the protocol contains an entry handling bug if the entry delete code is executed?

Joel, if a protocol is written incorrectly, that's that protocol's problem.

I would not about protocols failing because of the assert.  You don't want to

add the assert, that's ok with me.  Add a warn_once.  But I am not ok with 

deleting the entries that the protocol should be deleting.

I see. Are you worried that a protocol that is mishandling entries might also be maintaining other pointers to the entry in the cache, so deleting them may cause protocol memory bugs? I can buy that. If so, I'll just add the warn_once instead of deletion and update the review request.

You got it right.  I should have been more clear that the responsibility

of deletion lies with the protocol since it is doing the allocation.  We

cannot predict when is the right time to carry out deletions.
No need to repost.