ianpatt / f4se

Fallout 4 Script Extender
https://f4se.silverlock.org
120 stars 24 forks source link

add trampoline interface #6

Closed Ryan-rsm-McKenzie closed 3 years ago

Ryan-rsm-McKenzie commented 3 years ago

Note that by exposing the pool before we commit our hooks, we run the risk of a plugin allocating the entire pool before we can use it. The solution would be to move g_pluginManager.Init(); to the end, but this may or may not be a compatibility issue. https://github.com/ianpatt/f4se/blob/14c99cbade4a077edea2eb1bc10fb98292cea0b8/f4se/f4se.cpp#L87-L98

ianpatt commented 3 years ago

Before exposing this, we need to properly handle cases where plugins allocate too much memory and one of the pools runs out. Currently all of the internal code is written assuming that the size of the buffer specified during startup will be large enough. One option would be to separately budget plugin allocations and reserve a fixed size for internal allocations.

Ryan-rsm-McKenzie commented 3 years ago

I believe Skyrim's trampoline interface also has this problem

ianpatt commented 3 years ago

Sorry for the delay. I am still getting used to the git workflow. I guess if I want to push a commit in to this pull request I need to fork your fork, commit to that, then send you a pull request? While I am doing that, the change is to use the types in common for PluginAllocator and not assume all plugins developers are on C++17. The new project layout is going to cause a lot of friction for people who have been used to a drag-and-drop upgrade process already. There is no reason to make it not compile for them as well.

--- a/f4se/PluginManager.h
+++ b/f4se/PluginManager.h
@@ -64,13 +64,14 @@ private:
 class PluginAllocator
 {
 public:
-       using Byte = unsigned char;
+       PluginAllocator()
+               :m_first(nullptr), m_last(nullptr) { }

-       Byte* Allocate(size_t n)
+       UInt8* Allocate(size_t n)
        {
                const Locker l(m_lock);

-               Byte* result = nullptr;
+               UInt8* result = nullptr;
                if (m_first + n < m_last)
                {
                        result = m_first;
@@ -84,17 +85,17 @@ public:
        {
                const Locker l(m_lock);
                assert(!m_first && !m_last);
-               m_first = static_cast<Byte*>(memory);
+               m_first = static_cast<UInt8*>(memory);
                m_last = m_first + size;
        }

 private:
-       using Lock = std::mutex;
-       using Locker = std::lock_guard<Lock>;
+       typedef std::mutex Lock;
+       typedef std::lock_guard<Lock> Locker;

        Lock m_lock;
-       Byte* m_first{ nullptr };
-       Byte* m_last{ nullptr };
+       UInt8* m_first;
+       UInt8* m_last;
 };
ianpatt commented 3 years ago

Hm, and apparently it's awkward if github thinks you already have a fork of the same project. It seems like I should be able to do something with branches on this existing project. Will mess around with that.

Ryan-rsm-McKenzie commented 3 years ago
ianpatt commented 3 years ago

Ah. It sounds totally plausible that this is just bad standards support in vs2012, and just guessed at when those features were introduced. I've personally moved on from vs2012 for a long time, but there are probably plugin authors still using it. There is no reason to make this a breaking change.

ianpatt commented 3 years ago

Thanks, sorry for the bumps while figuring out github. Skyrim's trampoline interface has this problem as well, but I didn't write or code review it.