Bug 25798 - LessThanOrEqual wrong result for nuint type on 32bit platform
Summary: LessThanOrEqual wrong result for nuint type on 32bit platform
Status: VERIFIED FIXED
Alias: None
Product: iOS
Classification: Xamarin
Component: XI runtime (show other bugs)
Version: XI 8.6.0
Hardware: Macintosh Mac OS
: --- normal
Target Milestone: 8.6.0
Assignee: Zoltan Varga
URL:
Depends on:
Blocks:
 
Reported: 2015-01-07 14:29 UTC by Rustam Zaitov
Modified: 2015-01-08 15:51 UTC (History)
4 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:


Attachments

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

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.


Create a new report for Bug 25798 on Developer Community or GitHub if you have new information to add and do not yet see a matching report.

  • Export the original title and description: Developer Community HTML or GitHub Markdown
  • Copy the title and description into the new report. Adjust them to be up-to-date if needed.
  • Add your new information.

In special cases on GitHub you might also want the comments: GitHub Markdown with public comments


Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.

Related Links: