Use GC.malloc_atomic with GC.realloc, not Pointer#realloc

[HertzDevil]

This PR ensures that pointers allocated using GC.malloc and GC.malloc_atomic are only reallocated using GC.realloc. This is necessary for the interpreter to run on Windows properly, and may also be relevant on other systems if eventually the interpreter's GC is distinct from the compiler's own GC.

Pointer.malloc and Pointer#realloc are defined as primitives. For compiled code, these internally call the top-level funs __crystal_malloc64, __crystal_malloc_atomic64, and __crystal_realloc64, which in turn forward to the class methods on GC, so Pointer and GC's methods happen to be exactly the same:

crystal/src/gc.cr

Lines 16 to 47 in a5054ce

	# :nodoc:
	fun __crystal_malloc64(size : UInt64) : Void*
	{% if flag?(:bits32) %}
	if size > UInt32::MAX
	raise ArgumentError.new("Given size is bigger than UInt32::MAX")
	end
	{% end %}
	GC.malloc(LibC::SizeT.new(size))
	end
	# :nodoc:
	fun __crystal_malloc_atomic64(size : UInt64) : Void*
	{% if flag?(:bits32) %}
	if size > UInt32::MAX
	raise ArgumentError.new("Given size is bigger than UInt32::MAX")
	end
	{% end %}
	GC.malloc_atomic(LibC::SizeT.new(size))
	end
	# :nodoc:
	fun __crystal_realloc64(ptr : Void*, size : UInt64) : Void*
	{% if flag?(:bits32) %}
	if size > UInt32::MAX
	raise ArgumentError.new("Given size is bigger than UInt32::MAX")
	end
	{% end %}
	GC.realloc(ptr, LibC::SizeT.new(size))
	end

For interpreted code, the same primitives call Pointer.malloc and Pointer#realloc from the compiler itself:

crystal/src/compiler/crystal/interpreter/instructions.cr

Lines 1004 to 1015 in a5054ce

	pointer_malloc: {
	operands: [element_size : Int32],
	pop_values: [size : UInt64],
	push: true,
	code: Pointer(Void).malloc(size * element_size).as(UInt8*),
	},
	pointer_realloc: {
	operands: [element_size : Int32],
	pop_values: [pointer : Pointer(UInt8), size : UInt64],
	push: true,
	code: pointer.realloc(size * element_size),
	},

Currently, the compiler on Windows is statically linked, whereas LibGC inside interpreted code would be dynamically loaded (using #11573 and #12140); there would be two copies of the garbage collector which do not conflict like on Unix-like systems. Consequently, allocating memory from one allocator and then reallocating it in another is undefined behavior.

All instances of GC.malloc in the repository are already paired to GC.realloc. Note also that adding @[Primitive] to GC's class methods doesn't work because these have different signatures than the methods in Pointer.

[straight-shoota]

I'm wondering if we should also take steps to reduce the error probability through the API. Pointer#realloc is very inviting to be used, even if the pointer was allocated via GC.malloc. The fact that it doesn't make a difference in most cases, doesn't help either.

An idea might be to make Pointer#realloc a class method Pointer.realloc. So there'd be Pointer.malloc/Pointer.realloc analog to GC.malloc/GC.realloc.

[asterite]

There's another one in io_spec.cr

I think the main issue is that there's no Pointer.malloc_atomic, so we have to use GC.malloc_atomic. So maybe we should consider adding that. It would remove all uses of GC.realloc.

We could even hide GC.malloc and GC.realloc completely with that.

[straight-shoota]

Pointer.malloc uses GC.malloc_atomic when the pointer type is known to contain no pointers.
So I suppose it could simply be implemented in stdlib by delegating to Pointer(UInt8).malloc:

struct Pointer(T)
  def self.malloc_atomic(size : UInt64)
    Pointer(UInt8).malloc(size * sizeof(T)).as(T*)
  end
end

[HertzDevil]

It would be odd if Pointer.malloc may allocate atomically and Pointer.malloc_atomic always does so. For consistency we would need yet another method that never allocates atomically. In particular, this seems to be required here:

crystal/src/log/metadata.cr

Lines 33 to 38 in f160040

	def self.new(parent : Metadata? = nil, entries : NamedTuple | Hash = NamedTuple.new)
	data_size = instance_sizeof(self) + sizeof(Entry) * {entries.size + (parent.try(&.max_total_size) || 0) - 1, 0}.max
	data = GC.malloc(data_size).as(self)
	data.setup(parent, entries)
	data
	end

This raises another question actually. If the interpreter calls Pointer(Void).malloc for any use of Pointer.malloc in interpreted code, wouldn't that always choose the atomic version? Is that fine?

[straight-shoota]

Pointer(Void).malloc is actually not atomic because Void is considered as potentially containing inner pointers (because it's just unknown what type it actually is).

crystal/src/compiler/crystal/codegen/types.cr

Lines 48 to 61 in f160040

	# Returns `true` if the type has inner pointers.
	# This is useful to know because if a type doesn't have
	# inner pointers we can use `malloc_atomic` instead of
	# `malloc` in `Pointer.malloc` for a tiny performance boost.
	#
	# This behaviour is documented in Pointer.malloc
	def has_inner_pointers?
	case self
	when .void?
	# We consider Void to have pointers, so doing
	# Pointer(Void).malloc(...).as(ReferenceType)
	# will consider potential inner pointers as such.
	true
	when PointerInstanceType

So it's definitely fine as it's safe. But it might miss some performance possibilities.