Remove CoreSuspender from interpose functions

[myk002]

since it may end up deadlocking, depending on what's in the call chain

[ab9rf]

I am fairly convinced that it is entirely unsafe, and inappropriate, to acquire a CoreSuspender from a vmethod interpose. If the vmethod is being called from the simulation thread, the thread will already hold the core lock (because the simulation thread holds the core mutex whenever DFHack isn't in the "update" callback) and so it's not needed. If it's being called from a DFhack thread, then the caller should have already acquired a core lock because to get to the interpose it would have had to call a DF entry point, which our protocols require obtaining a CoreSuspender so acquiring it again is redundant. Trying to acquire the corelock from the render thread is unsafe, and trying to acquire it from a DF compute worker thread will cause a deadlock.

I am convinced that attempting to acquire a CoreSuspender while in a vmethod interpose is a logic error and risks deadlock.

So exactly why are we acquiring a suspender in these situations in the first place?

[myk002]

I had thought I found the CoreSuspender to be required in overlay, which interposes the viewscreen methods and calls Lua, but when I tested right now, I did not hit any assert (and I double-checked that this is a non-Release build).

Let me turn this PR into a "remove all interposed CoreSuspenders" change

[ab9rf]

I had thought I found the CoreSuspender to be required in overlay, which interposes the viewscreen methods and calls Lua, but when I tested right now, I did not hit any assert (and I double-checked that this is a non-Release build).

It is likely that I'll add some asserts to monitor for certain thread-safety violations, but this is outside the scope of this PR. We could add an assert-check for a vmethod interpose entered by a thread that doesn't hold the core mutex. This will trigger in the event that such a method is called by a DF threaded worker, but that's intelligence that we could probably stand to learn anyway.

I think we already have an assert for certain Lua operations without holding the core mutex; I have a PR in process that will move Lua::Core::State into Core and it would be fairly trivial after that to have the Core::getLuaState method that that PR will add include an assert for holding the core mutex as well.

[ab9rf]

lgtm - did not confirm that this covers all interposes, however

[myk002]

I didn't do stonesense, but this should be everything in the main repo

[lethosor]

I had thought I found the CoreSuspender to be required in overlay, which interposes the viewscreen methods and calls Lua, but when I tested right now, I did not hit any assert (and I double-checked that this is a non-Release build).

Historically, the problematic assertions were in AssertCoreSuspend() (my guess is this is likely what you are thinking of). This is still called from the following places in LuaTools.cpp:

• DFHack::Lua::SafeCall()
• DFHack::Lua::SafeResume()
• DFHack::Lua::PushModule()
• DFHack::Lua::SafeCallString()
• DFHack::Lua::Event::Invoke()

In particular, I suspect we have several places still calling SafeCall() from vmethod hooks, and maybe building-hacks calls Event::Invoke() indirectly. Have these been verified to still work and not trigger AssertCoreSuspend()?

Maybe it's possible that DFHack::Core::getInstance().isSuspended() in AssertCoreSuspend() returns true now when called from vmethod hooks, and didn't before? The implementation of AssertCoreSuspend() hasn't changed since 2012.

[lethosor]

Also, stonesense doesn't use vmethod hooks so no changes should be needed there. (it well predates our vmethod machinery, and doesn't really integrate into the game aside from the overlay, which is/was renderer-level, essentially drawing a buffer on top of another without regard for DF's representation of anything).