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

Fix malloc and realloc for 64 bits platforms #4960

merged 6 commits into from Sep 14, 2017

Conversation

@asterite
Copy link
Member

@asterite asterite commented Sep 12, 2017

Fixes #4589

There were several issues:

  1. Pointer#copy_from and others would cast the given size with to_u32, which would not work well in 64 bits
  2. Even fixing 1 this was not enough to fix the whole problem. The main issue is that __crystal_malloc and __crysta_realloc had UInt32 hardcoded.

For 1, the LLVM::Builder type had a malloc call that would define and call C's malloc call. However, it generated malloc(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 the i32 type. So, I removed the buggy LLVM::Builder#malloc method and instead define malloc (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 write SomeClass.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.

@asterite asterite self-assigned this Sep 12, 2017
@asterite
Copy link
Member Author

@asterite asterite commented Sep 12, 2017

As a side note, we should have a way to know, via a flag, if code is 64 bits or not. Right now we have to rely on the existance of a few flags (like x86_64)

src/pointer.cr Outdated
end

def clone
self
end

private def bytesize(count)
{% if flag?(:x86_64) %}
Copy link
Contributor

@luislavena luislavena Sep 12, 2017

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?

Copy link
Contributor

@ysbaddaden ysbaddaden Sep 13, 2017

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?

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.

Surely if the semantics of pointer address is to sleazy be 64bits, this method should also always use 64bits?

Copy link
Member Author

@asterite asterite Sep 13, 2017

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
Copy link
Member

@bcardiff bcardiff Sep 12, 2017

Choose a reason for hiding this comment

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

left over

Copy link
Member Author

@asterite asterite Sep 13, 2017

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)
Copy link
Member

@bcardiff bcardiff Sep 12, 2017

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

@asterite asterite Sep 13, 2017

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.


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

@kostya kostya Sep 13, 2017

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

@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.

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

@akzhan
Copy link
Contributor

@akzhan akzhan commented Sep 13, 2017

Why not alloc_sizet that always be restricted with LibC::SizeT?

And LibC::PtrdiffT should be added too.

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
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

@asterite asterite Sep 13, 2017

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

@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.

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

@RX14
Copy link
Contributor

@RX14 RX14 commented Sep 13, 2017

I'm sure there are more places in the stdlib where bits32 and bits64 could be used, would it make sense to include the flag additions + stdlib refactors in a commit in this PR?

@asterite
Copy link
Member Author

@asterite asterite commented Sep 13, 2017

@RX14 it can't be done because bits32 and bits64 are only defined by the next compiler, and don't exist in the current one. For Pointer it's OK because the code was buggy anyway, so it's like bits64 is false right now.

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")
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

@asterite asterite Sep 13, 2017

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")
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.

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")
Copy link
Contributor

@bew bew Sep 13, 2017

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?

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.

yes please.

Copy link
Member Author

@asterite asterite Sep 13, 2017

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

Copy link
Contributor

@bew bew Sep 13, 2017

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 ....?

Copy link
Member Author

@asterite asterite Sep 13, 2017

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")
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.

one given (at the end) too much ;)

Copy link
Member Author

@asterite asterite Sep 13, 2017

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")
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.

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")
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.

ditto

fun __crystal_malloc64(size : UInt64) : Void*
{% if flag?(:bits32) %}
if size > UInt32::MAX
raise ArgumentError.new("Given size is bigger than UInt32::MAX")
Copy link
Member

@bcardiff bcardiff Sep 13, 2017

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?

Copy link
Member Author

@asterite asterite Sep 13, 2017

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.

Copy link
Contributor

@bew bew Sep 13, 2017

Choose a reason for hiding this comment

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

I disagree @bcardiff, I think malloc/realloc should always return a pointer to a valid memory zone. To ensure that, it should raise an ArgumentError on bad argument (like here), or when not enough memory (some discussions about that in #4345)

Copy link
Member

@bcardiff bcardiff Sep 13, 2017

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.

Copy link
Member

@bcardiff bcardiff Sep 13, 2017

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")
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.

One moar copy/pasted exception message to fix.

Copy link
Member Author

@asterite asterite Sep 13, 2017

Choose a reason for hiding this comment

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

Fixed!

@asterite
Copy link
Member Author

@asterite asterite commented Sep 14, 2017

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.

Copy link
Member

@mverzilli mverzilli left a comment

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 :).

@asterite asterite merged commit 464268b into crystal-lang:master Sep 14, 2017
2 checks passed
@asterite asterite added this to the Next milestone Sep 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment