Bug 25798

Summary: LessThanOrEqual wrong result for nuint type on 32bit platform
Product: iOS Reporter: Rustam Zaitov <rustam.zaitov>
Component: XI runtimeAssignee: Zoltan Varga <vargaz>
Status: VERIFIED FIXED    
Severity: normal CC: gouri.kumari, mono-bugs+monotouch, oleg.demchenko, sebastien
Priority: ---    
Version: XI 8.6.0   
Target Milestone: 8.6.0   
Hardware: Macintosh   
OS: Mac OS   
Tags: Is this bug a regression?: ---
Last known good build:

Description Rustam Zaitov 2015-01-07 14:29:51 UTC
Self contained example here https://github.com/rzaitov/selfContained/tree/master/LessOrEqual
I got the following results:

min < max | 10000000 < 40000000 == True
min <= max | 10000000 <= 40000000 == False <-- wrong
min > max | 10000000 > 40000000 == False
min >= max | 10000000 >= 40000000 == False

where min and max defined as follow:
readonly nuint min = 10000000;
readonly nuint max = 40000000;

If I change type from nuint to nint, I will get the correct results.
I face up with issue only on 32bit device (iPad mini 1 gen). iPhone6 works for me.
Comment 1 Rustam Zaitov 2015-01-07 14:44:55 UTC
Xamarin.iOS
Version: 8.6.0.48 (Enterprise Edition) 

Works for iPhone6 (8.1.2)
Wrong results for iPadmini 1 gen (8.1.2)
Comment 2 Sebastien Pouliot 2015-01-07 14:48:25 UTC
Seems to work on master using the simulator (JIT) 32bits

2015-01-07 14:46:41.326 dontlink[29262:3670521] [MonoTouch Version:	8.9.0]
2015-01-07 14:46:41.327 dontlink[29262:3670521] [Assembly:	Xamarin.iOS.dll (32 bits)]
2015-01-07 14:46:41.327 dontlink[29262:3670521] [GC:	sgen+NewRefCount]
2015-01-07 14:46:41.328 dontlink[29262:3670521] [iPhone Simulator:	iPhone OS v8.1]
2015-01-07 14:46:41.328 dontlink[29262:3670521] [Device Name:	iPhone Simulator]

2015-01-07 14:46:54.532 dontlink[29262:3670521] min < max | 10000000 < 40000000 == True
2015-01-07 14:46:55.107 dontlink[29262:3670521] min <= max | 10000000 <= 40000000 == True
2015-01-07 14:46:55.599 dontlink[29262:3670521] min > max | 10000000 > 40000000 == False
2015-01-07 14:46:56.100 dontlink[29262:3670521] min >= max | 10000000 >= 40000000 == False

will switch to device...
Comment 3 Sebastien Pouliot 2015-01-07 15:17:49 UTC
So turning this into a unit tests 

		[Test]
		public void Bug25798 ()
		{
			Assert.True (min < max, "1");
			Assert.True (min <= max, "2");
			Assert.False (min > max, "3");
			Assert.False (min >= max, "4");
		}

gave me

2015-01-07 15:14:47.025 dontlink[2001:707] 	[FAIL] Bug25798 :   2
  Expected: True
  But was:  False

on a 32bits device. Even if the local (immediate) evaluation is correct, 

> ? min <= max
true

I suspect there's something wrong in the 32bits ARM support for native types -> Zoltan
Comment 4 Sebastien Pouliot 2015-01-07 15:21:44 UTC
The code generated from the template maccore/src/NativeTypes/Primitives.tt looks ok (which makes sense as it works on other architectures).

public static bool operator <= (nuint l, nuint r) {
	return l.v <= r.v;
}
Comment 5 Sebastien Pouliot 2015-01-07 16:52:34 UTC
Having OP_ICGE_UN identical to OP_ICLE_UN seems wrong to me - but, if that's the bug, then it means it's an opcode that's never used normally (as `uint` comparison works correctly).

Anyway reversing OP_ICGT_UN seems to works fine (and seems logical to me :-)

diff --git a/mono/mini/mini-arm.c b/mono/mini/mini-arm.c
index 7e7c593..af05f2d 100644
--- a/mono/mini/mini-arm.c
+++ b/mono/mini/mini-arm.c
@@ -5205,10 +5205,13 @@ mono_arch_output_basic_block (MonoCompile *cfg, MonoBasicBlock *bb)
                        ARM_MOV_REG_IMM8_COND (code, ins->dreg, 0, ARMCOND_GT);
                        break;
                case OP_ICGE_UN:
-               case OP_ICLE_UN:
                        ARM_MOV_REG_IMM8 (code, ins->dreg, 1);
                        ARM_MOV_REG_IMM8_COND (code, ins->dreg, 0, ARMCOND_LO);
                        break;
+               case OP_ICLE_UN:
+                       ARM_MOV_REG_IMM8 (code, ins->dreg, 0);
+                       ARM_MOV_REG_IMM8_COND (code, ins->dreg, 1, ARMCOND_LO);
+                       break;
Comment 6 Sebastien Pouliot 2015-01-07 16:53:40 UTC
wrong diff :-|

diff --git a/mono/mini/mini-arm.c b/mono/mini/mini-arm.c
index 7e7c593..dedf54b 100644
--- a/mono/mini/mini-arm.c
+++ b/mono/mini/mini-arm.c
@@ -5205,10 +5205,13 @@ mono_arch_output_basic_block (MonoCompile *cfg, MonoBasicBlock *bb)
                        ARM_MOV_REG_IMM8_COND (code, ins->dreg, 0, ARMCOND_GT);
                        break;
                case OP_ICGE_UN:
-               case OP_ICLE_UN:
                        ARM_MOV_REG_IMM8 (code, ins->dreg, 1);
                        ARM_MOV_REG_IMM8_COND (code, ins->dreg, 0, ARMCOND_LO);
                        break;
+               case OP_ICLE_UN:
+                       ARM_MOV_REG_IMM8 (code, ins->dreg, 1);
+                       ARM_MOV_REG_IMM8_COND (code, ins->dreg, 0, ARMCOND_HI);
+                       break;
                case OP_COND_EXC_EQ:
                case OP_COND_EXC_NE_UN:
                case OP_COND_EXC_LT:
Comment 7 Zoltan Varga 2015-01-07 18:29:38 UTC
Applied it in mono master 15e9a6180722274aad52347635dec5470698c8f7.
Comment 8 Sebastien Pouliot 2015-01-08 13:06:46 UTC
Backported to mono/monotouch-8.6.0-branch, milestone adjusted.

Fixed (bump) in monotouch/monotouch-8.6.8-branch dfb682fa615491b012ed24198d2bdcd641e1874f

QA: unit tests added (monotouch-test.app) in the same revision
Comment 9 GouriKumari 2015-01-08 15:16:19 UTC
Verified with  Xamarin.iOS 8.6.0.51.pkg and sample app.

Expected Output:
2015-01-07 14:46:54.532 dontlink[29262:3670521] min < max | 10000000 < 40000000
== True
2015-01-07 14:46:55.107 dontlink[29262:3670521] min <= max | 10000000 <=
40000000 == True
2015-01-07 14:46:55.599 dontlink[29262:3670521] min > max | 10000000 > 40000000
== False
2015-01-07 14:46:56.100 dontlink[29262:3670521] min >= max | 10000000 >=
40000000 == False

Actual Output: 
2015-01-08 15:07:08.438 LessOrEqual[533:60b] min < max | 10000000 < 40000000 == True
2015-01-08 15:07:08.441 LessOrEqual[533:60b] min <= max | 10000000 <= 40000000 == True
2015-01-08 15:07:08.443 LessOrEqual[533:60b] min > max | 10000000 > 40000000 == False
2015-01-08 15:07:08.444 LessOrEqual[533:60b] min >= max | 10000000 >= 40000000 == False

Marking this bug as verified fixed.

Application Output: https://gist.github.com/GouriKumari/8442b5f29ddf52a02e86
Build Log: https://gist.github.com/GouriKumari/9d319005c8317d2eec59
Test Env Log: 
Xamarin Studio
Version 5.7 (build 661)
Installation UUID: 473de7c6-b183-4ba3-899e-3c03c5650a88
Runtime:
	Mono 3.12.0 ((detached/a813491)
	GTK+ 2.24.23 (Raleigh theme)
 
	Package version: 312000068
 
Apple Developer Tools
Xcode 6.1.1 (6611)
Build 6A2008a
 
Xamarin.iOS
Version: 8.6.0.51 (Enterprise Edition)
Hash: dfb682f
Branch: 
Build date: 2015-01-08 13:39:32-0500
Comment 10 GouriKumari 2015-01-08 15:34:56 UTC
Updates to above comment:

Verified with both Armv7 and Armv7+Arm64 architecture

Test Results with ARMv7: https://gist.github.com/GouriKumari/44b033bba1f12da715d8
Test Results with ARMv7 +ARM64: https://gist.github.com/GouriKumari/8442b5f29ddf52a02e86
Device  info: iPhone5S
Comment 11 Rustam Zaitov 2015-01-08 15:51:22 UTC
verified with Armv7 on iPad mini 1 gen (32bit) – works well
result – https://gist.github.com/rzaitov/19d66c0a7840363c3dfb
8.6.0.51