Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider memory alloc API with explicit size param for m_free() #2

Closed
pfalcon opened this issue Dec 29, 2013 · 2 comments
Closed

Consider memory alloc API with explicit size param for m_free() #2

pfalcon opened this issue Dec 29, 2013 · 2 comments

Comments

@pfalcon
Copy link
Contributor

pfalcon commented Dec 29, 2013

When dealing with interpreters, object size size is either implicitly known (for example, basic representation of object has fixed size, like 8 or 32 bytes), or size is stored explicitly on interpreter's level (for example, array size needs to be stored in object header anyway). This means that it may be possible to optimize low-level memory allocation system by not storing memory chunk size (thus saving memory), instead relying that higher levels will pass size explicitly.

Quick look at current sources doesn't show that MicroPython is ideally suited for such optimization, but that's why I write - to propose to add that as a (non-immediate) design goal.

First steps towards that can be simple: make m_free() have signature m_free(ptr, size), and m_realloc(ptr, old_size, new_size), then while going over code to adjust call sites, see if what to pass as "size" param is obvious. In case it's not, well, pass 0, and leave larger refactors to someone who really will implement alternative allocators. Ultimately, there're good reasons for having explicit size field for all variable-length objects, IMHO.

@dpgeorge
Copy link
Member

Good idea.

In the current garbage collector (that runs properly only on the MCU), the object size is implicitly known. There is a bitmap at the very start of the heap which indicates which blocks (32 bytes each) are allocated. So an object size can be computed by working out how many blocks are contiguously allocated. There is also a bit to indicate "start of run of blocks".

@dpgeorge
Copy link
Member

Okay, I've implemented it as you suggested, with an explicit size parameter for m_free and m_realloc. The macros you should use are m_del, m_del_obj and m_renew.

pfalcon referenced this issue in pfalcon/pycopy Jun 19, 2016
Storing a chain of pbuf was an original design of @pfalcon's lwIP socket
module. The problem with storing just one, like modlwip does is that
"peer closed connection" notification is completely asynchronous and out of
band. So, there may be following sequence of actions:

1. pbuf #1 arrives, and stored in a socket.
2. pbuf #2 arrives, and rejected, which causes lwIP to put it into a
queue to re-deliver later.
3. "Peer closed connection" is signaled, and socket is set at such status.
4. pbuf #1 is processed.
5. There's no stored pbufs in teh socket, and socket status is "peer closed
connection", so EOF is returned to a client.
6. pbuf #2 gets redelivered.

Apparently, there's no easy workaround for this, except to queue all
incoming pbufs in a socket. This may lead to increased memory pressure,
as number of pending packets would be regulated only by TCP/IP flow
control, whereas with previous setup lwIP had a global overlook of number
packets waiting for redelivery and could regulate them centrally.
pfalcon added a commit that referenced this issue Jun 19, 2016
Storing a chain of pbuf was an original design of @pfalcon's lwIP socket
module. The problem with storing just one, like modlwip does is that
"peer closed connection" notification is completely asynchronous and out of
band. So, there may be following sequence of actions:

1. pbuf #1 arrives, and stored in a socket.
2. pbuf #2 arrives, and rejected, which causes lwIP to put it into a
queue to re-deliver later.
3. "Peer closed connection" is signaled, and socket is set at such status.
4. pbuf #1 is processed.
5. There's no stored pbufs in teh socket, and socket status is "peer closed
connection", so EOF is returned to a client.
6. pbuf #2 gets redelivered.

Apparently, there's no easy workaround for this, except to queue all
incoming pbufs in a socket. This may lead to increased memory pressure,
as number of pending packets would be regulated only by TCP/IP flow
control, whereas with previous setup lwIP had a global overlook of number
packets waiting for redelivery and could regulate them centrally.
EcmaXp added a commit to OpenPythons/micropython-opencom-legacy that referenced this issue Aug 28, 2016
turbinenreiter pushed a commit to turbinenreiter/micropython that referenced this issue Oct 16, 2016
aykevl referenced this issue in aykevl/micropython Oct 6, 2017
Fix up Makefile dependencies
neil-pozetroninc referenced this issue in pozetroninc/micropython Jan 11, 2018
Update from upstream repo micropython/micropython@master
tannewt referenced this issue in tannewt/circuitpython Aug 16, 2018
Merge last mod. in patch-1
tannewt referenced this issue in tannewt/circuitpython Jan 15, 2019
bergzand pushed a commit to bergzand/micropython that referenced this issue Nov 25, 2019
riot: Modify Pin to (PORT, PIN) tuple
cfobel pushed a commit to sci-bots/micropython that referenced this issue Nov 26, 2019
cfobel pushed a commit to sci-bots/micropython that referenced this issue Nov 26, 2019
xobs pushed a commit to xobs/micropython that referenced this issue Mar 24, 2020
xobs pushed a commit to xobs/micropython that referenced this issue Mar 24, 2020
xobs pushed a commit to xobs/micropython that referenced this issue Mar 24, 2020
merge with current master of circuitpython
xobs pushed a commit to xobs/micropython that referenced this issue Mar 24, 2020
xobs pushed a commit to xobs/micropython that referenced this issue Mar 24, 2020
xobs pushed a commit to xobs/micropython that referenced this issue May 15, 2020
xobs pushed a commit to xobs/micropython that referenced this issue May 19, 2020
Better handle //| and do __init__.c first.
irsla added a commit to irsla/micropython that referenced this issue Oct 1, 2020
this includes patch for PR6494 and allows both patch to co-exists

Users can then choose between 2 ways
   machine.deepsleep([ms])
   pyb.standby([ms], [wakeup Pins X1|X18], [wakeup trigger FALLING|RISING])

-- tested 1.13
machine.deepsleep(2000) # OK
machine.deepsleep(5000) # OK
machine.deepsleep(10000) # OK
machine.deepsleep(20000) # OK
machine.deepsleep() # OK waited 2 minutes

-- tested new firmware
machine.deepsleep(2000) # OK
machine.deepsleep(5000) # OK
machine.deepsleep(10000) # OK
machine.deepsleep(20000) # OK
machine.deepsleep() # OK waited 2 minutes

pyb.standby(0) # OK waited for 2 minutes
pyb.standby(2000) # OK
pyb.standby(5000) # OK
pyb.standby(10000) # OK
pyb.standby(20000) # OK
pyb.standby() # OK waited for 2 minutes

pyb.standby(0, pyb.WKUP_X1, pyb.WKUP_X1_FALLING) # OK wake up on interrupt within 10 sec
pyb.standby(2000, pyb.WKUP_X1, pyb.WKUP_X1_FALLING) # OK wakes up after 2sec
pyb.standby(5000, pyb.WKUP_X1, pyb.WKUP_X1_FALLING) # OK wakes up after 5 sec
pyb.standby(10000, pyb.WKUP_X1, pyb.WKUP_X1_FALLING) # OK wakes up after 10 sec and wakes up on interrupt within 10 sec
pyb.standby(20000, pyb.WKUP_X1, pyb.WKUP_X1_FALLING) # OK wakes up after 20 sec  and wakes up on interrupt within 20 sec

pyb.standby(0, pyb.WKUP_X18, pyb.WKUP_X18_FALLING) # OK wake up on interrupt within 10 sec
pyb.standby(2000, pyb.WKUP_X18, pyb.WKUP_X18_FALLING) # OK wakes up after 2sec
pyb.standby(5000, pyb.WKUP_X18, pyb.WKUP_X18_FALLING) # OK wakes up after 5 sec
pyb.standby(10000, pyb.WKUP_X18, pyb.WKUP_X18_FALLING) # OK wakes up after 10 sec and wakes up on interrupt within 10 sec
pyb.standby(20000, pyb.WKUP_X18, pyb.WKUP_X18_FALLING) # OK wakes up after 20 sec  and wakes up on interrupt within 20 sec

-- TO TEST
pyb.standby(0, pyb.WKUP_X1 | pyb.WKUP_X18, pyb.WKUP_X1_FALLING | pyb.WKUP_X18_FALLING) #
machine.deepsleep with upower.py
jgillick pushed a commit to jgillick/micropython that referenced this issue Nov 22, 2020
jgillick pushed a commit to jgillick/micropython that referenced this issue Nov 22, 2020
A crash like the following occurs in the unix port:
```
Program received signal SIGSEGV, Segmentation fault.
0x00005555555a2d7a in mp_obj_module_set_globals (self_in=0x55555562c860 <ulab_user_cmodule>, globals=0x55555562c840 <mp_module_ulab_globals>) at ../../py/objmodule.c:145
145	    self->globals = globals;
(gdb) up
#1  0x00005555555b2781 in mp_builtin___import__ (n_args=5, args=0x7fffffffdbb0) at ../../py/builtinimport.c:496
496	                mp_obj_module_set_globals(outer_module_obj,
(gdb)
micropython#2  0x00005555555940c9 in mp_import_name (name=824, fromlist=0x555555621f10 <mp_const_none_obj>, level=0x1) at ../../py/runtime.c:1392
1392	    return mp_builtin___import__(5, args);
```

I don't understand how it doesn't happen on the embedded ports, because
the module object should reside in ROM and the assignment of self->globals
should trigger a Hard Fault.

By checking VERIFY_PTR, we know that the pointed-to data is on the heap
so we can do things like mutate it.
jgillick pushed a commit to jgillick/micropython that referenced this issue Nov 22, 2020
It was incorrect to NULL out the pointer to our heap allocated buffer in
`reset`, because subsequent to framebuffer_reset, but while
the heap was still active, we could call `get_bufinfo` again,
leading to a fresh allocation on the heap that is about to be destroyed.

Typical stack trace:
```
#1  0x0006c368 in sharpdisplay_framebuffer_get_bufinfo
micropython#2  0x0006ad6e in _refresh_display
micropython#3  0x0006b168 in framebufferio_framebufferdisplay_background
micropython#4  0x00069d22 in displayio_background
micropython#5  0x00045496 in supervisor_background_tasks
micropython#6  0x000446e8 in background_callback_run_all
micropython#7  0x00045546 in supervisor_run_background_tasks_if_tick
micropython#8  0x0005b042 in common_hal_neopixel_write
micropython#9  0x00044c4c in clear_temp_status
micropython#10 0x000497de in spi_flash_flush_keep_cache
micropython#11 0x00049a66 in supervisor_external_flash_flush
micropython#12 0x00044b22 in supervisor_flash_flush
micropython#13 0x0004490e in filesystem_flush
micropython#14 0x00043e18 in cleanup_after_vm
micropython#15 0x0004414c in run_repl
micropython#16 0x000441ce in main
```
When this happened -- which was inconsistent -- the display would keep
some heap allocation across reset which is exactly what we need to avoid.

NULLing the pointer in reconstruct follows what RGBMatrix does, and that
code is a bit more battle-tested anyway.

If I had a motivation for structuring the SharpMemory code differently,
I can no longer recall it.

Testing performed: Ran my complicated calculator program over multiple
iterations without observing signs of heap corruption.

Closes: micropython#3473
jgillick pushed a commit to jgillick/micropython that referenced this issue Nov 22, 2020
JonBruchim added a commit to JonBruchim/micropython that referenced this issue Nov 26, 2020
# This is the 1st commit message:

special_accessors: runtime addition of descriptors could set

Adding descriptors at runtime doesn't set the special accessor flag.
or atleast the way I was doing it.

type_attr calls `check_for_special_accessors` (before adding the special accessor flag)  which will obviously return false (because no special accessor was added
on the original object)

I've replaced the validation to check if the new attribute to be added is an descriptor (if true - then set the special accessor flag)

Signed-off-by: Jonathan Bruchim <yonbruchim@gmail.com>

# This is the commit message micropython#2:

I've noticed that adding descriptors at runtime doesn't set the special accessor flag.
or atleast the way I was doing it.

type_attr calls `check_for_special_accessors` (before adding the special accessor flag)  which will obviously return false (because no special accessor was added
on the original object)

I've replaced the validation to check if the new attribute to be added is an descriptor (if true - then set the special accessor flag)

Signed-off-by: Jonathan Bruchim <yonbruchim@gmail.com>
dpgeorge pushed a commit that referenced this issue May 30, 2021
asan considers that memcmp(p, q, N) is permitted to access N bytes at each
of p and q, even for values of p and q that have a difference earlier.
Accessing additional values is frequently done in practice, reading 4 or
more bytes from each input at a time for efficiency, so when completing
"non_exist<TAB>" in the repl, this causes a diagnostic:

    ==16938==ERROR: AddressSanitizer: global-buffer-overflow on
    address 0x555555cd8dc8 at pc 0x7ffff726457b bp 0x7fffffffda20 sp 0x7fff
    READ of size 9 at 0x555555cd8dc8 thread T0
        #0 0x7ffff726457a  (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xb857a)
        #1 0x555555b0e82a in mp_repl_autocomplete ../../py/repl.c:301
        #2 0x555555c89585 in readline_process_char ../../lib/mp-readline/re
        #3 0x555555c8ac6e in readline ../../lib/mp-readline/readline.c:513
        #4 0x555555b8dcbd in do_repl /home/jepler/src/micropython/ports/uni
        #5 0x555555b90859 in main_ /home/jepler/src/micropython/ports/unix/
        #6 0x555555b90a3a in main /home/jepler/src/micropython/ports/unix/m
        #7 0x7ffff619a09a in __libc_start_main ../csu/libc-start.c:308
        #8 0x55555595fd69 in _start (/home/jepler/src/micropython/ports/uni

    0x555555cd8dc8 is located 0 bytes to the right of global variable
    'import_str' defined in '../../py/repl.c:285:23' (0x555555cd8dc0) of
    size 8
      'import_str' is ascii string 'import '

Signed-off-by: Jeff Epler <jepler@gmail.com>
sblaksono pushed a commit to sblaksono/micropython that referenced this issue Jun 15, 2021
Add new image functions to qstrs
ksekimoto pushed a commit to ksekimoto/micropython that referenced this issue Jul 16, 2021
asan considers that memcmp(p, q, N) is permitted to access N bytes at each
of p and q, even for values of p and q that have a difference earlier.
Accessing additional values is frequently done in practice, reading 4 or
more bytes from each input at a time for efficiency, so when completing
"non_exist<TAB>" in the repl, this causes a diagnostic:

    ==16938==ERROR: AddressSanitizer: global-buffer-overflow on
    address 0x555555cd8dc8 at pc 0x7ffff726457b bp 0x7fffffffda20 sp 0x7fff
    READ of size 9 at 0x555555cd8dc8 thread T0
        #0 0x7ffff726457a  (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xb857a)
        micropython#1 0x555555b0e82a in mp_repl_autocomplete ../../py/repl.c:301
        micropython#2 0x555555c89585 in readline_process_char ../../lib/mp-readline/re
        micropython#3 0x555555c8ac6e in readline ../../lib/mp-readline/readline.c:513
        micropython#4 0x555555b8dcbd in do_repl /home/jepler/src/micropython/ports/uni
        micropython#5 0x555555b90859 in main_ /home/jepler/src/micropython/ports/unix/
        micropython#6 0x555555b90a3a in main /home/jepler/src/micropython/ports/unix/m
        micropython#7 0x7ffff619a09a in __libc_start_main ../csu/libc-start.c:308
        micropython#8 0x55555595fd69 in _start (/home/jepler/src/micropython/ports/uni

    0x555555cd8dc8 is located 0 bytes to the right of global variable
    'import_str' defined in '../../py/repl.c:285:23' (0x555555cd8dc0) of
    size 8
      'import_str' is ascii string 'import '

Signed-off-by: Jeff Epler <jepler@gmail.com>
j-devel pushed a commit to AnimaGUS-minerva/micropython that referenced this issue Aug 26, 2021
ports/riot: fix include in mpconfigport.h
RetiredWizard pushed a commit to RetiredWizard/micropython that referenced this issue Apr 16, 2023
fix rp2040 with new shared irq usb handler
RetiredWizard pushed a commit to RetiredWizard/micropython that referenced this issue Feb 12, 2024
Update changes of the last month for new hardware models
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants