Fix m_realloc/m_free when MICROPY_MALLOC_USES_ALLOCATED_SIZE is set

[SamantazFox]

I wanted to build CircuitPython with MICROPY_MEM_STATS enabled to debug some out of memory issues I had, but encountered a few compilation errors.

MICROPY_MEM_STATS requires MICROPY_MALLOC_USES_ALLOCATED_SIZE to be set, which in turn causes m_realloc, m_realloc_maybe and m_free to take an extra argument: the previously allocated size.

I fixed what I could with my limited knowledge of that code. Some places of the code still lack a proper support for MICROPY_MALLOC_USES_ALLOCATED_SIZE.

[tannewt]

This is fine with me. Please ensure pre-commit passes though.

[SamantazFox]

I think I also found a bug in pre-commit:

pre-commit hook(s) made changes.
If you are seeing this message in CI, reproduce locally with: `pre-commit run --all-files`.
To run `pre-commit` as part of git workflow, use `pre-commit install`.
All changes made by hooks:
diff --git a/shared-module/atexit/__init__.c b/shared-module/atexit/__init__.c
index 6cc13e9..3960732 100644
--- a/shared-module/atexit/__init__.c
+++ b/shared-module/atexit/__init__.c
@@ -47,7 +47,7 @@ void shared_module_atexit_register(mp_obj_t *func, size_t n_args, const mp_obj_t
 
     callback = (atexit_callback_t *)m_realloc(callback,
         #if MICROPY_MALLOC_USES_ALLOCATED_SIZE
-        callback_len * sizeof(cb), // Old size
+        callback_len *sizeof(cb),  // Old size
         #endif
         (callback_len + 1) * sizeof(cb));
 

[SamantazFox]

And a nice case of "it works on my machine" :/

For the record, the python venv is a fresh install, and I have the same version (4.1.0) of pre-commit than the CI.

[jepler]

Hi! thanks for the PR. We don't mind seeing changes for alternate configuration options like MICROPY_MEM_STATS if the impact of the required changes can easily be managed.

we kinda get to live with whatever uncrustufy does; and the CI runs the canonically correct version. Even if it looks wrong to your eyes and mine :-/ Unlike the ideal for pre-commit, we depend on an externally installed uncrustify program; I think micropython-uncrustify from pip install -r requirements.txt.

def check_uncrustify_version():
    version = subprocess.check_output(
        ["uncrustify", "--version"], encoding="utf-8", errors="replace"
    )
    if version < "Uncrustify-0.71":
        raise SystemExit(f"codeformat.py requires Uncrustify 0.71 or newer, got {version}")

If there's a newer version of uncrustify that formats this differently we may need to add a check for a too-new uncrustify version. What does uncrustify --version print for you? We have this note in a document but we don't seem to be checking it:

BUILDING.md: * uncrustify version 0.71 (0.72 is also tested and OK; 0.75 is not OK)

The other thing I wonder about here is whether these sites should (mostly) change to use gc_alloc / gc_free. Since we ALWAYS have gc enabled in any circuitpython build, we can depend on it being available; and its call signature doesn't change depending on MICROPY_MALLOC_USES_ALLOCATED_SIZE. On the other hand, does that mean MICROPY_MEM_STATS doesn't track gc allocations in the first place? I'm not super familiar with this functionality.

[SamantazFox]

What does uncrustify --version print for you?

Well, Uncrustify-373ed72

The other thing I wonder about here is whether these sites should (mostly) change to use gc_alloc / gc_free. Since we ALWAYS have gc enabled in any circuitpython build, we can depend on it being available; and its call signature doesn't change depending on MICROPY_MALLOC_USES_ALLOCATED_SIZE. On the other hand, does that mean MICROPY_MEM_STATS doesn't track gc allocations in the first place? I'm not super familiar with this functionality.

Oh, that's a great question. I assumed that m_alloc and friends were using the GC.

[jepler]

I don't see any problem with taking these changes, if they're useful to someone. A CI-time build that enables USES_ALLOCATED_SIZE would be helpful to try to prevent these sites breaking, but can be done as a separate PR if it someone wants to put in the work.

done

[jepler]

Thanks @SamantazFox !