-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: use platform specific Pointer sizes #4594
Fix: use platform specific Pointer sizes #4594
Conversation
src/intrinsics.cr
Outdated
@@ -1,6 +1,6 @@ | |||
lib Intrinsics | |||
fun debugtrap = "llvm.debugtrap" | |||
{% if flag?(:x86_64) %} | |||
{% if flag?(:x86_64) || flag(: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.
Would it not make more sense to check the type of LibC::SizeT
here? It's beyond the scope of this PR because I don't think there's a way because of how aliases are handled in macros right now, but it might be a good idea as we add more 64bit platforms.
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 thought of adding the :abi64
and :abi32
flags, based on the architecture, to solve this.
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 thought of that too. Then I thought that maybe SizeT
could possibly be the wrong type if the platform's libc is weird. But I think that's such an abstract worry that adding flags is probably the best solution.
src/pointer.cr
Outdated
@@ -405,7 +405,7 @@ struct Pointer(T) | |||
# ptr.address # => 5678 | |||
# ``` | |||
def self.new(address : Int) | |||
new address.to_u64 | |||
new LibC::SizeT.new(0) |
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.
Typo: 0
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.
May we introduce ::SizeT type because it's very common one.
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.
And ::OffsetT too.
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.
May be this way: #4594 (comment) ?
Still, why not expose LibC::SizeT? struct Pointer
alias UIntPtr = LibC::SizeT # used for allocation size
alias IntPtr = LibC::SSizeT # used for pointer arithmetic
# note: looks like there is no need in SSizeT/PtrDiffT/IntPtrT distinction like in C,
# cause Crystal is supported only on platforms where they are equal
end
struct Int
def to_uptr
Pointer::UIntPtr.new(self)
end
def to_iptr
Pointer::IntPtr.new(self)
end
end |
We could put these types under Crystal::System since they're platform specific. Not sure if we want them instead of using SizeT though. |
No, pointers and pointer arithmetic are different use cases. First, we already have
|
There could be issue on 32bit platform when allocated array is on the 2GB border: ie array start is below So Note, names could be shorter: |
|
I like With @ysbaddaden , what do you think about? |
No. |
Some aparte notes:
typeof(Pointer(UInt8).null.address) # => UInt64 (on x86_64)
typeof(Pointer(UInt8).null.address) # => UInt32 (on ARM)
Again: Crystal isn't C nor Rust and doesn't need the numerous C types. |
1aa790e
to
3a9d2b2
Compare
No, it is not. |
The current implementation uses UInt64 for pointer allocation sizes, but UInt32 for operating on pointer sizes (memcpy, memmove, memset) despite using the 64 bit LLVM intrinsics; and is illogical: if we allocate a large section of memory, we must be able to zero and copy/move it, whereas 32bit platforms should deal with UInt32, the maximum possible value, not larger ones. This patch proposes to constraint to a platform specific size only (i.e. LibC::SizeT) in order to always use UInt32 on 32 bit platforms, and UInt64 on 64 bit platforms. We cannot clamp sizes to Int32::MAX, because some allocations (e.g. Array(Int32).new(Int32::MAX)) can be larger than allowed. Also fixes usage of 64bit variant of LLVM intrinsics on AArch64.
3a9d2b2
to
fd84715
Compare
I should refrain from touching such internals. This is broken. |
Now, this was a weird PR 😕 |
@@ -52,6 +52,12 @@ struct Pointer(T) | |||
end | |||
end | |||
|
|||
{% if flag?(:x86_64) || flag?(:aarch64) %} | |||
private alias SizeT = UInt64 |
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.
why private? and why not alias SizeT = LibC::SizeT
?
@ysbaddaden I don't really understand why you closed this. |
AFAIS Simple update of OffT to LongLong to support 64bit offset in files require a lot of changes in stdlib, and so on. It should be a major move of Crystal to 64bit. |
The current implementation uses UInt64 for pointer allocation sizes, but UInt32 for operating on pointer sizes (memcpy, memmove, memset) despite using the 64 bit LLVM intrinsics; and was illogical: if we allocate a large section of memory, we must be able to zero and copy/move it, whereas 32bit platforms should deal with UInt32, the maximum possible value, not larger ones.
This patch proposes to constraint to a platform specific size only (i.e.
LibC::SizeT
) in order to always use UInt32 on 32 bit platforms, and UInt64 on 64 bit platforms.We cannot clamp sizes to Int32::MAX, because some allocations (e.g.
Array(Int32).new(Int32::MAX))
can be larger than allowed.Alternatively we could disallow such allocations, thus allowing to clamp the allocation sizes? I mean, that's 2 million entries; the example array above with
T=Int32
would be 8GB; withT=Int32|Int64
that's 32GB... Yet, since it could end up being an issue with external C programs, I (reluctantly) admit we should allow a platform specific sizes.This patch also fixes the use of 64bit variants of LLVM intrinsics on AArch64 (memcpy, memmove, memset).
Fixes #4589