Unregister processes when they die

[UncleGrumpy]

Adds the function globalcontext_maybe_unregister_process_id to globalcontext.c to allow removing the appropriate entry from the registered atoms table when the associated context is destroyed.

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

[bettio]

this may cause a memory corruption, we have to free(registered_process). Right now this code works just by chance.

[UncleGrumpy]

That was my first inclination too, but globalcontext_unregister_process we free'd item not registered_process. I believe we had a similar discussion then.

[UncleGrumpy]

Indeed, we had a lengthy discussion... #364.
I can change it here, if you feel it reads better that way. If so, we should also change it in globalcontext_unregister_process for consistency.

[UncleGrumpy]

In fact it must be free(item), when I changed it to free(registered_process) th CI for MacOS builds fails with the following error:

/Users/runner/work/AtomVM/AtomVM/src/libAtomVM/globalcontext.c:327:18: error: passing 'const struct RegisteredProcess *' to parameter of type 'void *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
            free(registered_process);
                 ^~~~~~~~~~~~~~~~~~
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk/usr/include/malloc/_malloc.h:42:18: note: passing argument to parameter here
void     free(void *);
                    ^

[bettio]

So I think we should remove that const from const struct RegisteredProcess *registered_process.

[bettio]

In order to check which is the right thing to do I suggest doing this change:

 struct RegisteredProcess
 {
+    uint64_t dummy1;
+    uint32_t dummy2;
+
     struct ListHead registered_processes_list_head;
 
     int atom_index;

or even this change:

 struct RegisteredProcess
 {
-    struct ListHead registered_processes_list_head;
-
     int atom_index;
     int local_process_id;
+
+    struct ListHead registered_processes_list_head;
 };

I think you will end with both to a crash, that means that the code is now depending on the struct memory layout.
If you do free(registered_process); you make sure you are going to free at offset 0, which is the same pointer that is returned from malloc.

Does it make sense to you?

[UncleGrumpy]

Yes. I was not thinking about how future API changes could break this in bad ways. Assuming that removing the const from const struct RegisteredProcess *registered_process will pass the MacOS CI workflows, would you like me to fix the free(item) in globalcontext_unregister_process as well? I can add it in a separate bug fix commit, without flattening.

[bettio]

In order to check which is the right thing to do I suggest doing this change:

 struct RegisteredProcess
 {
+    uint64_t dummy1;
+    uint32_t dummy2;
+
     struct ListHead registered_processes_list_head;
 
     int atom_index;

or even this change:

 struct RegisteredProcess
 {
-    struct ListHead registered_processes_list_head;
-
     int atom_index;
     int local_process_id;
+
+    struct ListHead registered_processes_list_head;
 };

I think you will end with both to a crash, that means that the code is now depending on the struct memory layout.
If you do free(registered_process); you make sure you are going to free at offset 0, which is the same pointer that is returned from malloc.

Does it make sense to you?

[UncleGrumpy]

Yes. I was not thinking about how future API changes could break this in bad ways. I went ahead and fixed the free(item) in globalcontext_unregister_process in a separate bug fix commit, without flattening.