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

Deactivate memory tracking once freed #421

Merged
merged 2 commits into from
Aug 27, 2024
Merged

Conversation

krtab
Copy link
Collaborator

@krtab krtab commented Aug 26, 2024

No description provided.

@krtab krtab requested a review from filipeom August 26, 2024 16:42
@zapashcanon
Copy link
Member

zapashcanon commented Aug 27, 2024

I'm not sure I understand this one. Could you explain when such a pointer is going to be used?

Also, I'm surprised the code in src/libc/stdlib.c is not changed. Shouldn't you change realloc and free along the way ?

Copy link
Collaborator

@filipeom filipeom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Just cast the calls to owi_free to void to get rid of the import type errors.

For example, change free in

void free(void *ptr) { owi_free(ptr); }

To

void free(void *ptr) { (void)owi_free(ptr); }

@filipeom
Copy link
Collaborator

filipeom commented Aug 27, 2024

I'm not sure I understand this one. Could you explain when such a pointer is going to be used?

The rust allocator does a bunch of offset reads with a heap chunk when deallocating stuff. So in order to avoid having false positive heap overflows e when deallocating things in rust we need dealloc to deregister the pointer from Owi's chunk table and return an untracked pointer so that the rust allocator can do nasty stuff with it.

Also, I'm surprised the code in src/libc/stdlib.c is not changed. Shouldn't you change realloc and free along the way ?

free and realloc can remain the same I think

@zapashcanon
Copy link
Member

I see. It would be great if we could start to test these things in Owi itself...

@krtab there is a conflict that needs to be resolved before I can merge

@filipeom
Copy link
Collaborator

I see. It would be great if we could start to test these things in Owi itself...

What do you suggest? I think for this case in particular it may be a bit difficult since we don't have the rust frontend atm

@zapashcanon
Copy link
Member

I'm not suggesting anything for the current PR, but yes, it would be nice to have a small Rust frontend integrated in Owi for us to test this.

@zapashcanon
Copy link
Member

Thanks!

@zapashcanon zapashcanon merged commit b581e5d into OCamlPro:main Aug 27, 2024
1 of 4 checks passed
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.

3 participants