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

Make RPLs functional #325

Closed
wants to merge 2 commits into from
Closed

Make RPLs functional #325

wants to merge 2 commits into from

Conversation

Exzap
Copy link
Contributor

@Exzap Exzap commented Jun 22, 2023

This PR addresses two problems:

  • LR isn't correctly restored in __rpl_start leading to an endless loop or crash
  • wutmalloc calls MEMGetSizeForMBlockExpHeap on pointers returned from MEMAllocFromDefaultHeap for realloc. But the underlying heap is not an ExpHeap anymore. Thus any realloc call causes havoc.

I reworked wutmalloc to not assume any underlying heap implementation and instead add a header to each allocation to store the required information about size and alignment.

I also opted to add a canary value in the allocation header that is checked on each free/realloc operation. On corruption it will OSFatal(). I do believe that it's better to have controlled crashing with a message than to silently ignore it and crash further down the line. Even in release builds.

With these changes RPLs are functional. At least during my testing I have never encountered any problems.

On a side note, it may be wise to rename wut_malloc to wut_rpl_malloc in order to make it more clear that it's only used for RPLs. RPXs seem to use a custom sbrk based heap. If maintainers agree with this I can do a follow-up PR.

@Maschell
Copy link
Contributor

Maschell commented Jun 22, 2023

Worth mentioning without thinking about any implications yet:
wutmalloc is currently used by plugins/modules in Aroma as well

@GaryOderNichts
Copy link
Contributor

On a side note, it may be wise to rename wut_malloc to wut_rpl_malloc in order to make it more clear that it's only used for RPLs. RPXs seem to use a custom sbrk based heap. If maintainers agree with this I can do a follow-up PR.

wutmalloc can be used by RPXs by overriding __preinit_user and calling __init_wut_malloc. This will cause wut_malloc.o to be linked in.

@fincs
Copy link
Member

fincs commented Jun 24, 2023

Is there any way we can fast path wutmalloc when the default heap is not overridden by the application, and avoid the overhead in that case?

@Exzap
Copy link
Contributor Author

Exzap commented Jun 26, 2023

We could add a boolean parameter to __init_wut_malloc which restores the old behavior of assuming ExpHeap. But then we have ABI breakage and two separate code paths in every function so this feels a bit shoehorned.

I think a better approach would be to keep the original wut_malloc as-is and introduce wut_rpl_malloc specifically for the purpose of RPLs where the underlying heap implementation isn't known.

@Exzap
Copy link
Contributor Author

Exzap commented Jul 6, 2024

Superseded by #388

@Exzap Exzap closed this Jul 6, 2024
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

Successfully merging this pull request may close these issues.

4 participants