GrognardsFromHell
SpellRemoveItemCond assumes condition argument is last in list
This is a vanilla bug. The function that removes item-based conditions in the original code always removes arguments as if the condition being removed were also most recently added. I guess they were not anticipating that anyone would use multiple spells on the same item.
I think this could also cause the conditions to get corrupted. If you cast Magic Weapon, then crafted a 3-argument condition onto the weapon, the expiration of Magic Weapon would remove 5 arguments from the end, but it would be the 3 arguments for the crafted condition, and 2 from Magic Weapon. Meanwhile, the crafted condition id would get removed, while the enhancement remains, so you'd end up with fewer arguments than the conditions would indicate.
There's already a function with the right logic, but the .c file says SpellRemoveItemCond is a __usercall. Does that act somehow different than __cdecl with respect to replacing functions?
Yes. Usercall is used when args are passed through registers rather than solely on the stack (that is cdecl). It's usually done as a compiler optimization when the function is used privately inside a particular c file (as opposed to one that's shared and used elsewhere).
You can hook it, but it requires doing the prologue and epilogue by hand with asm commands (search for functions with declspec "naked" for examples). This is a bit of a PITA which is why I often prefer to completely replace it, together with the context where it's called. In this case, you could expand the switch cases handled in the spell_remove hook.
Is that the one in spell_condition.cpp? There seem to be multiple places that that function is hooked in some way.
Yeah (spell_remove_spell). It's only hooked once though. It has 4 cases where it calls that - for keen edge, mw/gmw, and magic armor, and they're all similar.