Make tests work on 32-bit #486

helje5 · 2018-06-12T13:11:02Z

Ported test code to 32-bit Linux (Raspi)

Motivation:

Nothing will work without tests!

Modifications:

NIO has edge conditions wrt Int32.max. Mostly workaround this by using lower values, or adjusting types.
Also replaced some 0xdeadbeef w/ 0x1337beef in the tests to make stuff compile.

Result:

The test suite runs through w/o hard crashes. One test doesn't run at all (testAllocationOfReallyBigByteBuffer), another one fails regularly.

Test Suite 'All tests' failed at 2018-06-12 12:55:45.411
Executed 711 tests, with 2 failures (0 unexpected) in 608.633 (608.633) seconds

But at least the suite runs through now!

Motivation: Make tests compile on Raspberry Pi. Modifications: Change tests so that they compile on 32-bit machines, like Raspi. Replaced 0xDEADBEEF which doesn't fit into an Int32, w/ 0x1337BEEF. Also use TimeAmount.Value in one test. Result: Tests will compile on Raspi ...

Motivation: The testAllocationOfReallyBigByteBuffer fails because ByteBuffer has some UInt32 dependencies (which is >Int on 32-bit) Modifications: Fallback to Int32 in some places, reduce really big buffer size on 32-bit. Result: The test survives a few Swift Int asserts, but on Raspi it still eventually fails in mempcy. Don't know why.

Motivation: Make tests work on 32-bit. Modifications: Reduced the `writevLimitBytes` from Int32.max to Int32.max / 4. Int32 produces too many edge conditions on 32-bit. This is not optimal, but should be fine in practice. Result: More ChannelTests run.

Motivation: Make tests work on 32-bit. Modifications: Reduced `maxNIOFrameSize` from Int32.max to Int32.max / 4 on `arm` platforms. Int32 produces too many edge conditions on 32-bit. This is not optimal, but should be fine in practice. Result: WebSocket tests run.

Motivation: If the tests don't run, nothing else will. Modifications: - make the testAllocationOfReallyBigByteBuffer always fail on Raspi, produces a low-level fault which needs to be investigated eventually: Thread 1 "swift-nioPackag" received signal SIGSEGV, Segmentation fault. __memcpy_neon () at ../sysdeps/arm/armv7/multiarch/memcpy_impl.S:362 362 ../sysdeps/arm/armv7/multiarch/memcpy_impl.S: No such file or directory. - Reduce "lotsOfData" tests from Int32.max to Int32.max/8 bytes. Full Int32.max hits too many edge conditions in NIO. Result: Test suite runs through w/o hard crashes on Raspi 32-bit / Swift 4.1. Test Suite 'All tests' failed at 2018-06-12 12:55:45.411 Executed 711 tests, with 2 failures (0 unexpected) in 608.633 (608.633) seconds real 15m19.413s user 35m29.010s sys 1m15.440s

Motivation: Make it compile on 64-bit Intel again. Modifications: Just some type casting. Result: Builds on 64-bit again.

swift-nio-bot · 2018-06-12T13:11:04Z

Can one of the admins verify this patch?

swift-nio-bot · 2018-06-12T13:11:04Z

Can one of the admins verify this patch?

Lukasa

Thanks @helje5! I've left a few small notes inline but this mostly looks great.

Lukasa · 2018-06-12T15:18:26Z

Sources/NIO/ByteBuffer-int.swift

+            let max = UInt32(Int32.max)
+        #else
+            let max = UInt32.max
+        #endif


This change violates the doc comment up above, so we should probably change this.

Incidentally, I assume the reason for this change is that malloc accepts Int on 32-bit platforms, so returning UInt32.max is not a super great plan, so it may be worth changing the doc comment to say something like:

"Returns the next power of two unless that would overflow, in which case UInt32.max (on 64-bit systems) or Int32.max (on 32-bit systems) is returned. The returned value is always safe to be cast to Int and passed to malloc on all platforms."

@helje5 / @Lukasa wait. malloc takes size_t which is unsigned, so it should be CSize == UInt imho. So I don't quite understand this change

This was probably to support the sysMalloc(Int) issue. Looks like you changed that upstream to sysMalloc(size_t), so it may not be necessary anymore.

Lukasa · 2018-06-12T15:26:43Z

Sources/NIO/Socket.swift

+            // Note(hh): This is not a _proper_ fix, but necessary because
+            //           other places extend on that. Should be fine in
+            //           practice on 32-bit platforms.
+            return Int(Int32.max / 4)


Hrm, I don't love this. AFAICT this change works around the fact that we have a test that validates what happens if we attempt to write more than writevLimitBytes, which does some unguarded addition on this value.

For the test in ChannelTests.swift, it may be best to change it on 32-bit systems to create the buffer at writevLimitBytes and adjust the expected return values. That's the largest possible allocated buffer on 32-bit NIO anyway.

No, there are more NIO issues wrt that. I.e. ByteBuffer uses UInt32 and nextPoW2 to round up the size, which subsequently fails when casting backt to malloc, which wants an Int.
It simply is no good to live on that edge on 32-bit platforms. IMO it is not worthwhile to adjust all of NIO just for the 32-bit edge case. And you don't do similar edge-checks for 64-bit either (but stick to Int32.max ;-) )

ByteBuffer uses UInt32 and nextPoW2 to round up the size, which subsequently fails when casting backt to malloc

Doesn't this patch change that?

I tracked down this specific thing, yes (but stuff kept failing). What I was trying to say is that it probably makes no sense to support that max-addressable-memory edge-case in NIO (generally). If you really want to, I suggest starting out by writing tests that check against Int64.max on 64-bit too. It is not worth it, IMO.

@helje5 I don't understand why Int32.max doesn't work here. It fits into an Int on both 32 and 64 bit platforms.

// Note(hh): This is not a _proper_ fix, but necessary because // other places extend on that. Should be fine in // practice on 32-bit platforms.

Well, it is in the comment above it (and in my reply above yours). It is not a proper fix, but it is a lot of work to track down all edge cases (for little gain).

P.S.: Maybe a lot of that went away when you changed sysMalloc(Int) to sysMalloc(size_t).

@helje5 could we re-check this?

@helje5 if you could just re-check that this is still the case. And then just remove the two note(hh) comments with a good explanation why we need (if we need) special casing.

if that's done we can merge I think.

Lukasa · 2018-06-12T15:28:11Z

Sources/NIOWebSocket/WebSocketFrameEncoder.swift

+    // Note(hh): This is not a _proper_ fix, but necessary because
+    //           other places extend on that. Should be fine in
+    //           practice on 32-bit platforms.
+    private let maxNIOFrameSize = Int(Int32.max / 4)


Any reason not to just set this to Int(Int32.max )? So far as I know we don't ever do math on this, so setting it to Int32.max gets the behaviour as close as reasonably possible to the 64-bit behaviour.

Same as above. Probably breaks when it travels to the ByteBuffer, which will use UInt32 and then fail.

we should fully understand this. I think Int32.max should just work

Lukasa · 2018-06-12T15:29:32Z

Tests/NIOTests/ByteBufferTest.swift

@@ -1073,22 +1073,37 @@ class ByteBufferTest: XCTestCase {
    }

    func testAllocationOfReallyBigByteBuffer() throws {
+        #if !arch(arm) // !32-bit, Raspi/AppleWatch/etc


Can we invert this conditional? It'd be nice to see the intended effect of the other branch closer to the top of the test.

Lukasa · 2018-06-12T15:30:08Z

Tests/NIOTests/ByteBufferTest.swift

@@ -1073,22 +1073,37 @@ class ByteBufferTest: XCTestCase {
    }

    func testAllocationOfReallyBigByteBuffer() throws {
+        #if !arch(arm) // !32-bit, Raspi/AppleWatch/etc


I assume this conditional is intended to be temporary? If it's not, can we remove the tests for arch(arm) elsewhere in this test function, as they're unreachable.

Yes. This fails in a specific ARM memcpy, I hope that we get a fix for that eventually. I raised this in the ARM-community slack. The checks within make the test working until that memcpy error. (maybe that memcpy has additional size or alignment restrictions, no idea yet).

Lukasa · 2018-06-12T15:30:25Z

Tests/NIOTests/ChannelTests.swift

@@ -169,9 +169,15 @@ public class ChannelTests: XCTestCase {
        for _ in 0..<bufferSize {
            buffer.write(staticString: "a")
        }
+


Mind removing this whitespace line?

Lukasa · 2018-06-12T15:31:48Z

Tests/NIOTests/ChannelTests.swift

-            XCTAssertEqual(0xdeadbeef, Int(bitPattern: iovecs.last!.iov_base))
-            XCTAssertEqual(0xdeadbeef, iovecs.last!.iov_len)
+            XCTAssertEqual(0x1337beef, Int(bitPattern: iovecs.last!.iov_base))
+            XCTAssertEqual(0x1337beef, iovecs.last!.iov_len)


Out of interest why does the old value prevent compilation?

Int(0xdeadbeef) (which the compiler generates for literals) overflows Int32 (which then asserts). The other option which I think I actually used in the original patch is 0xdeadbeef as UInt or sth like that. I don't mind either.

Might be better to use that other version, just for consistency with ourselves (we use deadbeef as our marker value elsewhere in the code)

I'd propose 0xdedbeef or 0xdadbeef or smth but only if we can't use 0xdeadbeef (which fits fine in a UInt32)

helje5 · 2018-06-12T16:06:28Z

Just noticed that this PR is appropriately numbered: #486

@Lukasa

As requested by @Lukasa in PR 486.

@Lukasa

in testAllocationOfReallyBigByteBuffer() ... as requested by @Lukasa in PR 486.

@Lukasa

... as requested by @Lukasa in PR 486. Not possible in all places, some expect an Int in the API (withPendingDatagramWritesManager).

helje5 · 2018-06-13T10:31:56Z

OK, I incorporated the easy changes requested, but I don't have the time to harden NIO for the max-mem edge case. So your choice whether you want no tests or all tests with no memory-edge-case like on 64-bit. Or maybe someone else steps in.

helje5 · 2018-06-13T11:33:48Z

Do not merge yet, I may have to revert another deadbeef to 1337beef, tests coredump again.

Was crashing the tests. Back to 0x1337beef which works everywhere.

helje5 · 2018-06-13T11:47:54Z

OK, done. Runs through again.

Lukasa

Mind changing the remaining instances of 1337beef to deadbeef as UInt? There are a few more. 😄

helje5 · 2018-06-13T19:37:09Z

@Lukasa I originally changed all, but some didn't work. See the commit log. E.g.:

Revert one 0xdeadbeef as UInt back into 1337beef

(some API is expecting Int, not UInt on 32-bit)

weissi · 2018-06-18T15:15:19Z

@helje5 I find it slightly amusing that your patch which makes the 32 bits tests run is #486 :D

helje5 · 2018-06-18T16:13:05Z

You are late, scroll upwards:

Just noticed that this PR is appropriately numbered: #486

My original PR #383 also just missed the proper primary-key, I need to be more prudent and consider the details before submitting PRs. Sorry about that!

Lukasa · 2018-06-19T09:49:38Z

Ok, I think I'm happy. @weissi want to review?

normanmaurer · 2018-06-19T09:50:55Z

Sources/NIO/ByteBuffer-core.swift

@@ -332,7 +332,7 @@ public struct ByteBuffer {
                self._slice = _ByteBufferSlice(_slice.lowerBound ..< self._storage.capacity)
            }
        }
-        assert(self._slice.lowerBound + index + capacity <= self._slice.upperBound)
+        assert(self._slice.lowerBound + index + capacity <= self._slice.upperBound) // TODO(hh): fails on 32-bit


Remove the TODO ?

@helje5 why does this fail on 32-bit ? If that fails, the code above doesn't work

Probably was a temporary note for myself and I fixed it on the calling side, need to check.

normanmaurer · 2018-06-19T09:51:04Z

Sources/NIO/ByteBuffer-core.swift

@@ -231,7 +231,7 @@ public struct ByteBuffer {
        }

        private static func allocateAndPrepareRawMemory(bytes: Capacity, allocator: Allocator) -> UnsafeMutableRawPointer {
-            let bytes = Int(bytes)
+            let bytes = Int(bytes) /* Note(hh): this can fail on 32-bit (Int32 vs UInt32 Capacity)) */


remove the Note ?

I'm also not sure why this is an Int. malloc takes size_t which is a UInt. Maybe our allocator interface needs to change? Hope it's not public

ping @helje5

Just checked: size_t is unsigned int on the 32-bit RasPi Ubuntu (I kinda assumed that it might be signed int because quite often you can't actually alloc more than 2GB on 32-bit platforms).

It probably did fail when one of the max-out tests passed in 0xDEADBEEF, which will fail the Int(bytes) conversion.

Why you are using Int I don't know, I didn't introduce this. But your code says:

let sysMalloc: @convention(c) (Int) -> UnsafeMutableRawPointer? = malloc

I don't know, maybe you should just use size_t?

Moved this one to: #499.

weissi · 2018-06-19T09:52:43Z

Tests/NIOTests/ByteBufferTest.swift

-
-        XCTAssertEqual(AllocationExpectationState.reallocDone, testAllocationOfReallyBigByteBuffer_state)
-        XCTAssertEqual(buf.capacity, Int(UInt32.max))
+        #if arch(arm) // 32-bit, Raspi/AppleWatch/etc


instead of indenting everything, can we just do

#if arch(arm) // this test fails on 32 bit platforms return #endif

I don't care if you prefer that, but visually an indent makes sense, IMO. Sometimes people do a 4-indent for regular Swift code and indent the # stuff by 2, which I also kinda like.

Oh, you are saying I should return early, not remove the actual indent. But this will produce a Swift compiler warning wrt unreachable code.

ahh, ok, then leave this as is.

weissi · 2018-06-19T09:53:08Z

Tests/NIOTests/ChannelTests.swift

+        #if arch(arm) // 32-bit, Raspi/AppleWatch/etc
+            let lotsOfData = Int(Int32.max / 8)
+        #else
+            let lotsOfData = Int(INT32_MAX) // TBD(hh): should that be Int32.max?


please use Int32.max

weissi · 2018-06-19T09:55:31Z

Tests/NIOTests/ChannelTests.swift


+        #if arch(arm) // 32-bit, Raspi/AppleWatch/etc
+            let lotsOfData = Int(Int32.max / 8)


why do we use less data here? Is that documented to be the maximum?

Why do you use Int32.max instead of Int64.max on 64-bit? Same reason ;-) (allocating 4GB on 64-bit is usually OK, but a Raspi has only 512MB or maybe 1GB of RAM - and no swap by default either. so lets not overdo this test).

but we're only allocating 2MB here, right? We'll write the same buffer over and over again, this should work. The sizes were chosen so it will exhaust both the number of bytes limit (IIRC 2GB) as well as the number of chunks (1024) you're allowed to give to writev. If we think this isn't worthwhile, then we can lower the number for all platforms. CC @Lukasa

@weissi is correct, the purpose of this is to validate that we correctly handle having more data to pass to writev than writev will accept. As @weissi notes, we only actually allocate one buffer, so there is no risk.

Oh, I see. You just repeatedly write the 2MB buffer. Maybe the overflow happens because of <=? I guess I need to check this one on a Raspi again.
(I mean, internally it still has to spool up Int32.max of buffers, which won't work out well, right?)

it'd be awesome if you could validate why it fails. There must be something else wrong.

Well, see my () comment:

internally it still has to spool up Int32.max of buffers, which won't work out

Motivation: Get the PR merged. Modifications: - drop two `// Note(hh)` comments - use Int32.max Result: Beauty. Pure beauty.

helje5 · 2018-07-03T13:29:08Z

Not sure why Merging is blocked now. I updated the master and pulled it in ...

helje5 · 2018-07-09T14:23:03Z

I think this should be good now? I don't know how/why GitHub is confused.

weissi · 2018-07-10T13:32:45Z

Sources/NIO/ByteBuffer-int.swift

    public func nextPowerOf2ClampedToMax() -> UInt32 {
        guard self > 0 else {
            return 1
        }

        var n = self

+        #if arch(arm) // 32-bit, Raspi/AppleWatch/etc
+            let max = UInt32(Int32.max)


this is maybe nit-picking but we'd like it to be Int.max (yes, same as Int32.max on 32-bit platforms) because really we'd be okay with it being larger but Swift uses the Int type so maybe it's clearer to users? I'm okay with leaving that too though. @Lukasa

Fine with changing this to Int.max

I'm not entirely sure about this. You want UInt32(Int.max)? That would fail on 64-bit platforms? This really only is for the UInt32(Int32.max) case, and it makes sense to be explicit here, no?

yes UInt32(Int.max) would be good. I want it to fail if it's compiled on 64-bits. This thing should only be there for 32-bit platforms

weissi · 2018-07-10T13:33:36Z

Sources/NIO/ByteBuffer-int.swift

@@ -105,21 +105,27 @@ extension FixedWidthInteger {
 }

 extension UInt32 {
-    /// Returns the next power of two unless that would overflow in which case UInt32.max is returned.
+    /// Returns the next power of two unless that would overflow, in which case UInt32.max (on 64-bit systems) or Int32.max (on 32-bit systems) is returned. The returned value is always safe to be cast to Int and passed to malloc on all platforms.


mind breaking that line in two so it's doesn't go over 120 characters ideally?

weissi · 2018-07-10T13:43:47Z

Sources/NIO/ByteBuffer-int.swift

        n -= 1
        n |= n >> 1
        n |= n >> 2
        n |= n >> 4
        n |= n >> 8
        n |= n >> 16
-        if n != .max {
+        if n != max {


do we actually have a test that on 32-bit systems tests that this returns the right value?

There is no test for this on 64-bit either. /shrug

that's bad, we should definitely have one

@weissi

... as requested by @weissi. 78 or _maybe_ 80 would be right though. Hope this compiles nevertheless.

weissi · 2018-08-07T14:16:17Z

@helje5 let's get this merged. If you address the remaining comments then we can get this through really quickly

weissi · 2018-08-07T14:16:29Z

@swift-nio-bot test this please

helje5 · 2018-08-08T14:37:19Z

@weissi I'm a little busy with other stuff. What exactly is still missing?

weissi · 2018-08-16T16:36:40Z

@swift-nio-bot test this please

* 32-bit fix for tests: Replace 0xDEADBEEF w/ 0x1337BEEF Motivation: Make tests compile on Raspberry Pi. Modifications: - Change tests so that they compile on 32-bit machines, like Raspi. Replaced 0xDEADBEEF which doesn't fit into an Int32, w/ 0x1337BEEF. - make ByteBuffer work better on 32-bit - fix other wrong assertions and limits that are too big on 32bit Also use TimeAmount.Value in one test. Result: Tests will compile on Raspi ... Motivation: Explain here the context, and why you're making that change. What is the problem you're trying to solve. Modifications: Describe the modifications you've done. Result: After your change, what will change.

helje5 added 6 commits June 12, 2018 12:21

Fix compile issues introduced in 32-bit port

d4c40b0

Motivation: Make it compile on 64-bit Intel again. Modifications: Just some type casting. Result: Builds on 64-bit again.

Lukasa requested changes Jun 12, 2018

View reviewed changes

helje5 added 3 commits June 13, 2018 12:11

Fix comment of nextPowerOf2ClampedToMax

6641f8a

As requested by @Lukasa in PR 486.

Invert a #if-else arch conditional

6ab4f11

in testAllocationOfReallyBigByteBuffer() ... as requested by @Lukasa in PR 486.

Use 0xdeadbeef as UInt instead of 0x1337beef

abd6238

... as requested by @Lukasa in PR 486. Not possible in all places, some expect an Int in the API (withPendingDatagramWritesManager).

Revert one 0xdeadbeef as UInt back into 1337beef

d49bee0

Was crashing the tests. Back to 0x1337beef which works everywhere.

Lukasa reviewed Jun 13, 2018

View reviewed changes

Lukasa approved these changes Jun 19, 2018

View reviewed changes

normanmaurer requested changes Jun 19, 2018

View reviewed changes

weissi reviewed Jun 19, 2018

View reviewed changes

helje5 mentioned this pull request Jul 3, 2018

ByteBuffer uses UInt for Capacity, but Int internally #499

Closed

helje5 added 2 commits July 3, 2018 15:25

Address PR486 reviews items

b2fe95b

Motivation: Get the PR merged. Modifications: - drop two `// Note(hh)` comments - use Int32.max Result: Beauty. Pure beauty.

Merge branch 'master' into feature/raspi-nio-tests-1

3246661

Merge branch 'master' into feature/raspi-nio-tests-1

1ee4edd

weissi reviewed Jul 10, 2018

View reviewed changes

helje5 and others added 3 commits July 10, 2018 17:20

Merge branch 'master' into feature/raspi-nio-tests-1

7175fc8

Just wraps a comment to 120 columns

2eb3e6a

... as requested by @weissi. 78 or _maybe_ 80 would be right though. Hope this compiles nevertheless.

Merge branch 'master' into feature/raspi-nio-tests-1

fb1a6e1

Merge branch 'master' into feature/raspi-nio-tests-1

19a4199

weissi added 2 commits August 14, 2018 15:03

Merge branch 'master' into feature/raspi-nio-tests-1

8e8870d

Merge branch 'master' into feature/raspi-nio-tests-1

91c38d3

weissi merged commit e8c04e0 into apple:master Aug 16, 2018


		#if arch(arm) // 32-bit, Raspi/AppleWatch/etc
		let lotsOfData = Int(Int32.max / 8)

Make tests work on 32-bit #486

Make tests work on 32-bit #486

Conversation

helje5 commented Jun 12, 2018

Motivation:

Modifications:

Result:

swift-nio-bot commented Jun 12, 2018

swift-nio-bot commented Jun 12, 2018

Lukasa left a comment • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissi Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

helje5 commented Jun 12, 2018 • edited Loading

helje5 commented Jun 13, 2018

helje5 commented Jun 13, 2018

helje5 commented Jun 13, 2018

Lukasa left a comment

Choose a reason for hiding this comment

helje5 commented Jun 13, 2018

weissi commented Jun 18, 2018

helje5 commented Jun 18, 2018

Lukasa commented Jun 19, 2018

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

weissi Jun 19, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helje5 Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helje5 Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helje5 commented Jul 3, 2018

helje5 commented Jul 9, 2018

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

Choose a reason for hiding this comment

weissi commented Aug 7, 2018

weissi commented Aug 7, 2018

Lukasa left a comment •

edited

Loading

weissi Jun 19, 2018 •

edited

Loading

helje5 commented Jun 12, 2018 •

edited

Loading

weissi Jun 19, 2018 •

edited

Loading

helje5 Jul 3, 2018 •

edited

Loading

helje5 Jul 10, 2018 •

edited

Loading