This is Xamarin's bug tracking system. For product support, please use the support links listed in your Xamarin Account.
Bug 48378 - For builds on Mac, XamlC or Cecil incorrectly updates .mdb files for iOS apps, maybe only when the Xamarin.Build.Download package is also used
Summary: For builds on Mac, XamlC or Cecil incorrectly updates .mdb files for iOS apps...
Status: RESOLVED FIXED
Alias: None
Product: Forms
Classification: Xamarin
Component: Forms (show other bugs)
Version: 2.3.3
Hardware: PC Mac OS
: --- major
Target Milestone: ---
Assignee: Bugzilla
URL:
Depends on:
Blocks:
 
Reported: 2016-11-30 05:32 UTC by Brendan Zagaeski
Modified: 2017-02-02 10:22 UTC (History)
4 users (show)

See Also:
Tags: AC XamlC
Is this bug a regression?: Yes
Last known good build: 2.3.1.114


Attachments
Test case (13.02 KB, application/zip)
2016-11-30 05:32 UTC, Brendan Zagaeski
Details

Description Brendan Zagaeski 2016-11-30 05:32:58 UTC
Created attachment 18714 [details]
Test case

For builds on Mac, XamlC or Cecil incorrectly updates .mdb files for iOS apps, maybe only when the Xamarin.Build.Download package is also used

(One common use of the Xamarin.Build.Download NuGet package is to support the Xamarin.Google.iOS.Analytics NuGet package.)

Note that this bug is different from non-public Bug 48285 because in this bug the XamlCTask _is_ updating the .mdb file (that is, in this case the `DebugSymbols` parameter is `true`).  The problem in this case is instead that the XamlC task seems to be corrupting the .mdb file.

While researching this bug I came across Bug 45021, and that bug does share at least a few similarities with the bad results for this bug.  So updating the Cecil version from 0.9.6.0 to 0.10 or higher in both the Xamarin.Forms NuGet package and the Xamarin.Build.Download might help with this bug too [1].  On the other hand, in my testing I was not able to replicate the problem on Windows.  But maybe the `pdb2mdb` conversion on Windows hides the bug in Cecil?



[1] I made sure to double-check the results below on Mono and Xamarin.iOS versions that included the Cecil update discussed in Bug 45021.  The same problems were still present, but the Xamarin.Forms and Xamarin.Build.Download packages also include their own internal copies of Cecil that I did _not_ attempt to update.




## Regression status: maybe a regression between 2.3.1.114 and 2.3.2.127

For this particular test case:

BAD:  2.3.3.168
BAD:  2.3.2.127
GOOD: 2.3.1.114
GOOD: 2.3.0.49

I am somewhat uncertain about this assessment of regression status due to the subtlety of this particular bug.  For example, maybe I would still be able to hit the problem on older versions of Xamarin.Forms if I used a different or more complicated test case.




## Steps to replicate


1. Unzip the attached test case into /Users/Shared/Projects/ so that the full path to the solution file is /Users/Shared/Projects/XamlCMdbFileMixup/FormsShared1.sln.  (.mdb files contain absolute paths, so this is step is necessary to ensure that you'll see the exact BAD results as reported below.)


2. Open the solution in Xamarin Studio to restore the NuGet packages.


3. Run a command line build for the "Debug|iPhoneSimulator" configuration with xbuild:

> $ xbuild /v:quiet /t:Build /p:Configuration="Debug" /p:Platform="iPhoneSimulator" FormsShared1.sln

4. Dump the mdb file using the included mdbdump.exe binary (or compile mdbdump from [1]).

> $ mono mdbdump.exe iOS/bin/iPhoneSimulator/Debug/FormsShared1.iOS.exe

[1] https://github.com/mono/mono/blob/master/mcs/tools/mdbdump/mdbdump.cs




## "More obvious" BAD results


(a) The "most obvious" bad behavior in the attached test case is that breakpoints do not work in the IDE.  For example, you can try adding a breakpoint in `Bar()` at FormsShared1Page.cs, line 19.  The debugger should break on that breakpoint once the app launches, but it doesn't.


(b) The output from `mdbdump` helps reveal what's happening.  The methods are matched up with the wrong sets of sequencepoints.  One clear example is that the `file_ref` for `FormsShared1.FormsShared1Page::Bar()` is wrong.

The .mdb file thinks the method is in `file_ref` 7:

> <method token="0x600000d" signature="System.Void FormsShared1.FormsShared1Page::Bar()">
>   <sequencepoints>
>     <entry il="0x0" row="20" col="44" file_ref="7" />
But `file_ref` 7 is the `.xaml.g.cs` file:
> <file id="7" name="FormsShared1.iOS..Users.Shared.Projects.XamlCMdbFileMixup.FormsShared1.FormsShared1Page.xaml.g.cs" checksum="e258a7f0299dd776486327ec29cc2354" />

In contrast, the sibling method `Foo()` has `file_ref` 4:
> <method token="0x600000c" signature="System.Void FormsShared1.FormsShared1Page::Foo()">
>   <sequencepoints>
>     <entry il="0x0" row="18" col="9" file_ref="4" />
And `file_ref` 4 is the correct `.xaml.cs` file:
> <file id="4" name="FormsShared1Page.xaml.cs" checksum="9922f60246ebf809eb63749d7ad5e6db" />   
_But_ if you look carefully at the sequence points, you'll see that the `row` attributes are still _incorrect_: `Foo()` has been matched up with the sequence points for `Bar()` rather than its own sequence points.



## "Less obvious" BAD results (after removing the Xamarin.Build.Download.targets `<Import>` element)

The Xamarin.Build.Download.targets `<Import>` element in the attached test case somehow helps expose the "more obvious" bad behavior.  In further testing, I found that running the XamarinBuildiOSResourceRestore task by itself seemed to be sufficient to get that "more obvious" behavior (as long as XamlC was still enabled).  I would guess that the XamarinBuildiOSResourceRestore task also makes use of Mono.Cecil and that the two overlapping uses of Mono.Cecil lead to a special failure case.

Removing the Xamarin.Build.Download.targets `<Import>` reveals the "normal" (though maybe still unintended) effects of the XamlC task:



### Steps

1. Remove or comment out the following line from FormsShared1.iOS:

<Import Project="..\packages\Xamarin.Build.Download.0.2.2\build\Xamarin.Build.Download.targets" Condition="Exists('..\packages\Xamarin.Build.Download.0.2.2\build\Xamarin.Build.Download.targets')" />


2. Rebuild the project:

> $ xbuild /v:quiet /t:Rebuild /p:Configuration="Debug" /p:Platform="iPhoneSimulator" FormsShared1.sln
(Caution: Using a command line build is recommended for this step.  If you use the IDE to build, you will need to _quit_ and reopen the IDE before rebuilding in order for the changes to take effect.)



### Results


(a) `Bar()` is matched up with the correct set of sequence points, _but_ now the `col` attributes are all set to "-1":

> <method token="0x600000d" signature="System.Void FormsShared1.FormsShared1Page::Bar()">
>   <sequencepoints>
>     <entry il="0x0" row="18" col="-1" file_ref="5" />
Note that the `col` attribute is apparently not required for the IDE to break successfully on breakpoints: _breakpoints work OK in this case_.


(b) Another (possibly intentional?) change is that the unused `AssemblyAttribute.cs` `file_ref` has been removed from the `mdbdump` output, and some empty elements have also been removed.

Perhaps these "less obvious" bad results are just a known side-effect of Cecil?  On the other hand, on Windows neither (a) nor (b) happens when XamlC is enabled, so these behaviors _might_ also be something slightly different and more problematic?




## GOOD Results: Disable XamlC



### Steps

1. Remove or comment out the following line from App.xaml.cs:

[assembly: Xamarin.Forms.Xaml.XamlCompilation(Xamarin.Forms.Xaml.XamlCompilationOptions.Compile)]


2. Rebuild the project:

> $ xbuild /v:quiet /t:Rebuild /p:Configuration="Debug" /p:Platform="iPhoneSimulator" FormsShared1.sln


### Results

`Bar()` is matched up with the correct set of sequence points, and the `col` attributes are all set to their correct values:

> <method token="0x600000c" signature="System.Void FormsShared1.FormsShared1Page::Bar()">
>   <sequencepoints>
>     <entry il="0x0" row="18" col="9" file_ref="4" />



## Testing environment info (brief)

Xamarin.Forms 2.3.3.168

Mono 4.8.0 (mono-4.8.0-branch/902b4a9)
Xamarin.iOS 10.4.0.25 (master: 0160ba1)

Mac OS 10.11.6
Comment 1 Stephane Delcroix 2017-01-18 13:14:10 UTC
Here's some info:
- For whatever reason, cecil 0.9.6 always generate -1 as `col`.
- even with Xamarin.Build.Download target turned off, some debug information are still off
- the messed up debug information comes from the duplicated `InitializeComponent` we added for the previewer to work even with XamlC on
- removing the duplication code produce correct debug symbols, even with Xamarin.Build.Download turned on
- cecil 0.10.0-beta2 generates the right `col` value.

Here's the mob info related to FormsShared1Page:

    <method token="0x600000a" signature="System.Void FormsShared1.FormsShared1Page::.ctor()">
      <sequencepoints>
        <entry il="0x0" row="7" col="16" file_ref="4" />
        <entry il="0x6" row="8" col="9" file_ref="4" />
        <entry il="0x7" row="9" col="13" file_ref="4" />
        <entry il="0x8" row="9" col="13" file_ref="4" />
        <entry il="0xd" row="10" col="13" file_ref="4" />
        <entry il="0xe" row="10" col="13" file_ref="4" />
        <entry il="0x13" row="11" col="9" file_ref="4" />
      </sequencepoints>
      <locals />
      <scopes />
    </method>
    <method token="0x600000b" signature="System.Void FormsShared1.FormsShared1Page::Foo()">
      <sequencepoints>
        <entry il="0x0" row="14" col="9" file_ref="4" />
        <entry il="0x1" row="15" col="9" file_ref="4" />
      </sequencepoints>
      <locals />
      <scopes />
    </method>
    <method token="0x600000c" signature="System.Void FormsShared1.FormsShared1Page::Bar()">
      <sequencepoints>
        <entry il="0x0" row="18" col="9" file_ref="4" />
        <entry il="0x1" row="19" col="13" file_ref="4" />
        <entry il="0x4" row="20" col="9" file_ref="4" />
      </sequencepoints>
      <locals>
        <entry name="x" il_index="0" scope_ref="0" />
      </locals>
      <scopes />
    </method>
    <method token="0x600000d" signature="System.Void FormsShared1.FormsShared1Page::InitializeComponent()">
      <sequencepoints>
        <entry il="0x0" row="18" col="44" file_ref="7" />
        <entry il="0x1" row="19" col="13" file_ref="7" />
        <entry il="0xc" row="19" col="51" file_ref="7" />
        <entry il="0x12" row="20" col="9" file_ref="7" />
      </sequencepoints>
      <locals />
      <scopes />
    </method>

With the files being:

  <files>
    <file id="1" name="Main.cs" checksum="aaa610a969fcf6a2ac800d47359586ae" />
    <file id="2" name="AppDelegate.cs" checksum="bb3f39dfb16fa81977b07b66cd83cc41" />
    <file id="3" name="App.xaml.cs" checksum="3c4fde433fffe35ae97783944d348207" />
    <file id="4" name="FormsShared1Page.xaml.cs" checksum="9922f60246ebf809eb63749d7ad5e6db" />
    <file id="5" name="Xamarin.iOS,Version=v1.0.AssemblyAttribute.cs" checksum="edee6220f0276d160b1300e9f4861679" />
    <file id="6" name="FormsShared1.iOS..Users.Shared.Projects.XamlCMdbFileMixup.FormsShared1.App.xaml.g.cs" checksum="33c537f4a9da4d69ca1f9523fb641b3f" />
    <file id="7" name="FormsShared1.iOS..Users.Shared.Projects.XamlCMdbFileMixup.FormsShared1.FormsShared1Page.xaml.g.cs" checksum="ba301b2b68200dafc6b82c4956e313ad" />
  </files>

As you can see, everything is correct (except that `InitializeComponent()` shouldn't have any sequence points defined, as the IL for that method has been completely replaced)
Comment 2 Brendan Zagaeski 2017-01-19 03:00:50 UTC
That sounds sensible and looks pretty good to me.  I wouldn't be too surprised to hear that simply updating the Cecil versions included in the Xamarin.Forms and Xamarin.Build.Download packages does indeed resolve this issue.

Comment 1 sounds like it might also already have been using a nice narrow test for determining the involvement of Cecil: start with the commit for the known "bad" version of Xamarin.Forms 2.3.3.168, update the Cecil reference to a "good" version (0.10.0-beta2), rebuild the Xamarin.Forms NuGet package, and then check the test case with the new package.  Is that indeed how the test in Comment 1 was done?  If so, then I think I'd be fine with having this bug marked "resolved fixed" as soon as the master branch of Xamarin.Forms has been updated to reference one of those new "good" versions of Cecil (which might have already happened).

If there is a Xamarin.Forms package easily available that already includes the newer Cecil version, we might as well do one additional quick check just to be extra thorough: diff the `mdbdump` output to compare between when XamlC is enabled vs. disabled to make sure my "by eye" assessment of the results in Comment 1 is accurate.  I'll tentatively plan to take a quick try at that when I get a chance.
Comment 3 Stephane Delcroix 2017-01-19 07:55:29 UTC
you're misreading me... the bug is not fixed, the pasted mdb is the one with the duplication code removed. But I can't remove that as it's required by the previewer.
Comment 4 Stephane Delcroix 2017-01-20 08:39:48 UTC
Here I am, 3 days later. And I figured it out...

> For builds on Mac, XamlC or Cecil incorrectly updates .mdb files for iOS apps

is the source of the confusion.

As it turns out, the .mdb file ISN'T modified at all. Modifying the Xamarin.Forms.targets in this way proves it:

    <Target Name="XamlC">
        <Exec Command="mdbdump.sh $(IntermediateOutputPath)$(TargetFileName);md5 $(IntermediateOutputPath)$(TargetFileName).mdb; sleep 10" />
	    <XamlCTask
                Assembly = "$(IntermediateOutputPath)$(TargetFileName)"
                ReferencePath = "@(ReferencePath)"
                Verbosity = "2"
                OptimizeIL = "true"
                DebugSymbols = "$(DebugSymbols)" />
        <Exec Command="mdbdump.sh $(IntermediateOutputPath)$(TargetFileName);md5 $(IntermediateOutputPath)$(TargetFileName).mdb; sleep 10" />
    </Target>

The mdbdump *looks* different, but as the md5hash shows, it wasn't modified, and the matching between the sequence points and the IL is wrong, then producing a bad mdbdump, and not allowing you to set breakpoints.

Forcing the load of the MdbReader and MdbWriter fixes it.

see https://github.com/xamarin/Xamarin.Forms/pull/699
Comment 5 Brendan Zagaeski 2017-01-23 19:01:15 UTC
Ohhh very interesting and nice catch!  Sounds good on the fix too.  Thanks much!
Comment 6 Stephane Delcroix 2017-02-02 10:22:04 UTC
this will be fixed in 2.3.5-pre1

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