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

build.zig: Support ignore_free #625

Closed
wants to merge 1 commit into from
Closed

Conversation

plajjan
Copy link
Contributor

@plajjan plajjan commented Mar 18, 2024

Add new enable_ignore_free option to build.zig and fix an actual bug related to the implementation of ignore_free. That it was a comment must have been a mistake. I made the original comment that adds the UNUSED_ARG(p) line so no idea how I messed that up, perhaps some rebase issue, but this fixes it!

Add new enable_ignore_free option to build.zig and fix an actual bug
related to the implementation of ignore_free. That it was a comment must
have been a mistake.
malloc.c Show resolved Hide resolved
@ivmai
Copy link
Owner

ivmai commented Mar 18, 2024

I don't like adding this option.
I suggest to use CFLAGS_EXTRA instead:
e.g. cmake -DCFLAGS_EXTRA=-DIGNORE_FREE .. && cmake --build .
or cmake "-DCFLAGS_EXTRA=-D IGNORE_FREE" .. && cmake --build .
Okay?

@plajjan
Copy link
Contributor Author

plajjan commented Mar 18, 2024

Okay. Might I ask about the reasoning?

It seems to me that I need ignore free when used with redirect malloc and other people's libraries that do explicit frees, no?

@ivmai
Copy link
Owner

ivmai commented Mar 18, 2024

It seems to me that I need ignore free when used with redirect malloc

Why? What's the purpose?
Generally malloc/free/realloc redirection is used for 2 purposes:

  • leak detection
  • force 3rd-party code e.g. containers (which uses malloc) to allocate object from the GC heap so that to have such objects traceable by GC (i.e. able to store objects allocated by GC_malloc to a container which implementation uses plain malloc).

And, normally, in these cases, free should be redirected to real GC_free.

IGNORE_FREE is intended to be used merely for test/debug purposes.

@ivmai
Copy link
Owner

ivmai commented Mar 19, 2024

fix an actual bug related to the implementation of ignore_free

Done by commit d76963c.
Thank you!

@ivmai ivmai closed this Mar 19, 2024
@plajjan
Copy link
Contributor Author

plajjan commented Mar 19, 2024

Hmm ok. I started noticing problems when I implemented fs related functions in Acton. Previously all test programs had been running fine but doing uv_fs_scandir() would segfault the program. I don't have the backtraces in front of me right now (but should be able to reproduce) but something led me to believe to a double-free-looking scenario or perhaps it's running GC_free on something allocated via normal malloc. Enabling IGNORE_FREE fixed the problem so the segfault went away.

I sort of just presumed actually that the preferred option when doing link time redircetion would be to just ignore calls to free() and instead let the GC collect all unreachable objects. Why do we actually want to support explicit free() at all?

@plajjan
Copy link
Contributor Author

plajjan commented Mar 19, 2024

Maybe the better solution is to stop using malloc redirect and do explicit calls to GC_MALLOC instead. I have actually patched all the external libraries used in Acton to support custom allocators so I can select which malloc to use but have run into problem when trying to remove malloc-redirect in GC, it seems something is misaligned...

@ivmai
Copy link
Owner

ivmai commented Mar 19, 2024

Hmm ok. I started noticing problems when I implemented fs related functions in Acton. Previously all test programs had been running fine but doing uv_fs_scandir() would segfault the program. I don't have the backtraces in front of me right now (but should be able to reproduce) but something led me to believe to a double-free-looking scenario or perhaps it's running GC_free on something allocated via normal malloc. Enabling IGNORE_FREE fixed the problem so the segfault went away.

I sort of just presumed actually that the preferred option when doing link time redircetion would be to just ignore calls to free() and instead let the GC collect all unreachable objects. Why do we actually want to support explicit free() at all?

You could add an issue if it is not difficult to reproduce. A sort of use after free or double free, or alloc-free allocator mismatch, as you pointed out.

@ivmai
Copy link
Owner

ivmai commented Mar 19, 2024

Maybe the better solution is to stop using malloc redirect and do explicit calls to GC_MALLOC instead. I have actually patched all the external libraries used in Acton to support custom allocators so I can select which malloc to use but have run into problem when trying to remove malloc-redirect in GC, it seems something is misaligned...

May be. Ideally to be able to configure which allocator to use dynamically (which is not always possible because of static constructors) or at library build time.

In reality, there's no universal advice.

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.

2 participants