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

There is no check if heap size is within configured limits #59

Open
mkacperek opened this issue Sep 30, 2021 · 4 comments
Open

There is no check if heap size is within configured limits #59

mkacperek opened this issue Sep 30, 2021 · 4 comments

Comments

@mkacperek
Copy link

According to the documentation, max heap size = block_size * uint15 max, so for 8-byte blocks (by default), it's:

8 * 32767 = 2621424 B

This limitation is not mentioned in the "configuration" section of the README. I would consider this information important enough to mention there, but even more important would be adding a runtime check to verify if the numbers align.

I would consider adding an assert inside umm_init_heap() to check if requested size of the heap is within limits.

@rhempel what do you think about it? I can submit a PR if you agree it makes sense.

@rhempel
Copy link
Owner

rhempel commented Sep 30, 2021

In the README file we have this paragraph…

Each memory block holds 8 bytes, and there are up to 32767 blocks available, for about 256K of heap space. If that's not enough, you can always add more data bytes to the body of the memory block at the expense of free block size overhead.

… is that what you are looking for?

@mkacperek
Copy link
Author

mkacperek commented Oct 1, 2021

Sure, that's what I mean, but this information if quite hidden (in the "Background" section). And a bigger issue to me is that the heap size is never verified at runtime - if you initialize the heap with a bigger size, it seems to work at the first glance, but has some unexpected side effects later. To avoid unpleasant surprises, I think it's better to add a check and inform the user as early as possible that they got the size wrong.

The change I'm talking about is actually fairly small, so I just prepared the PR to show you what I meant ;) see: #60

@evanrichter
Copy link

For what it's worth, this also bit me for a while as I tried to initialize with too much memory...

The segfault occurred at the last statement in umm_init_heap when it attempted to write a value just before what I passed in as ptr, the beginning of the heap.

As a newcomer to this library, it would have been helpful to me if umm_init_heap documented it's assumptions/requirements inline with where it is defined, perhaps similar to this nearby function.

Once I got past that, I started to really enjoy this library. So thank you!

@evanrichter
Copy link

To clarify, I'm in favor of documentation update at the most. I would have to remove any runtime assertions in my use case.

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

3 participants