Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Templated SourceHook #176

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Mooshua
Copy link

@Mooshua Mooshua commented Apr 24, 2024

Introduces one massive template for generating SourceHook managers. The goal is for this to simplify the definition of SourceHooks long-term by replacing macros with templates.

Draft until API is settled upon, but is most likely merge-able now.

Introducing SourceHook::HookImpl

SourceHook::HookImpl is a template that implements the previous functionality of the SH_DECL macros. It uses the following template parameters in it's implementation:

  • ISourceHook** SH - Pointer to SourceHook (eg &g_SHPtr)
  • Plugin* PL - Pointer to global PluginId value (eg &g_PLID)
  • typename Interface - The interface to hook
  • Result (Interface::Method)(Args...) - The argument to hook (eg &IEngine::DoThing
  • typename Result - the return type or void
  • typename ...Args - all arguments, specified as a varardic type pack.

The macro PLUGIN_GLOBALVARS() now exposes the Hook template with the SH and PL fields filled in from the global variables--so all plugin authors need to do to get started is specify `Hook<Interface, Method, Return, Args...>::Make().

Example usage:

//  Declares Hook<> template
PLUGIN_GLOBALVARS()

//                                 Interface       Method                     Result
//  Generic Hook                   vvvvv           vvvvv                      vvvv
auto OnGameInit = SourceHook::Hook<IServerGameDLL, &IServerGameDLL::GameInit, bool>::Make();
auto OnLevelInit = SourceHook::Hook<IServerGameDLL, &IServerGameDLL::LevelInit, bool, const char*, const char*, const char*, const char*, bool, bool>::Make();

//  Varfmt hook
//  virtual void ClientCommand(edict_t* pClient, const char* fmt, ...);
auto OnClientCommand = SourceHook::FmtHook<IVEngineServer, &IVEngineServer::ClientCommand, void, edict_t*>::Make();

void AddHooks()
{
	OnGameInit->Add(server, false, SH_MEMBER(this, &SourceProvider::Hook_GameInit));
 
        //  Alternative hook modes available in fourth argument (defaulting to Normal)
	OnLevelInit->Add(server, false, SH_MEMBER(this, &SourceProvider::Hook_LevelInit), ISourceHook::AddHookMode::Hook_VP);
}

Introduces one massive template for generating SourceHook managers.
The goal is for this to simplify the definition of SourceHooks long-term by replacing macros with templates.

This introduces some new templates for metaprogramming, but they're hidden away in the SourceHook::metaprogramming namespace.

Tested on Windows 32 bit and Linux 32 bit TF2.
…feat/templated-sourcehook

Sometimes my vast intellect... it scares me.

# Conflicts:
#	core/provider/source/provider_source.cpp
This allows us to specify the SH pointer and the Plugin ID pointer in the template, so when the plugin uses Hook<> it doesn't have to touch g_PLID or g_SHPtr.

Also added a little struct that wraps ISourceHook->xHookById methods.
@GAMMACASE
Copy link
Member

Very neat stuff, but would throw my 2 cents here, what do you think on renaming exposed struct Hook to something more verbose, like SH_Hook? Just to prevent any potential name collisions and further confusion due to it on the user end.

@Mooshua
Copy link
Author

Mooshua commented Apr 24, 2024

Very neat stuff, but would throw my 2 cents here, what do you think on renaming exposed struct Hook to something more verbose, like SH_Hook? Just to prevent any potential name collisions and further confusion due to it on the user end.

How about "HookFactory" or "SourceHookFactory", since that's effectively what it is?

@dvander
Copy link
Member

dvander commented Apr 24, 2024

Thanks for doing this! Will take a look this weekend hopefully.

Namespaces are good avoiding for name collisions.

This is a large refactor that splits our mega-template into a few smaller ones. First off, the PLUGIN_GLOBALVARS() helpers were put in the SourceHook namespace.

- HookHandlerImpl: Responsible for the lowered delegates (post-vafmt), can be used independently of other templates added here. Relies on parent HookManager class to handle the unlowered original invocation logic. (As a template parameter)
- HookCoreImpl: Adds a public interface & glue layer beterrn managers and HookHandlerImpl

- HookImpl: non-varardic hook manager
- FmtHookImpl: format-string hook manager

FmtHookImpl was tested by hooking IVEngineServer::ClientCommand.
@Mooshua
Copy link
Author

Mooshua commented Apr 25, 2024

Added varardic hook macros, and I decided to just put the helpers into the SourceHook namespace. Hopefully this doesn't cause issues with plugins.

Split the original Hook template into four new templates: One for SourceHook -> Delegate APIs (HookHandlerImpl), one for Plugin -> SourceHook APIs (HookCoreImpl, and two handling the ABI for the raw hooks (HookImpl and FmtHookImpl). They're still a little rough around the edges and need some boundary refinement.

As a result of this, varfmt hooks are now supported, with similar handling to macro varfmt hooks:

auto OnClientCommand = SourceHook::FmtHook<IVEngineServer, &IVEngineServer::ClientCommand, void, edict_t*>::Make();

void SamplePlugin::Hook_SendClientCommand(edict_t* pEntity, const char* pszCommand)
{
    // pszCommand = printf(fmt, ...)
}

Still thinking about how I want to tackle manual hooks; open to suggestions.

@dvander
Copy link
Member

dvander commented Apr 25, 2024

Not everything has to be solved in one PR! Happy to take incremental improvements. API changes don't get frozen until release.

@Mooshua Mooshua marked this pull request as ready for review April 25, 2024 03:12
Copy link
Member

@Kenzzer Kenzzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First thing first, that's an awesome PR everything is thoroughly documented. And SourceHook really needed a makeover with templates, this is amazing.

Also since I happen to also be modifying SourceHook in another PR. I think I'm familiar enough with the framework to give a tiny review. There's one problem spotted, everything else is nitpicks, I might do another review if I've time to try this out in a project.

core/sourcehook/generate/sourcehook.hxx Outdated Show resolved Hide resolved
core/sourcehook/generate/sourcehook.hxx Outdated Show resolved Hide resolved
core/sourcehook/generate/sourcehook.hxx Show resolved Hide resolved
@Mooshua
Copy link
Author

Mooshua commented Apr 25, 2024

Don’t merge just yet—-just found some issues with reference return handling. Should be a quick fix. FIXED.

On another note, are the test cases still maintained and regularly built? I’m getting some errors in the macros on MSVC and on GCC all of the tests fail. I was able to get it to build on windows by gutting some of the tests and all the tests passed there. Linux was WSL Debian x64 with G++ multilib (-m32)

Copy link
Member

@dvander dvander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is absolutely fantastic! I'm impressed with how straightforward it is and how much nicer the actual interface is. Best of all it does not touch the ABI as far as I can tell.

Aside from some minor nits, I'd be happy to take this on 2.0 and/or 1.12 (though you probably want to bake it for a little bit before doing a backport).

I think one thing is needed before committing to this on 2.0: some tests. They don't have to be that advanced, but some kind of addition to the SourceHook test suite would be great. If for no other reason to document the new API a little.

And to commit on 1.12: Take a new MM:S and old SourceMod and make sure they run together, as a smoke test that the ABI hasn't changed.

core/provider/source/provider_source.cpp Show resolved Hide resolved
core/sourcehook/sourcehook.h Outdated Show resolved Hide resolved
core/sourcehook/sourcehook.h Outdated Show resolved Hide resolved

// You're probably wondering what the hell this does.
// https://stackoverflow.com/questions/11709859/how-to-have-static-data-members-in-a-header-only-library/11711082#11711082
// I hate C++.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we assume C++17 now, does that help?

(Alternately, I'm not above requiring linking to a libsourcehook, but the hack is so small that that seems like overkill)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was back when I was trying to get this to work on C++11, before I realized that was plain impossible. I'll look into an inline static.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to keep this as a hack for the time being just because I don't understand the semantic differences between this and inline static. Would be more than willing to do either I just dont know what is and isn't UD these days :/

core/sourcehook/sourcehook.h Outdated Show resolved Hide resolved
core/sourcehook/sourcehook.h Outdated Show resolved Hide resolved
core/ISmmPlugin.h Outdated Show resolved Hide resolved
core/sourcehook/generate/sourcehook.hxx Outdated Show resolved Hide resolved
@dvander
Copy link
Member

dvander commented May 1, 2024

There's also a build error:

/home/runner/work/metamod-source/metamod-source/metamod-source/core/sourcehook/sourcehook.h:1279:5: error: use of undeclared identifier 'vsnprintf'
                                vsnprintf(buf, sizeof(buf), fmt, ap);

I'm not sure why this didn't get pulled in as part of <cstdarg>.

- A few style improvements
- Add & correct some documentation
- Change AddHook signature to not allow DVP as an option (for now!)
- Fix cstdio not being pulled in on linux (bleh)
- Add some more static_asserts to make errors easier to interpret (yay)
@Mooshua
Copy link
Author

Mooshua commented May 2, 2024

I'm not sure why this didn't get pulled in as part of <cstdarg>.

Fixed. Hopefully. I think.
Did a lot of the changes requested here! Working on tests & sample MM sometime in the future. Let me know if there's anything else that suits your fancy :^)

zer0k-z pushed a commit to zer0k-z/metamod-source that referenced this pull request Jun 7, 2024
* Replace protobuf 2.6.1 with 3.21.8

* Update/add protobuf libs

* Add CS2 protos

* Remove old csgo/dota protos

* Add versioned protoc bin

* Comment out Valve's `schema` define for now

* Use ENetworkDisconnectionReason

* Fix-up `offsetof` to avoid errors on some Clang versions
@MSWS
Copy link

MSWS commented Jun 9, 2024

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants