Skip to content

Conversation

@leecannon
Copy link
Contributor

This PR makes debug builds of the compiler use GeneralPurposeAllocator even when linking libc.

Without this change debug builds on targets that always link libc or when the compiler links libc for tracy would use c_allocator meaning the issues checked by GeneralPurposeAllocator.deinit() are not caught.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Need to make sure the API boundaries between LLVM and stage2 are using the correct allocator, as well as stage1 and stage2.

For example maybe one side allocs and another frees; they need to use the same allocator. Not sure if there is a problem, but it needs to be audited before merging this.

@leecannon
Copy link
Contributor Author

leecannon commented Jan 23, 2022

Looks fine at the stage2 <-> stage1 boundary:

  • zig_stage1_create has a matching zig_stage1_destroy that frees with the same allocator.
  • zig_stage1_build_object - uses buf_create_from_mem and buf_init_from_mem to duplicate the slices given to it by stage2, no other allocated memory is passed to zig_stage1_build_object except stage1->main_pkg which is allocated into an ArenaAllocator in updateStage1Module and not freed on the c++ side

My knowledge of the boundary between stage2 and LLVM is a too lacking to comfortably say if all is well.

However looking through the code I can't find anywhere where we pass memory allocated by LLVM to a zig allocator or where we pass memory allocated by a zig allocator to LLVM without there also being a matching free of that memory on the zig side.

@andrewrk
Copy link
Member

andrewrk commented Jan 25, 2022

Thanks for looking at the API boundaries.

After spending a ton of time debugging issues during #10656, I don't want to unconditionally use GPA in debug mode. It is useful to use valgrind sometimes which works better with libc allocator. I used this several times to troubleshoot something. However, I also used GPA to troubleshoot stuff sometimes.

I think the defaults should stay what they are, and overriding to use GPA should be a new -Dforce-gpa option to the build script. So default allocator will be chosen like this:

  1. -Dforce-gpa can be used to force GeneralPurposeAllocator (this is what I am requesting you to implement)
  2. -Dforce-link-libc can be used to force libc allocator (this already exists)
  3. If linking libc, libc allocator
  4. otherwise, GPA

@leecannon
Copy link
Contributor Author

Added -Dforce-gpa as requested

@leecannon leecannon requested a review from andrewrk January 25, 2022 13:46
@andrewrk andrewrk merged commit fbe5336 into ziglang:master Jan 25, 2022
@andrewrk
Copy link
Member

Thanks 👍

@leecannon leecannon deleted the gpa_in_debug branch January 25, 2022 23:35
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