On Sat Feb 8 20:41:10 2025 +0000, Jinoh Kang wrote:
Thanks! I also largely agree that `current_modref` can go away and replaced with explicit parameters.[^1] I think, at least for testing, each commit replacing `current_modref` can be split into the following with much finer granularity:
- Introduce the `wm` parameter if not already, *but* don't use it just
yet. Instead, simply assert() that `wm == current_modref`. That way, if a function writes to `current_modref`, make the same changes to `wm` as well. This will catch any de-sync bugs due to incorrect arguments. 2. Replace all reads to `current_modref` with `wm`.[^2] If (1) passes tests, then this commit will pass them too. 3. If, **and only if,** no callees are known to use `current_modref`, replace all writes to `current_modref` with `current_moderef = (WINE_MODREF *)-0x10000;` or any other poison value. This will catch any unintended use of `current_modref` from one of its callees. Then, when we actually eliminate the `current_modref`, the poison value will finally go away. Once we have assurance that the commits splitted as above are indeed atomic and do no cause (test) regressions, I think we can proceed with the split. If you don't have time, I can volunteer. [^1]: In general, I believe that eliminating dynamically scoped (global or TLS) variabls in favor of lexically scoped variables (function parameters and locals) helps dissect data flow and make function behavior more predictable. [^2]: We just asserted that `current_modref == wm` in all circumstances, barring any callees failing to restore `current_modref` before returning due to e.g., SEH unwinding.
I split it differently. I also noticed that I messed up previous version in some cases (mixed importer with imported module), that should be fixed in the new version.
Here is a version that with asserts: https://gitlab.winehq.org/jacek/wine/-/merge_requests/18. I'm not sure what's the point of 3, a simple grep should be enough to ensure that there are no users (or, once it's removed, compiler verifies that for us).