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)

See Also:
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

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

Note You need to log in before you can comment on or make changes to this bug.