Make the x86_64 hook manager generator work on Linux - #285
Conversation
…copy a ton of notes from System V docs
Seven defects kept the generated code from working on 64-bit Linux, each found by a failing case in the existing TestHookManGen suite: - GenerateHookFunc opened with a leftover breakpoint(). - PushParameters ignored v_this on System V and restored the incoming argument registers instead, so a hook delegate was called with the hooked object as its this pointer. - Objects passed by value with a destructor or copy constructor arrive by invisible reference, so every hook and the original shared the caller's object. Copy them per call and destroy the copies afterwards, the way the x86 generator does; scope the end-of-function parameter destruction to MSVC, since the Itanium ABI has the caller destroy arguments. - The three internal return objects were destroyed before DoReturn copied one of them out. - Returning an object by value was refused outright unless it was exactly 12 bytes, and the memory threshold was 64 bytes rather than two eightbytes. - Vafmt was accepted but never formatted, so hooks saw the raw format string. Build a va_list over the register save area and format once. - A reference to a float takes an integer register, not an SSE one. Protos that would need an argument register the ABI does not have are now declined instead of generated wrongly. The suite passes on x86_64 Linux with clang and gcc, and under valgrind. Enabling it required scoping the generator's metamod dependency, which is only there for a debug helper, out of test builds.
…ocated x64GenContext leaked its two PassInfo arrays: the destructor has Clear() commented out, and nothing else released them, so every generated hook manager lost them. Freeing them there rather than restoring the Clear() call keeps the generated code, which outlives the context, untouched. Both generators also released the arrays with delete rather than delete[] when rebuilding them. valgrind over the test suite goes from 3,840 bytes definitely lost in 81 blocks to 144 in 7, all of which are in the tests themselves.
Which registers hold an aggregate returned by value depends on the class of each of its eightbytes, and a PassInfo carries sizes but not field types, so it cannot be worked out here. SourceHook's copy of the value is read as INTEGER, which is right for most aggregates and wrong for one made only of floats. Keeping the registers the original returned in, and loading both files on the way out, makes that case behave: the integer registers follow SourceHook's copy, a hook's replacement for it included, while the SSE ones carry the original through. An aggregate of the SSE class now comes back intact instead of depending on nothing having touched xmm0 in the meantime; replacing its value from a hook still does not work, which is what already happened, only quietly. Checked on a live CS2 server against a function returning two floats: the value survives a hook that asks to override it, where before the override was dropped and the result only happened to be right.
A post hook that returns a value of the same type leaves it in xmm0, which is where a caller expecting an aggregate of floats looks. Without the change that goes with this the caller gets the hook's value rather than the function's, so the case is worth pinning down. The hooks come off before the values are judged. A CHECK that returns early would leave them installed, and SourceHook shutting down after the generator is gone takes the process with it, which turns a readable failure into a crash after the test has already found the fault.
|
Verified this end to end on a live 64-bit HL2DM dedicated server, since the diff alone does not make the before-state obvious. On What I ran: Metamod built from this branch ( For the generator itself I used a throwaway Metamod plugin that calls Two notes. HL2DM ships no 64-bit |
|
The With #286 applied on top of this branch the job runs to completion: 16 of 16 on linux-x86 and 16 of 16 on linux-x86_64. On clean |
2 participants
core/AMBuilderleavessourcehook_hookmangen_x86_64.cppout of the build on Linux, and the file would not compile there anyway:sh_asm_x86_64.hnames a methodxor, which outside of MSVC is an alternative token for^. So on 64 bit Linux Metamod:Source ships with no hook manager generator at all, and anything that builds hook managers at runtime cannot work on a CS2 server there.This is @rtldg's branch from #212, rebased onto current master with his commits intact, plus fixes for what still kept the generated code from working. He closed that PR himself. The work was close, and this is the rest of it.
What was wrong
Each of these was found by a failing case in the existing
TestHookManGen, which is turned off for 64 bit with a// TODO: Fix for 64-bit.GenerateHookFuncopened with a leftoverbreakpoint(). That one is only on the branch, not on master.PushParametersignoredv_thison System V and restored the incoming argument registers instead, so a hook delegate ran with the hooked object as itsthis. It read a foreign object for its own fields and returned whatever that produced as itsMETA_RES— in the test suite that came out asMRES_SUPERCEDE, so the original never ran. The MSVC path already did this correctly.DoReturncopied one of them out.CallConv_HasVafmtwas accepted but nothing was ever formatted, so hooks were handed the raw format string. Ava_listis now built over a register save area laid out the way the ABI describes, andvsnprintfruns once before any hook sees anything.x64GenContextleaked its twoPassInfoarrays —Clear()is commented out in the destructor and nothing else released them — and both generators released those arrays withdeleterather thandelete[]. The second half of that applies to x86 as well, so it affects builds that ship today.PassInfocarries sizes but not field types. Nothing was written to the SSE registers on the way out, so a post hook returning a value of the same type left its own inxmm0and the caller got that instead of the function's. Both register files are loaded now: the integer ones follow SourceHook's copy of the value, the SSE ones carry the original's through.Protos that would need an argument register the ABI does not have are declined rather than generated wrongly.
Checks
--enable-optimize--smc-check=all-non-file --leak-check=fullTestHookManGenis around forty protos: empty ones and ones with five parameters, floats and doubles, PODs of 7 and 600 bytes by value and by reference, objects with constructors, destructors and assignment operators, returns of char, short, int, float, double and PODs of 1, 4, 8 and 13 bytes, objects with a destructor in the return, vafmt in every combination, high vtable indices — and for each of them the full matrix of pre and post hooks with ignore, supercede, override and recall, checking the exact sequence of constructor and destructor calls.Building and running it again for 64 bit Linux needed one more thing: the generator's dependency on metamod is only there for a debug helper whose calls are commented out, so it moves under
#if !defined(SOURCEHOOK_TESTS). The CI already runstest_sourcehookforlinux-x86_64when that directory exists, so this starts being covered without any change there.Stack alignment was checked rather than argued about. Every generated function was dumped, disassembled and walked over its control flow graph, tracking
rspthrough pushes, pops and adjustments: 74 functions, 414callinstructions, every one of them withrsp16 byte aligned and everyretwith the alignment the ABI asks for. That covers the two boxes left unticked in the review on #212,GeneratePubFuncandCallSetupHookLoop— the second had already been dealt with on the branch.Beyond the suite, this ran on a CS2 dedicated server with a plugin that takes metamod's own
IHookManagerAutoGenand hooks through generated code: pre and post hooks onGameFramereading a field throughthis, an object passed by value where the hook scribbles over its copy and the original still sees clean data, a vafmt function where the hook is handed the formatted string, an aggregate of two floats returned through a post hook, and a thousandMakeHookManandReleaseHookMancycles repeated three times — the first run grows the page pool, the two after it move nothing.Known limits
delete[]fixes is under#if SH_COMP == SH_COMP_GCC, and the test stays off for MSVC.ProtoInfo, which seemed like your call rather than something to slip in here.