Fix: use platform specific Pointer sizes #4594

ysbaddaden · 2017-06-19T09:03:10Z

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; with T=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

RX14 · 2017-06-19T09:27:59Z

src/intrinsics.cr

@@ -1,6 +1,6 @@
 lib Intrinsics
  fun debugtrap = "llvm.debugtrap"
-  {% if flag?(:x86_64) %}
+  {% if flag?(:x86_64) || flag(:aarch64) %}


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.

I thought of adding the :abi64 and :abi32 flags, based on the architecture, to solve this.

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.

RX14 · 2017-06-19T09:28:28Z

src/pointer.cr

@@ -405,7 +405,7 @@ struct Pointer(T)
  # ptr.address # => 5678
  # ```
  def self.new(address : Int)
-    new address.to_u64
+    new LibC::SizeT.new(0)


May we introduce ::SizeT type because it's very common one.

And ::OffsetT too.

May be this way: #4594 (comment) ?

funny-falcon · 2017-06-19T09:57:01Z

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

RX14 · 2017-06-19T10:43:18Z

We could put these types under Crystal::System since they're platform specific. Not sure if we want them instead of using SizeT though.

akzhan · 2017-06-19T11:13:03Z

No, pointers and pointer arithmetic are different use cases.

First, we already have Pointer type. So we need only SizeT to manipulate sizes, and OffsetT to manipulate offsets.

OffsetT is always signed because may be negative. http://www.cplusplus.com/reference/cstddef/ptrdiff_t/

SizeT is unsigned by design. http://www.cplusplus.com/reference/cstddef/size_t/

funny-falcon · 2017-06-19T11:40:24Z

Pointer#address have to return some integer, and best name for is Pointer::UIntPtr.

There could be issue on 32bit platform when allocated array is on the 2GB border: ie array start is below
border, and array end is above. It breaks end-to-start comparison with pointer address if address is signed.
Also there is PowerPC platform with full 64bit pointers (in contrast to 48bit on x86_64). Although I'm not convenient with how different OS allocates memory on PowerPC.

So Pointer#address have to be unsigned. And then it matches SizeT (signed). And expected name for is Pointer::UIntPtr.
Then signed variant is expected to be named Pointer::IntPtr, imho.

Note, names could be shorter: Pointer::Uint and Pointer::Int. But I think, it is a bit uglier, and could lead to confusion with ::Int. I could be mistaken, though.

akzhan · 2017-06-19T12:01:44Z

Pointer::Address and Pointer::Diff?

funny-falcon · 2017-06-19T12:12:38Z

I like Pointer::Diff.
But Pointer::Address serves both address and allocation amount.
On the other hand, Pointer::Diff could serve allocation amount well, if allocation of >= 2GB on 32bit platform will be forbidden. And then Diff is a diff of total allocated memory :-)

With Pointer::IntPtr and Pointer::UIntPtr methods Int#to_iptr and Int#to_uptr will have clear meaning.
But I could agree that this methods are not strictly necessary.

@ysbaddaden , what do you think about?

ysbaddaden · 2017-06-19T12:45:40Z

No.

ysbaddaden · 2017-06-19T13:19:11Z

Some aparte notes:

LibC::SizeT is unsigned, as per C functions / LLVM intrinsics:
Pointer#address is already unsigned. This may be the one place where an unsigned integer is expected:

typeof(Pointer(UInt8).null.address) # => UInt64 (on x86_64)
typeof(Pointer(UInt8).null.address) # => UInt32 (on ARM)

Pointer#address says nothing about allocation amount, it's a mere pointer to an address in memory. The allocated size must be retained separately —hence why one must use Slice not Pointer.

Again: Crystal isn't C nor Rust and doesn't need the numerous C types.

funny-falcon · 2017-06-19T13:39:41Z

Again: Crystal isn't C nor Rust and doesn't need the numerous C types.

No, it is not.
But since it is used (and not C nor Rust) to implement itself and its standard library, it have to have all convenient facilities for low-level.

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.

ysbaddaden · 2017-06-19T14:54:58Z

I should refrain from touching such internals. This is broken.

sdogruyol · 2017-06-19T15:08:46Z

Now, this was a weird PR 😕

funny-falcon · 2017-06-19T15:31:52Z

src/pointer.cr

@@ -52,6 +52,12 @@ struct Pointer(T)
    end
  end

+  {% if flag?(:x86_64) || flag?(:aarch64) %}
+    private alias SizeT = UInt64


why private? and why not alias SizeT = LibC::SizeT ?

RX14 · 2017-06-19T15:35:59Z

@ysbaddaden I don't really understand why you closed this.

akzhan · 2017-06-19T19:42:31Z

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.

ysbaddaden · 2017-06-20T13:13:53Z

@akzhan OffT is libc dependent and has nothing to do with this PR.

@RX14 compiler intrinsics seem to have constraints on pointer address and allocation size handlings, and that goes beyond my knowledge. Only the to_u32 calls in pointer.cr should be changed to be platform specific (maybe).

ysbaddaden added the topic:stdlib label Jun 19, 2017

RX14 requested changes Jun 19, 2017

View reviewed changes

ysbaddaden force-pushed the fix-platform-specific-pointer-sizes branch 2 times, most recently from 1aa790e to 3a9d2b2 Compare June 19, 2017 13:22

ysbaddaden force-pushed the fix-platform-specific-pointer-sizes branch from 3a9d2b2 to fd84715 Compare June 19, 2017 13:44

ysbaddaden closed this Jun 19, 2017

funny-falcon reviewed Jun 19, 2017

View reviewed changes

ysbaddaden deleted the fix-platform-specific-pointer-sizes branch June 20, 2017 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: use platform specific Pointer sizes #4594

Fix: use platform specific Pointer sizes #4594

ysbaddaden commented Jun 19, 2017

RX14 Jun 19, 2017

ysbaddaden Jun 19, 2017

RX14 Jun 19, 2017

RX14 Jun 19, 2017

akzhan Jun 19, 2017

akzhan Jun 19, 2017

funny-falcon Jun 19, 2017

funny-falcon commented Jun 19, 2017

RX14 commented Jun 19, 2017

akzhan commented Jun 19, 2017 •

edited

Loading

funny-falcon commented Jun 19, 2017 •

edited

Loading

akzhan commented Jun 19, 2017

funny-falcon commented Jun 19, 2017

ysbaddaden commented Jun 19, 2017

ysbaddaden commented Jun 19, 2017

funny-falcon commented Jun 19, 2017 •

edited

Loading

ysbaddaden commented Jun 19, 2017

sdogruyol commented Jun 19, 2017

funny-falcon Jun 19, 2017

RX14 commented Jun 19, 2017

akzhan commented Jun 19, 2017 •

edited

Loading

ysbaddaden commented Jun 20, 2017

Fix: use platform specific Pointer sizes #4594

Fix: use platform specific Pointer sizes #4594

Conversation

ysbaddaden commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funny-falcon commented Jun 19, 2017

RX14 commented Jun 19, 2017

akzhan commented Jun 19, 2017 • edited Loading

funny-falcon commented Jun 19, 2017 • edited Loading

akzhan commented Jun 19, 2017

funny-falcon commented Jun 19, 2017

ysbaddaden commented Jun 19, 2017

ysbaddaden commented Jun 19, 2017

funny-falcon commented Jun 19, 2017 • edited Loading

ysbaddaden commented Jun 19, 2017

sdogruyol commented Jun 19, 2017

Choose a reason for hiding this comment

RX14 commented Jun 19, 2017

akzhan commented Jun 19, 2017 • edited Loading

ysbaddaden commented Jun 20, 2017

akzhan commented Jun 19, 2017 •

edited

Loading

funny-falcon commented Jun 19, 2017 •

edited

Loading

funny-falcon commented Jun 19, 2017 •

edited

Loading

akzhan commented Jun 19, 2017 •

edited

Loading