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
Fix malloc and realloc for 64 bits platforms #4960
Conversation
As a side note, we should have a way to know, via a |
src/pointer.cr
Outdated
end | ||
|
||
def clone | ||
self | ||
end | ||
|
||
private def bytesize(count) | ||
{% if flag?(:x86_64) %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this apply also to aarch64
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using LibC::SizeT.new(count)
would avoid the flags altogether. Maybe introducing SizeT is unavoidable after all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely if the semantics of pointer address is to sleazy be 64bits, this method should also always use 64bits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to flag?(:bits64)
. The 32 bits branch needs to check that the value isn't bigger than UInt32::MAX
.
@@ -332,6 +334,7 @@ module Crystal | |||
mod = info.mod | |||
push_debug_info_metadata(mod) unless @debug.none? | |||
|
|||
# puts mod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left over
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/gc/boehm.cr
Outdated
@@ -74,6 +74,21 @@ fun __crystal_realloc(ptr : Void*, size : UInt32) : Void* | |||
LibGC.realloc(ptr, size) | |||
end | |||
|
|||
# :nodoc: | |||
fun __crystal_malloc64(size : UInt64) : Void* | |||
LibGC.malloc(size) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 call
ed and never invoke
d in the codegen, so we should fix that later.
|
||
# :nodoc: | ||
fun __crystal_malloc_atomic64(size : UInt64) : Void* | ||
LibC.malloc(size) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Why not And It greatly increases portability. |
src/gc/boehm.cr
Outdated
@@ -76,16 +76,34 @@ end | |||
|
|||
# :nodoc: | |||
fun __crystal_malloc64(size : UInt64) : Void* | |||
{% if flag?(:bits32) %} | |||
if size > UInt32::MAX |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I'm sure there are more places in the stdlib where |
@RX14 it can't be done because We have to wait for a new release to change other places too. |
src/gc/boehm.cr
Outdated
LibGC.malloc(size) | ||
end | ||
|
||
# :nodoc: | ||
fun __crystal_malloc_atomic64(size : UInt64) : Void* | ||
{% if flag?(:bits32) %} | ||
if size > UInt32::MAX | ||
raise ArgumentError.new("Negative size") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
src/gc/boehm.cr
Outdated
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
src/gc/boehm.cr
Outdated
@@ -78,7 +78,7 @@ end | |||
fun __crystal_malloc64(size : UInt64) : Void* | |||
{% if flag?(:bits32) %} | |||
if size > UInt32::MAX | |||
raise ArgumentError.new("Negative size") | |||
raise ArgumentError.new("size bigger than UInt32::MAX given") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, use an upper case letter to start the exception message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size
refers to the argument name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, then maybe Given size to malloc is bigger ....
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/gc/boehm.cr
Outdated
@@ -78,7 +78,7 @@ end | |||
fun __crystal_malloc64(size : UInt64) : Void* | |||
{% if flag?(:bits32) %} | |||
if size > UInt32::MAX | |||
raise ArgumentError.new("Negative size") | |||
raise ArgumentError.new("Given size is bigger than UInt32::MAX given") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one given
(at the end) too much ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops!
src/gc/boehm.cr
Outdated
@@ -89,7 +89,7 @@ end | |||
fun __crystal_malloc_atomic64(size : UInt64) : Void* | |||
{% if flag?(:bits32) %} | |||
if size > UInt32::MAX | |||
raise ArgumentError.new("Negative size") | |||
raise ArgumentError.new("Given size is bigger than UInt32::MAX given") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
src/gc/boehm.cr
Outdated
@@ -100,7 +100,7 @@ end | |||
fun __crystal_realloc64(ptr : Void*, size : UInt64) : Void* | |||
{% if flag?(:bits32) %} | |||
if size > UInt32::MAX | |||
raise ArgumentError.new("Negative size") | |||
raise ArgumentError.new("Given size is bigger than UInt32::MAX given") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
fun __crystal_malloc64(size : UInt64) : Void* | ||
{% if flag?(:bits32) %} | ||
if size > UInt32::MAX | ||
raise ArgumentError.new("Given size is bigger than UInt32::MAX") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it make sense to just return nil
here. malloc may return nil to signal that there is no enough free continuous memory to allocate. Although there is a hard limit here, since the codegen is unable to use this yet, we could at least handle the nil
in the generated code at generic_malloc(type)
. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean, Pointer(Void).new(0)
? That will be a segfault after use.
I think these comments, and some of the last fixes, are a bit unrelated to the original issue. I don't want to keep adding unrelated changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @asterite a NULL
or Pointer(Void).new(0)
. I meant to return NULL
as a better behavior that could be handled without changing the llvm calls for llvm invokes as suggested #4960 (comment) . As is, the exceptions won't be handled as per the linked comment.
I'm ok to defer this until #4345, I was trying to avoid adding code that seems to won't be actually used as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure handling the NULL
requires changes in generic_malloc(type)
. Let's wait for #4345 then to change the call to invokes probably then and leave more code in crystal and less in the llvm builder.
src/pointer.cr
Outdated
count.to_u64 * sizeof(T) | ||
{% else %} | ||
if count > UInt32::MAX | ||
raise ArgumentError.new("Negative count") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
One more ✅ and I'll merge. The wiki says that if a core team member sends a PR that at least one other has to approve it, but I'd like at least two other team members to review PRs, independently of who sent the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but take my approval with a pinch salt since my knowledge of LLVM is too slim to fully understand some parts of this changeset :).
Fixes #4589
There were several issues:
Pointer#copy_from
and others would cast the givensize
withto_u32
, which would not work well in 64 bits__crystal_malloc
and__crysta_realloc
hadUInt32
hardcoded.For 1, the
LLVM::Builder
type had amalloc
call that would define and call C'smalloc
call. However, it generatedmalloc(i32)
. We used that at the beginning when we didn't have a GC and eventually replaced it with our own__crystal_malloc
and carried thei32
type. So, I removed the buggyLLVM::Builder#malloc
method and instead definemalloc
(if it's not already defined) with the proper type (though this is only used in tests, there's a comment about this in the diff).1 it's tricky because we can't simply change
__crystal_malloc
's type because current code will stop compiling. The solution is to define another function,__crystal_malloc64
, and at the same time change the compiler to search for this method.Just as a note,
Pointer#malloc
is implemented as a primitive that invokes__crystal_malloc
or__crystal_malloc_atomic
. Also, when you writeSomeClass.new
,__crystal_malloc
is invoked.After the next release we can change
__crystal_malloc
's type and change the compiler to use__crystal_malloc
, and in a next release we can remove the function with the 64 name suffix (though this is not a big deal as this name is internal and only used by crystal and some core std stuff).All failing code in this comment #4589 (comment) now pass with a newly compiled compiler.
I don't pretend this to solve all the problems regarding pointers and memory allocation, but it's a start.