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

Fix malloc and realloc for 64 bits platforms #4960

Merged
merged 6 commits into from Sep 14, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 26 additions & 0 deletions spec/std/pointer_spec.cr
Expand Up @@ -318,4 +318,30 @@ describe "Pointer" do
ptr = Pointer(Int32).new(123)
ptr.clone.should eq(ptr)
end

{% if flag?(:bits32) %}
it "raises on copy_from with size bigger than UInt32::MAX" do
ptr = Pointer(Int32).new(123)

expect_raises(ArgumentError) do
ptr.copy_from(ptr, UInt32::MAX.to_u64 + 1)
end
end

it "raises on move_from with size bigger than UInt32::MAX" do
ptr = Pointer(Int32).new(123)

expect_raises(ArgumentError) do
ptr.move_from(ptr, UInt32::MAX.to_u64 + 1)
end
end

it "raises on clear with size bigger than UInt32::MAX" do
ptr = Pointer(Int32).new(123)

expect_raises(ArgumentError) do
ptr.clear(UInt32::MAX.to_u64 + 1)
end
end
{% end %}
end
91 changes: 65 additions & 26 deletions src/compiler/crystal/codegen/codegen.cr
Expand Up @@ -8,9 +8,9 @@ require "./llvm_builder_helper"
module Crystal
MAIN_NAME = "__crystal_main"
RAISE_NAME = "__crystal_raise"
MALLOC_NAME = "__crystal_malloc"
MALLOC_ATOMIC_NAME = "__crystal_malloc_atomic"
REALLOC_NAME = "__crystal_realloc"
MALLOC_NAME = "__crystal_malloc64"
MALLOC_ATOMIC_NAME = "__crystal_malloc_atomic64"
REALLOC_NAME = "__crystal_realloc64"
PERSONALITY_NAME = "__crystal_personality"
GET_EXCEPTION_NAME = "__crystal_get_exception"

Expand Down Expand Up @@ -131,9 +131,11 @@ module Crystal
@rescue_block : LLVM::BasicBlock?
@malloc_fun : LLVM::Function?
@malloc_atomic_fun : LLVM::Function?
@c_malloc_fun : LLVM::Function?
@sret_value : LLVM::Value?
@cant_pass_closure_to_c_exception_call : Call?
@realloc_fun : LLVM::Function?
@c_realloc_fun : LLVM::Function?
@main_llvm_context : LLVM::Context
@main_llvm_typer : LLVMTyper
@main_module_info : ModuleInfo
Expand Down Expand Up @@ -1821,48 +1823,47 @@ module Crystal
end

def malloc(type)
generic_malloc(type) { malloc_fun }
generic_malloc(type) { crystal_malloc_fun }
end

def malloc_atomic(type)
generic_malloc(type) { malloc_atomic_fun }
generic_malloc(type) { crystal_malloc_atomic_fun }
end

def generic_malloc(type)
size = type.size

if malloc_fun = yield
size = trunc(type.size, llvm_context.int32)
pointer = call malloc_fun, size
bit_cast pointer, type.pointer
else
builder.malloc type
pointer = call_c_malloc size
end

bit_cast pointer, type.pointer
end

def array_malloc(type, count)
generic_array_malloc(type, count) { malloc_fun }
generic_array_malloc(type, count) { crystal_malloc_fun }
end

def array_malloc_atomic(type, count)
generic_array_malloc(type, count) { malloc_atomic_fun }
generic_array_malloc(type, count) { crystal_malloc_atomic_fun }
end

def generic_array_malloc(type, count)
size = trunc(type.size, llvm_context.int32)
count = trunc(count, llvm_context.int32)
size = builder.mul size, count
size = builder.mul type.size, count

if malloc_fun = yield
pointer = call malloc_fun, size
memset pointer, int8(0), size
bit_cast pointer, type.pointer
else
pointer = builder.array_malloc(type, count)
void_pointer = bit_cast pointer, llvm_context.void_pointer
memset void_pointer, int8(0), size
pointer
pointer = call_c_malloc size
end

memset pointer, int8(0), size
bit_cast pointer, type.pointer
end

def malloc_fun
def crystal_malloc_fun
@malloc_fun ||= @main_mod.functions[MALLOC_NAME]?
if malloc_fun = @malloc_fun
check_main_fun MALLOC_NAME, malloc_fun
Expand All @@ -1871,7 +1872,7 @@ module Crystal
end
end

def malloc_atomic_fun
def crystal_malloc_atomic_fun
@malloc_atomic_fun ||= @main_mod.functions[MALLOC_ATOMIC_NAME]?
if malloc_fun = @malloc_atomic_fun
check_main_fun MALLOC_ATOMIC_NAME, malloc_fun
Expand All @@ -1880,6 +1881,47 @@ module Crystal
end
end

def crystal_realloc_fun
@realloc_fun ||= @main_mod.functions[REALLOC_NAME]?
if realloc_fun = @realloc_fun
check_main_fun REALLOC_NAME, realloc_fun
else
nil
end
end

# We only use C's malloc in tests that don't require the prelude,
# so they don't require the GC. Outside tests these are not used,
# and __crystal_* functions are invoked instead.

def call_c_malloc(size)
size = trunc(size, llvm_context.int32) unless @program.bits64?
call c_malloc_fun, size
end

def c_malloc_fun
malloc_fun = @c_malloc_fun = @main_mod.functions["malloc"]? || begin
size = @program.bits64? ? @main_llvm_context.int64 : @main_llvm_context.int32
@main_mod.functions.add("malloc", ([size]), @main_llvm_context.void_pointer)
end

check_main_fun "malloc", malloc_fun
end

def call_c_realloc(buffer, size)
size = trunc(size, llvm_context.int32) unless @program.bits64?
call c_realloc_fun, [buffer, size]
end

def c_realloc_fun
realloc_fun = @c_realloc_fun = @main_mod.functions["realloc"]? || begin
size = @program.bits64? ? @main_llvm_context.int64 : @main_llvm_context.int32
@main_mod.functions.add("realloc", ([@main_llvm_context.void_pointer, size]), @main_llvm_context.void_pointer)
end

check_main_fun "realloc", realloc_fun
end

def memset(pointer, value, size)
pointer = cast_to_void_pointer pointer
call @program.memset(@llvm_mod, llvm_context), [pointer, value, trunc(size, llvm_context.int32), int32(4), int1(0)]
Expand All @@ -1890,13 +1932,10 @@ module Crystal
end

def realloc(buffer, size)
@realloc_fun ||= @main_mod.functions[REALLOC_NAME]?
if realloc_fun = @realloc_fun
realloc_fun = check_main_fun REALLOC_NAME, realloc_fun
size = trunc(size, llvm_context.int32)
if realloc_fun = crystal_realloc_fun
call realloc_fun, [buffer, size]
else
call @program.realloc(@llvm_mod, llvm_context), [buffer, size]
call_c_realloc buffer, size
end
end

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/crystal/codegen/llvm_typer.cr
Expand Up @@ -564,7 +564,7 @@ module Crystal
end

def size_t
if @program.has_flag?("x86_64") || @program.has_flag?("aarch64")
if @program.bits64?
@llvm_context.int64
else
@llvm_context.int32
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/crystal/codegen/primitives.cr
Expand Up @@ -858,7 +858,7 @@ class Crystal::CodeGenVisitor
when CharType
inst.alignment = 4
else
inst.alignment = @program.has_flag?("x86_64") || @program.has_flag?("aarch64") ? 8 : 4
inst.alignment = @program.bits64? ? 8 : 4
end
end

Expand Down
10 changes: 10 additions & 0 deletions src/compiler/crystal/semantic/flags.cr
Expand Up @@ -21,6 +21,10 @@ class Crystal::Program
flags.includes?(name)
end

def bits64?
has_flag?("bits64")
end

private def parse_flags(flags_name)
set = flags_name.map(&.downcase).to_set
set.add "darwin" if set.any?(&.starts_with?("macosx")) || set.any?(&.starts_with?("darwin"))
Expand All @@ -37,6 +41,12 @@ class Crystal::Program
set.add "armhf" if set.includes?("gnueabihf")
end

if set.includes?("x86_64") || set.includes?("aarch64")
set.add "bits64"
else
set.add "bits32"
end

set
end
end
33 changes: 33 additions & 0 deletions src/gc/boehm.cr
Expand Up @@ -74,6 +74,39 @@ fun __crystal_realloc(ptr : Void*, size : UInt32) : Void*
LibGC.realloc(ptr, size)
end

# :nodoc:
fun __crystal_malloc64(size : UInt64) : Void*
{% if flag?(:bits32) %}
if size > UInt32::MAX
Copy link
Contributor

@RX14 RX14 Sep 13, 2017

Choose a reason for hiding this comment

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

Thinking about this more generally, is the automatic integer size conversion around lib a mistake in the language design, because it hides the situations where 64bit integers can be downcoverted to 32bits, and so hides where you should make a check for truncation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but it's a different issue and of much bigger scope: #3103

I don't think we can fix this in this same PR. For now we have to do manual checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it's far out of scope for this PR.

raise ArgumentError.new("Negative size")
end
{% end %}

LibGC.malloc(size)
Copy link
Member

Choose a reason for hiding this comment

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

Since malloc64 is always used for allocation, and LibC / GC malloc use SizeT. The conversion from UInt64 to SizeT happens implicitly when calling a C method? How is the failure handle when request something over SizeT on 32bits arch?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a check and raise here. Unfortunately that raise can't be captured because it's always called and never invoked in the codegen, so we should fix that later.

end

# :nodoc:
fun __crystal_malloc_atomic64(size : UInt64) : Void*
{% if flag?(:bits32) %}
if size > UInt32::MAX
raise ArgumentError.new("Negative size")
Copy link
Contributor

@Sija Sija Sep 13, 2017

Choose a reason for hiding this comment

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

Isn't Negative size a copy paste here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, right, it should be a different message

end
{% end %}

LibGC.malloc_atomic(size)
end

# :nodoc:
fun __crystal_realloc64(ptr : Void*, size : UInt64) : Void*
{% if flag?(:bits32) %}
if size > UInt32::MAX
raise ArgumentError.new("Negative size")
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

end
{% end %}

LibGC.realloc(ptr, size)
end

module GC
def self.init
LibGC.set_handle_fork(1)
Expand Down
15 changes: 15 additions & 0 deletions src/gc/null.cr
Expand Up @@ -13,6 +13,21 @@ fun __crystal_realloc(ptr : Void*, size : UInt32) : Void*
LibC.realloc(ptr, size)
end

# :nodoc:
fun __crystal_malloc64(size : UInt64) : Void*
LibC.malloc(size)
end

# :nodoc:
fun __crystal_malloc_atomic64(size : UInt64) : Void*
LibC.malloc(size)
Copy link
Contributor

Choose a reason for hiding this comment

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

here should be LibGC.malloc_atomic?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the null gc, which doesn't do any gc, it just forwards to the libc malloc.

end

# :nodoc:
fun __crystal_realloc64(ptr : Void*, size : UInt64) : Void*
LibC.realloc(ptr, size)
end

module GC
def self.init
end
Expand Down
4 changes: 2 additions & 2 deletions src/io/memory.cr
Expand Up @@ -101,7 +101,7 @@ class IO::Memory
slice.copy_to(@buffer + @pos, count)

if @pos > @bytesize
Intrinsics.memset((@buffer + @bytesize).as(Void*), 0_u8, (@pos - @bytesize).to_u32, 0_u32, false)
(@buffer + @bytesize).clear(@pos - @bytesize)
end

@pos += count
Expand All @@ -125,7 +125,7 @@ class IO::Memory
(@buffer + @pos).value = byte

if @pos > @bytesize
Intrinsics.memset((@buffer + @bytesize).as(Void*), 0_u8, (@pos - @bytesize).to_u32, 0_u32, false)
(@buffer + @bytesize).clear(@pos - @bytesize)
end

@pos += 1
Expand Down
13 changes: 0 additions & 13 deletions src/llvm/builder.cr
Expand Up @@ -89,19 +89,6 @@ class LLVM::Builder
Value.new LibLLVM.build_load(self, ptr, name)
end

def malloc(type, name = "")
# check_type("malloc", type)

Value.new LibLLVM.build_malloc(self, type, name)
end

def array_malloc(type, value, name = "")
# check_type("array_malloc", type)
# check_value(value)

Value.new LibLLVM.build_array_malloc(self, type, value, name)
end

{% for method_name in %w(gep inbounds_gep) %}
def {{method_name.id}}(value, indices : Array(LLVM::ValueRef), name = "")
# check_value(value)
Expand Down
18 changes: 15 additions & 3 deletions src/pointer.cr
Expand Up @@ -242,7 +242,7 @@ struct Pointer(T)
raise ArgumentError.new("Negative count") if count < 0

if self.class == source.class
Intrinsics.memcpy(self.as(Void*), source.as(Void*), (count * sizeof(T)).to_u32, 0_u32, false)
Intrinsics.memcpy(self.as(Void*), source.as(Void*), bytesize(count), 0_u32, false)
else
while (count -= 1) >= 0
self[count] = source[count]
Expand All @@ -255,7 +255,7 @@ struct Pointer(T)
raise ArgumentError.new("Negative count") if count < 0

if self.class == source.class
Intrinsics.memmove(self.as(Void*), source.as(Void*), (count * sizeof(T)).to_u32, 0_u32, false)
Intrinsics.memmove(self.as(Void*), source.as(Void*), bytesize(count), 0_u32, false)
else
if source.address < address
copy_from source, count
Expand Down Expand Up @@ -495,10 +495,22 @@ struct Pointer(T)
# ptr.to_slice(6) # => Slice[0, 0, 0, 13, 14, 15]
# ```
def clear(count = 1)
Intrinsics.memset(self.as(Void*), 0_u8, (count * sizeof(T)).to_u32, 0_u32, false)
Intrinsics.memset(self.as(Void*), 0_u8, bytesize(count), 0_u32, false)
end

def clone
self
end

private def bytesize(count)
{% if flag?(:bits64) %}
count.to_u64 * sizeof(T)
{% else %}
if count > UInt32::MAX
raise ArgumentError.new("Negative count")
Copy link
Contributor

Choose a reason for hiding this comment

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

One moar copy/pasted exception message to fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

end

count.to_u32 * sizeof(T)
{% end %}
end
end