Bug 20562 - Seeming valid IL produces invalid result
Summary: Seeming valid IL produces invalid result
Status: RESOLVED FIXED
Alias: None
Product: Runtime
Classification: Mono
Component: JIT (show other bugs)
Version: 3.4.0
Hardware: Macintosh Mac OS
: --- normal
Target Milestone: ---
Assignee: Rodrigo Kumpera
URL:
Depends on:
Blocks:
 
Reported: 2014-06-12 16:52 UTC by vargolsoft
Modified: 2017-10-20 09:21 UTC (History)
6 users (show)

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


Attachments
The disassembly that gives the wrong values (3.68 MB, application/octet-stream)
2014-06-12 16:52 UTC, vargolsoft
Details
The disassembly of the 'fixed' code (3.68 MB, application/octet-stream)
2014-06-12 16:53 UTC, vargolsoft
Details
Program and Test Input (149.99 KB, application/zip)
2014-06-12 16:56 UTC, vargolsoft
Details
This is the real buggy version, with debug WriteConsole statements (243.94 KB, application/zip)
2014-06-12 17:06 UTC, vargolsoft
Details


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.

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.


Please create a new report on GitHub or Developer Community with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description vargolsoft 2014-06-12 16:52:14 UTC
Created attachment 7057 [details]
The disassembly that gives the wrong values

The C# code looks like something like this, its actually the top of a larger
function that has been called recursively


The variables start and end are the functions parameters and have the values 1 and 3;

private void balanceSegment(Photon[] temp, int index, int start, int end)
{
            Console.WriteLine(String.Format("index {0}, start {1}, end {2}", index, start, end));
           
int median = 1;

while ((4 * median) <= (end - start + 1))
    median += median;

Console.WriteLine(String.Format("4 median {0}", median));

if ((3 * median) <= (end - start + 1))
{
    median += median;
    median += (start - 1);
    Console.WriteLine(String.Format("3 median {0}", median));
}
else
    median = end - median + 1;

This prints...
index 1, start 1, end 3
4 median 1
3 median 1 


It looks like the  median += median for the if statement gets ignored.
If I change that for median *= 2 it prints the correct values...

private void balanceSegment(Photon[] temp, int index, int start, int end)
{
            Console.WriteLine(String.Format("index {0}, start {1}, end {2}", index, start, end));
    int median = 1;

while ((4 * median) <= (end - start + 1))
    median += median;

Console.WriteLine(String.Format("4 median {0}", median));

if ((3 * median) <= (end - start + 1))
{
    median *= 2;
    median += (start - 1);
    Console.WriteLine(String.Format("3 median {0}", median));
}
else
    median = end - median + 1;

index 1, start 1, end 3
4 median 1
3 median 2 

Oh, just for completeness, if I change the line median += (start - 1) to median += (start - 2) I get

index 1, start 1, end 3
4 median 1
3 median 0 
 
which means it is just ignoring the median += median line. 


The IL looks valid, although I'm not an expert, running monodis on both the first two versions above shows the
following when diffed using -c 

*** 33324,33331 ****
      IL_0022:  bgt IL_0036
      IL_0027:  ldloc.0
!     IL_0028:  ldc.i4.2
!     IL_0029:  mul
      IL_002a:  stloc.0
      IL_002b:  ldloc.0
      IL_002c:  ldarg.3
--- 33324,33331 ----
      IL_0022:  bgt IL_0036
      IL_0027:  ldloc.0
!     IL_0028:  ldloc.0
!     IL_0029:  add
      IL_002a:  stloc.0
      IL_002b:  ldloc.0
      IL_002c:  ldarg.3 

The bottom code looks like its doing median+=median so I'm assuming this is an issue with the run time or JIT compiler.

Now the bad news. If I isolate the code the bug disappears and I get 3 median 2 so I don't have a simple test case for you
Comment 1 vargolsoft 2014-06-12 16:53:25 UTC
Created attachment 7058 [details]
The disassembly of the 'fixed' code
Comment 2 vargolsoft 2014-06-12 16:56:50 UTC
Created attachment 7059 [details]
Program and Test Input

mono SunflowSharp.Test.exe particle_chau.sc
Comment 3 vargolsoft 2014-06-12 17:06:31 UTC
Created attachment 7060 [details]
This is the real buggy version, with debug WriteConsole statements

This is the real buggy version, with debug WriteConsole statements, compiled using 3.4.0
Comment 4 vargolsoft 2014-06-12 17:11:31 UTC
Additional info

I'm running on OSX 10.6.8

uname gives the version numbers as...
10.8.0 Darwin Kernel Version 10.8.0: Tue Jun  7 16:33:36 PDT 2011; root:xnu-1504.15.3~1/RELEASE_I386 i386


I've tried running the bad version using mono --llvm and it seems to work correctly.
A fresh compile against .net also works correctly.

I've created a branch on my github repository holding the broken code and an example file.

The repository is

https://github.com/Vargol/PhotonPump.git

The branch is mono_bug

to run the example

cd to the PhotonPump/SunflowSharp.Test/bin/Release directory and run
mono SunflowSharp.Test.exe ../../../examples/particle_chau.sc

It should eventually fail giving

...
LIGHT  info: Balancing caustics photon map ...
index 1, start 1, end 3
4 median 1
3 median 1
index 3, start 2, end 3
4 median 1
System.IndexOutOfRangeException: Array index is out of range.
...
followed by a stack crawl
Comment 5 vargolsoft 2014-06-13 13:37:03 UTC
A bit more information.


Running mono with the --llvm option gives the correct result, as does compiling and running against .net.

Also note that Ed from the mono-list mailing list has replicated my results and notes that if you attach the soft debugger you also get the correct result.
 
http://lists.ximian.com/pipermail/mono-list/2014-June/050970.html
Comment 6 Zoltan Varga 2014-06-17 16:03:55 UTC
What is the output of mono --version ?
Comment 7 Zoltan Varga 2014-06-17 16:32:04 UTC
Testcase:
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
using System;

class Tests
{
	static void balanceSegment(int start, int end) {
		int median = 1;

		if ((3 * median) <= (end - start + 1)) {
			median += median;
			median += (start - 1);
			Console.WriteLine (median);
		}
	}

	public static void Main (String[] args) {
		balanceSegment (1, 3);
	}
}
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
Running it normally outputs 2, running it with -O=-linears outputs 1.

This happens because the JIT transforms the median += median assignment into:

 int_add R13 <- R13 R13 clobbers: 1
 move R92 <- R13

then:

 loadi4_membase R575 <- [%ebp + 0xffffffd0]
 x86_add_membase_reg [%ebp + 0xffffffd0] R575
 move R92 <- R575

so its using the previous value of median, not the new one.
Comment 8 vargolsoft 2014-06-17 16:58:50 UTC
Macintosh:Release vargol$ mono --version
Mono JIT compiler version 3.4.0 ((no/d4511ef Tue Mar 25 14:35:52 EDT 2014)
Copyright (C) 2002-2014 Novell, Inc, Xamarin Inc and Contributors. www.mono-project.com
	TLS:           normal
	SIGSEGV:       altstack
	Notification:  kqueue
	Architecture:  x86
	Disabled:      none
	Misc:          softdebug 
	LLVM:          yes(3.4svn-mono-(no/e656cac)
	GC:            sgen
Comment 9 vargolsoft 2014-06-17 17:40:15 UTC
Just tried it with a fairly recent clone from github.

david@debian-processing:~/PhotonPump/SunflowSharp.Test/bin/Debug$ mono --version
Mono JIT compiler version 3.6.1 (master/59fd337 Fri Jun 13 20:51:14 BST 2014)
Copyright (C) 2002-2014 Novell, Inc, Xamarin Inc and Contributors. www.mono-project.com
	TLS:           __thread
	SIGSEGV:       altstack
	Notifications: epoll
	Architecture:  x86
	Disabled:      none
	Misc:          softdebug 
	LLVM:          supported, not enabled.
	GC:            sgen

LIGHT  info: Balancing caustics photon map ...
index 1, start 1, end 3
4 median 1
3 median 1

So, its there too and in 32 bit Linux.
Comment 10 Rodrigo Kumpera 2017-10-11 00:06:58 UTC
PR against master https://github.com/mono/mono/pull/5747