Make tracking allocator default to crashing on a bad free instead of adding to bad_free_array

[karl-zylinski]

This makes the tracking allocator default to crashing when a bad free occurs, instead of adding the bad free to an array. The crash will look something like this:

C:/code/playground/playground.odin(25:2) Tracking allocator error: Bad free of pointer 2003974722200

Rationale

Almost once a month I talk to someone who has misbehaving code, and I ask them if they check the bad free array. They say that they use tracking allocator but didn't think of checking the bad free array as required. After all, without tracking allocator bad frees tend to crash so one finds them anyways. But with the tracking allocator, bad frees just silently pass by if you forget to check the bad_free_array.

A better default behavior is therefore for tracking allocator to also crash, but still provide all the nice source code location information it has access to.

Also, not only does the old style of adding to bad_free_array cause people to miss bad frees. It can also cause memory corruption in their development build! Let me explain why that can happen, here's the old code that added stuff to the bad_free_array if it wasn't in the allocation_map:

if mode == .Free && old_memory != nil && old_memory not_in data.allocation_map {
	append(&data.bad_free_array, Tracking_Allocator_Bad_Free_Entry{
		memory = old_memory,
		location = loc,
	})
} else {
	result = data.backing.procedure(data.backing.data, mode, size, alignment, old_memory, old_size, loc) or_return
}

If you forgot to check bad_free_array, then all the bad frees were just silently added to the list. But to make things worse: Once in a blue moon the bad free would actually be of a valid pointer that exists in data.allocation_map. In that case the else block happens in the code above: The bad free just deallocates some random memory! This can of course happen with a crash-callback too. But it is very likely that it will have crashed on some allocation before getting to one of those rare cases that ends up in the else block.

Another problem is that core and vendor has several examples that look like this:

main :: proc() {
	track := mem.Tracking_Allocator{}
	mem.tracking_allocator_init(&track, context.allocator)

	context.allocator = mem.tracking_allocator(&track)

	demo()

	if len(track.allocation_map) > 0 {
		fmt.println("Leaks:")
		for _, v in track.allocation_map {
			fmt.printf("\t%v\n\n", v)
		}
	}
}

For anyone who learned about the tracking allocator from this example, they might never have known that the bad_free_array exists. So they just have lots of silent bad frees + possible memory corruptions.

I know people would probably see these bad frees if they ever make a build without tracking allocator (because then it would probably crash on those bad frees). But I wouldn't be surprised if there are people out there who just give up on Odin because their program is just misbehaving (due to tracking-allocator-i-didnt-check-the-bad-free-array-memory-corruption), but they can't figure out why.

Backwards compatibility

This new default behavior is implemented using a callback Tracking_Allocator.bad_free_callback. This callback can be overridden to tracking_allocator_bad_free_callback_add_to_array in order to have the old behavior.

The bad_free_array is still in the tracking allocator, making sure we don't break a lot of code at once. But in most programs it will not be used any more, so the "bad free checks" on shutdown won't do anything more. I can update the overview docs etc to remove the bad_free_array checks.

There were a few tests that explicitly checked bad_free_array, for those tests I use the callback that implements the old behavior.

[odiferousmint]

Hmm, probably should have led by example.

The only issue that I can see is that one cannot get all the bad frees in a codebase, you would have to compile/run per bad free; as in, run, find bad free, remove bad free, run, ... and so on.

The code snippet regarding allocation_map is a really popular one though, yeah.

[karl-zylinski]

The only issue that I can see is that one cannot get all the bad frees in a codebase, you would have to compile/run per bad free; as in, run, find bad free, remove bad free, run, ... and so on.

You can still use the callback tracking_allocator_bad_free_callback_add_to_array if you really want to. But bad frees aren't usually that many. I think this is a more safe default behavior for almost everyone.

[FourteenBrush]

Isn't the whole point of a tracking allocator that you ALWAYS print out the leaks and the bad frees... When setting up a tracking allocator, who doesn't copy the snippet thats above mem.tracking_allocator()?

This makes the tracking allocator default to crashing when a bad free occurs, instead of adding the bad free to an array. The crash will look something like this:

C:/code/playground/playground.odin(25:2) Tracking allocator error: Bad free of pointer 2003974722200

So now we find ourselves in a situation where, instead of seeing all bad frees, you only see one, and you now have to chase every single one with a new run..?
Sounds amusing to me, especially when you're having multiple sources of bad frees.. Perhaps that would need some clarification.

[karl-zylinski]

@FourteenBrush You usually don't have that many bad frees, crashing on them and fixing them is better default behavior. If one wants a list on shutdown + loop over bad free array, then one can still do that by setting track.bad_free_callback = tracking_allocator_bad_free_callback_add_to_array.

As I noted in the description of the PR, I run into people all the time that only check for leaks. Perhaps they set it up manually by just looping over allocation map. Or perhaps they used one of the setup examples found in core that don't check for bad frees.

The main problem is that the tracking (unknowingly to the user) changes the behavior of what happens on a bad free. With the default allocator it would almost always crash, so you find it. But with tracking allocator + not checking bad frees, all the bad frees suddenly go unnoticed, with the risk of a memory corruption (I explain how these memory corruptions happen in the description as well).

I've seen people develop programs for months and not knowing this. I didn't know about this for the first year of my Odin programming, and found out because I had random memory corruptions. It's just bad for the language on-boarding experience.

I don't have any exact data on how wide-spread the misuse of the tracking allocator is. But based on how many people I've helped add the bad free-check, I think if this get merged we'll hear from a lot of people who had no idea they had bad frees.