memory leak in both cvar systems (Cvar and cvar_t)

[ghost]

ASAN reports a 200 bytes leak for each Cvar and ~30 bytes at the very start of the engine.

This memleak.patches.zip seems to fix that, or at least ASAN stop exiting immediately after launching daemon.

[illwieckz]

For those wondering, this is the content of the zip file, this is a whole branch, not just a patch in a zip to workaround GitHub restrictions:

   Date      Time    Attr         Size   Compressed  Name
------------------- ----- ------------ ------------  ------------------------
2023-09-12 19:33:03 .....         8180         2397  0001-fix-Wunused-macro.patch
2023-09-12 19:33:03 .....         1215          468  0002-readability.patch
2023-09-12 19:33:03 .....          967          489  0003-R_LoadDDSImageData-is-static.patch
2023-09-12 19:33:03 .....         2265          768  0004-static-things.patch
2023-09-12 19:33:03 .....          656          435  0005-fix-missing-variable-declaration.patch
2023-09-12 19:33:03 .....        32670         8834  0006-more-static-stuff.patch
2023-09-12 19:33:03 .....         3036         1235  0007-fix-uninitialised-condition.patch
2023-09-12 19:33:03 .....         1655          615  0008-define-missing-ctors.patch
2023-09-12 19:33:03 .....         1107          525  0009-remove-unused-global-vars.patch
2023-09-12 19:33:03 .....         2077          786  0010-move-some-cvars-to-static.patch
2023-09-12 19:33:03 .....        15378         3619  0011-fix-a-200-Byte-leak-per-cvar.patch
2023-09-12 19:33:03 .....        18371         4790  0012-another-cvar_t-leak-fix.patch
------------------- ----- ------------ ------------  ------------------------
2023-09-12 19:33:03              87577        24961  12 files

I haven't found an easy way to share any review on it so I have not started reviewing it more than that.

[ghost]

https://git-scm.com/docs/git-am

[ghost]

also, since I dug in my own daemon branches, some being months old, it seems this stuff contains rebase or commit errors. Only the 2 commits about "fixing a memleak" were likely intended, and should be squashed, as apparently a part of the latter one was intended to be in the 1st one.

[illwieckz]

[Unvanquished/daemon] wip/bmorel-928 ± git am .../bmorel/memleak.patches/*
Applying: fix -Wunused-macro
Applying: readability
Applying: R_LoadDDSImageData is static
Applying: static things
Applying: fix missing-variable-declaration
Applying: more static stuff
error: patch failed: src/common/cm/cm_trace.cpp:41
error: src/common/cm/cm_trace.cpp: patch does not apply
error: patch failed: src/engine/client/cl_main.cpp:130
error: src/engine/client/cl_main.cpp: patch does not apply
Patch failed at 0006 more static stuff
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

It is not said on which reference this is meant to be merged on.

[illwieckz]

Not providing reference target for merging means it's not possible to produce a branch to be merged, and then makes impossible to use some git tools, actually disabling git features and reducing the amount of solutions git can provide to solve problems, turning git into a stripped down and primitive version of itself, losing years of improvements. Especially it removes the ability to leverage merge conflict tools and workflows, both from core git features (like git mergetool), and from third party tools (like meld).

[illwieckz]

The documentation ( https://git-scm.com/docs/git-am ) says:

You can recover from this in one of two ways:

1. skip the current patch by re-running the command with the --skip option.
2. hand resolve the conflict in the working directory, and update the index file to bring it into a state that the patch should have produced. Then run the command with the --continue option.

Some people say ( https://stackoverflow.com/a/49695879/9131399 ):

Your Git does not have enough information to turn the patch into a three-way merge. Without an incomplete three-way merge sitting in your index, there is nothing for your git mergetool to bite into. You may have to manually apply the patch.

It looks like git am seems to provide less merge solving ability than the legacy patch tool.

[illwieckz]

Some code those patches apply to doesn't exist in this tree, example:

--- src/engine/client/cl_main.cpp
+++ src/engine/client/cl_main.cpp
@@ -144,7 +144,7 @@ cvar_t                 *cl_consoleCommand; //see also com_consoleCommand for ter
 struct rsa_public_key  public_key;
 struct rsa_private_key private_key;

-cvar_t             *cl_gamename;
+static cvar_t             *cl_gamename;

 cvar_t             *cl_altTab;

[illwieckz]

What is that???

[Unvanquished/daemon] wip/bmorel-928 ± git am .../bmorel/memleak.patches/0010-move-some-cvars-to-static.patch 
Applying: move some cvars to static
error: patch failed: src/engine/server/server.h:319
error: src/engine/server/server.h: patch does not apply
error: patch failed: src/engine/server/sv_client.cpp:40
error: src/engine/server/sv_client.cpp: patch does not apply
Patch failed at 0001 move some cvars to static
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

[Unvanquished/daemon] wip/bmorel-928 ± git am --abort

[Unvanquished/daemon] wip/bmorel-928 ± patch -p1 < ...bmorel/memleak.patches/0010-move-some-cvars-to-static.patch 
patching file src/engine/server/server.h
Reversed (or previously applied) patch detected!  Assume -R? [n] n
Apply anyway? [n] n
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file src/engine/server/server.h.rej
patching file src/engine/server/sv_client.cpp
Reversed (or previously applied) patch detected!  Assume -R? [n] n
Apply anyway? [n] n
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file src/engine/server/sv_client.cpp.rej

Ah no, this one is a mistake of me, which is actually expected when it's done by hand and not by machine.

[illwieckz]

Here is a reconstructed branch for this dumped set of patches without target branch:

• https://github.com/DaemonEngine/Daemon/pull/957

Dealing with this set of patches and reconstructing the branch cost me 2 hours of work. This would have been avoided with a simple git push on author's side (less than a minute of work).

At this point the code is not read, not reviewed, and not approved.

Those two hours of work have been lost and can't be used to review this code.