Conversation
|
Any reason you've decided not to use #223 ? Personally I'm not against merging this, but if we do, the other PR is going to blow up sourcehook anyways. Still this is nice to have so perhaps the PR should be retargetted towards |
The Makefile had drifted from test/AMBuilder far enough that it no longer linked, and since AMBuilder only builds these tests for x86 and CI does not run them at all, nothing noticed. Five things were missing: the architecture-specific hook generator source and its symlink, added when hookmangen was split; SOURCEHOOK_TESTS, without which SH_DEBUG_LOG pulls in g_SHPtr, defined only in core/metamod.cpp; core/ on the include path; and -fno-devirtualize. That last one is worth calling out. Without it the suite builds and then fails ten of its sixteen tests and segfaults, which looks like SourceHook is broken rather than like a missing flag: devirtualisation assumes the vtable entries SourceHook swaps cannot change. AMBuilder passes it from gcc-4.9 on. With all five, the suite is green. x86_64 is left out on purpose, matching AMBuilder: that generator includes metamod.h and the HL2SDK, which a standalone build does not have. Building natively on x86_64 now says so instead of failing on a missing interface.h.
The constructor set m_CleanupTask and left the other eleven indeterminate. SetIgnoreHooks pushes a context with only m_State filled in, so GCC warns that the rest may be used uninitialized. Nothing reads them today: SetupHookLoop writes pVfnPtr and the others before any use, which the compiler cannot see. So this is removing a trap rather than fixing a fault, and it costs nothing on the dispatch path, where contexts come from CStack::make_next() out of an already-constructed sector rather than from this constructor. This is not the warning in alliedmodders#191, which is about orig_ret in SH_SETUPCALLS.
ModuleInMemory reads /proc/curproc/map into long, though it scans with %lx, which takes unsigned long, and compares against lower and upper, which are unsigned. The Linux branch five lines above already declares them unsigned. Behaviour is unchanged: the bit patterns match and the comparison converts to unsigned anyway. It is the format specifier and 22 warnings. Compile tested only, as the branch does not run on Linux.
1fe8920 to
e4629a0
Compare
|
Started from what was in front of me: I hit this while working on the x86_64 generator, fixed it there, and only looked at the open PR list afterwards, which was the wrong order. #223 is clearly the direction and I am not trying to work around it.
The same question applies to #285, which makes the x86_64 generator work on Linux. Its value is on the Source 2 side, which is the side #223 replaces. Worth landing before that, or would you rather I close it? Happy either way, I would just rather ask than leave it sitting. |
|
I think both PRs are very valuable to have regardless and we should merge them in 1.12 if we can. MMS is going to/has diverge(d) starting with version 2.0, but version 1.X is still gonna stick around. I can't review either PR for now, and its probably going to sit like this for a few weeks. But should I find any issues, I will most likely commit the requested changes. Thanks again for your work. |
2 participants
The standalone
core/sourcehook/test/Makefilehad drifted fromtest/AMBuilderfar enough that it no longer linked. Since AMBuilder builds these tests for x86 only and neither CI workflow runs them, nothing noticed. Five things were missing: the architecture-specific hook generator source and its symlink, added when hookmangen was split;SOURCEHOOK_TESTS, without whichSH_DEBUG_LOGpulls ing_SHPtrthat onlycore/metamod.cppdefines;core/on the include path; and-fno-devirtualize.That last one is worth calling out on its own. Without it the suite builds and then fails ten of its sixteen tests and segfaults, which reads as SourceHook being broken rather than as a missing flag — devirtualisation assumes the vtable entries SourceHook swaps cannot change. With all five the suite is green at 16/16, on both
CPP="gcc -m32"andCPP=i686-linux-gnu-gcc.x86_64 stays out, matching AMBuilder: that generator includes
metamod.hand the HL2SDK, which a standalone build does not have. Building natively on x86_64 now says so instead of failing on a missinginterface.h.Two warnings the revived build surfaced are fixed in their own commits.
CHookContext's constructor setm_CleanupTaskand left eleven members indeterminate; nothing reads them today, sinceSetupHookLoopwrites them before use, and it costs nothing on the dispatch path because contexts there come fromCStack::make_next()out of an already-constructed sector.ModuleInMemoryscanned/proc/curproc/mapintolongwhile using%lxand comparing against unsigned values, which the Linux branch five lines above already gets right; behaviour is unchanged, and that branch does not run on Linux so it is compile-tested only.Not related to #191, which is about
orig_retinSH_SETUPCALLS. That one looks like it needs initialising a return value on every hooked call, so it seemed worth leaving to you rather than paying for it on the dispatch path.