Do not emit implict_op if from/to are same type #862

Merged
merged 1 commit into from Apr 10, 2017

Conversation

Projects
None yet
4 participants
@kingces95
Member

kingces95 commented Apr 7, 2017

Description of Change

After bisection I found that change eacbc1acf4cc in GetImplicitOperatorTo causes the reproduction of https://bugzilla.xamarin.com/show_bug.cgi?id=54012 to throw an AccessViolationException in

var run = new Run { Text = span.Text };
. Note the bug reports a different issue; this is a regression that blocks investigation of the reported issue.

Expected MasterDetailNavigation.dll for 54012 to successfully pass peverify but actually observed errors. Expected xaml compiler to emit initlocals for all method bodies but actually did not. Expected xaml compiler to not emit op_implicit if fromType equals toType but actually emits op_implicit when that is the case. Either actual behavior produces unverifiable code.

Changed xaml compiler to emit initlocals for all MethodDefinitions. Changed xaml compiler to not emit op_implicit when from type equals to type.

Bugs Fixed

None. Discovered regression while investigating another bug.

API Changes

None.

Behavioral Changes

None.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

@kingces95 kingces95 requested review from jassmith and StephaneDelcroix Apr 7, 2017

@dnfclas dnfclas added the cla-required label Apr 7, 2017

@kingces95

This comment has been minimized.

Show comment
Hide comment
@kingces95

kingces95 Apr 7, 2017

Member

Here is the bad IL: The op_Implicit call takes a string but is passed V_10 which is a FormattedString.

IL_03bc:  ldsfld     class [Xamarin.Forms.Core]Xamarin.Forms.BindableProperty [Xamarin.Forms.Core]Xamarin.Forms.Label::FormattedTextProperty
IL_03c1:  ldloc.s    V_10
IL_03c3:  call       class [Xamarin.Forms.Core]Xamarin.Forms.FormattedString [Xamarin.Forms.Core]Xamarin.Forms.FormattedString::op_Implicit(string)
IL_03c8:  callvirt   instance void [Xamarin.Forms.Core]Xamarin.Forms.BindableObject::SetValue(class [Xamarin.Forms.Core]Xamarin.Forms.BindableProperty,
Member

kingces95 commented Apr 7, 2017

Here is the bad IL: The op_Implicit call takes a string but is passed V_10 which is a FormattedString.

IL_03bc:  ldsfld     class [Xamarin.Forms.Core]Xamarin.Forms.BindableProperty [Xamarin.Forms.Core]Xamarin.Forms.Label::FormattedTextProperty
IL_03c1:  ldloc.s    V_10
IL_03c3:  call       class [Xamarin.Forms.Core]Xamarin.Forms.FormattedString [Xamarin.Forms.Core]Xamarin.Forms.FormattedString::op_Implicit(string)
IL_03c8:  callvirt   instance void [Xamarin.Forms.Core]Xamarin.Forms.BindableObject::SetValue(class [Xamarin.Forms.Core]Xamarin.Forms.BindableProperty,
@StephaneDelcroix

👍 merge when it's green, and cherry-pick to 2.3.4-2

@StephaneDelcroix StephaneDelcroix merged commit 0f57733 into master Apr 10, 2017

5 of 6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests faile…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Debug : Tests passed: 3764, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests p…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests pa…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests pa…
Details

StephaneDelcroix added a commit that referenced this pull request Apr 10, 2017

@rmarinho rmarinho deleted the xamlcimpop branch Jun 22, 2017

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment