Bug 60923 - Out-of-date symbols causes mtouch build failure
Summary: Out-of-date symbols causes mtouch build failure
Status: RESOLVED FIXED
Alias: None
Product: iOS
Classification: Xamarin
Component: Tools ()
Version: master
Hardware: PC Mac OS
: --- critical
Target Milestone: 15.6
Assignee: Sebastien Pouliot
URL:
: 61142 ()
Depends on:
Blocks:
 
Reported: 2017-11-30 09:36 UTC by Rolf Bjarne Kvinge [MSFT]
Modified: 2018-02-22 10:42 UTC (History)
7 users (show)

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

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 Developer Community or GitHub 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 Rolf Bjarne Kvinge [MSFT] 2017-11-30 09:36:47 UTC
Repro:

* See unit tests here: https://github.com/rolfbjarne/xamarin-macios/commit/c4bcbb83dfa271b58f387c3c1c30602dd4d416d4

The "SymbolsOutOfDate1(True)" and "SymbolsOutOfDate2(True)" tests fail with:

> /work/maccore/master/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/Current/bin/mtouch  --sim /work/maccore/master/xamarin-macios/tests/mtouch/bin/Debug/tmp-test-dir/Xamarin.MTouchTool.CreateTemporaryDirectory/testApp.app --sdkroot /Applications/Xcode91.app/Contents/Developer  --sdk 11.1 --debug /work/maccore/master/xamarin-macios/tests/mtouch/bin/Debug/tmp-test-dir/Xamarin.MTouchTool.CreateTemporaryDirectory/testApp.exe -r:/work/maccore/master/xamarin-macios/_ios-build/Library/Frameworks/Xamarin.iOS.framework/Versions/git/lib/mono/Xamarin.iOS/Xamarin.iOS.dll --nolink
> 	Using Xcode 9.1 found in /Applications/Xcode91.app/Contents/Developer
> 	Xamarin.iOS 11.7.0.60 using framework: /Applications/Xcode91.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator11.1.sdk
> 	A full rebuild will be performed because the cache is either incomplete or entirely missing.
> 	A full rebuild has been forced because the cache for testApp is not valid.
> 	error MT0009: Error while loading assemblies: /work/maccore/master/xamarin-macios/tests/mtouch/bin/Debug/tmp-test-dir/Xamarin.MTouchTool.CreateTemporaryDirectory/testApp.exe
> 	--- inner exception
> 	System.InvalidOperationException: Operation is not valid due to the current state of the object.
> 	  at Mono.Cecil.ModuleDefinition.ReadSymbols (Mono.Cecil.Cil.ISymbolReader reader) [0x0002f] in <46b785a1f80b443099f11bdcad3ffda0>:0 
> 	  at Mono.Cecil.ModuleReader.ReadSymbols (Mono.Cecil.ModuleDefinition module, Mono.Cecil.ReaderParameters parameters) [0x0004a] in <46b785a1f80b443099f11bdcad3ffda0>:0 
> 	  at Mono.Cecil.ModuleReader.CreateModule (Mono.Cecil.PE.Image image, Mono.Cecil.ReaderParameters parameters) [0x00081] in <46b785a1f80b443099f11bdcad3ffda0>:0 
> 	  at Mono.Cecil.ModuleDefinition.ReadModule (Mono.Disposable`1[T] stream, System.String fileName, Mono.Cecil.ReaderParameters parameters) [0x0000d] in <46b785a1f80b443099f11bdcad3ffda0>:0 
> 	  at Mono.Cecil.ModuleDefinition.ReadModule (System.String fileName, Mono.Cecil.ReaderParameters parameters) [0x0006c] in <46b785a1f80b443099f11bdcad3ffda0>:0 
> 	  at MonoTouch.Tuner.MonoTouchResolver.Load (System.String fileName) [0x00091] in /work/maccore/master/xamarin-macios/tools/mtouch/AssemblyResolver.cs:115 
> 	---
> 	  at MonoTouch.Tuner.MonoTouchResolver.Load (System.String fileName) [0x000aa] in /work/maccore/master/xamarin-macios/tools/mtouch/AssemblyResolver.cs:118 
> 	  at MonoTouch.Tuner.MonoTouchManifestResolver.Load (System.String file) [0x00041] in /work/maccore/master/xamarin-macios/tools/mtouch/AssemblyResolver.cs:37 
> 	  at Xamarin.Bundler.Target.Initialize (System.Boolean show_warnings) [0x0006c] in /work/maccore/master/xamarin-macios/tools/mtouch/Target.cs:220 
> 	  at Xamarin.Bundler.Application.Initialize () [0x006ed] in /work/maccore/master/xamarin-macios/tools/mtouch/Application.cs:1226 
> 	  at Xamarin.Bundler.Application.BuildInitialize () [0x00001] in /work/maccore/master/xamarin-macios/tools/mtouch/Application.cs:827 
> 	  at Xamarin.Bundler.Application+<>c.<BuildAll>b__138_0 (Xamarin.Bundler.Application v) [0x00000] in /work/maccore/master/xamarin-macios/tools/mtouch/Application.cs:782 
> 	  at System.Collections.Generic.List`1[T].ForEach (System.Action`1[T] action) [0x00024] in <4656b2b94f914437bce672312dd9e44b>:0 
> 	  at Xamarin.Bundler.Application.BuildAll () [0x00023] in /work/maccore/master/xamarin-macios/tools/mtouch/Application.cs:782 
> 	  at Xamarin.Bundler.Driver.Main2 (System.String[] args) [0x00488] in /work/maccore/master/xamarin-macios/tools/mtouch/mtouch.cs:1408 
> 	  at Xamarin.Bundler.Driver.Main (System.String[] args) [0x00015] in /work/maccore/master/xamarin-macios/tools/mtouch/mtouch.cs:933
Comment 1 Rolf Bjarne Kvinge [MSFT] 2017-11-30 09:38:58 UTC
@Chris, can you have a look at this?

I suspect it started with https://github.com/xamarin/xamarin-macios/commit/91ded43b0b13025f4d6583e4571f1af002517348, but I haven't actually tested it.
Comment 2 Rolf Bjarne Kvinge [MSFT] 2017-11-30 09:39:34 UTC
Also feel free to cherry-pick the unit tests with any fixes 🙂
Comment 3 Timothy Risi 2017-12-21 01:36:38 UTC
I ran the tests locally and saw the same failures, reverting the commit you linked in comment 1 did indeed cause the tests to pass.
Comment 4 Sebastien Pouliot 2018-01-08 20:34:52 UTC
Like mentioned on PR3079 I don't think it's a _regression_ as something is now broken, more as something different.

IOW I think an error should happen, it did not before (bad) so the current behaviour is _better_ (@Chris, @Rolf?). However it is right now inconsistent: only on .pdb (#1) and not very useful/actionable (#2).


1. I suspect Cecil's MDB "validity" detection has been broken for a quite a while - maybe since it moved to ikvm [2] (@Marek ?) It just was not triggered before the change (from comment #1) and might have caused some hard to reproduce issues in the past.


2. Even for portable pdb it's hard (without a cecil change) to report a better error [3]. @JB ?


[1] https://github.com/xamarin/xamarin-macios/pull/3079
[2] https://github.com/xamarin/xamarin-macios/pull/3079#issuecomment-352798899
[3] https://github.com/xamarin/xamarin-macios/pull/3079#issuecomment-352787934
Comment 5 Rolf Bjarne Kvinge [MSFT] 2018-01-09 06:17:42 UTC
(In reply to Sebastien Pouliot from comment #4)
> Like mentioned on PR3079 I don't think it's a _regression_ as something is
> now broken, more as something different.
> 
> IOW I think an error should happen, it did not before (bad) so the current
> behaviour is _better_ (@Chris, @Rolf?). However it is right now
> inconsistent: only on .pdb (#1) and not very useful/actionable (#2).

I don't think the current status is better than it was before:

* Projects that used to build, may stop building. IMHO this is a breaking change, and something we should try to avoid, at least if it's something the user can't easily understand and fix.
* The error message is unhelpful, and does not give the user any idea how to fix it.

I think the optimal solution would be to warn when encountering invalid/out-of-date debug files (either .pdb or .mdb), but still build the project successfully.

Until it can be fixed properly I think it might be best to revert the commit that broke this [1], since it fixed a less serious issue (broken debugging in a fairly rare scenario).

[1]: https://github.com/xamarin/xamarin-macios/commit/91ded43b0b13025f4d6583e4571f1af002517348
Comment 6 Marek Safar 2018-01-09 09:59:05 UTC
This is not broken validity check detection but the exact opposite of that.

I am also not sure this is a regression it could most likely happen before but probably just not as frequently.

What happens is that e.g. monotouch instructs cecil to read debug symbols but it does not handle any error conditions. In the test above there is PDB mismatch which Cecil reports as IOE (which is not the great exception) but there could be other error conditions (e.g incomplete pdb, i/o issue, etc).

I don't know why wrong PDB is in the bin folder but the solution to this could be another Cecil flag where you set ignoreInvalidDebugSymbols to true similarly to existing throwIfNoSymbol flag [0]. This does not address the general error handling.

I suspect this line is hit https://github.com/mono/cecil/blob/dfee11e80d59e1a3d6d9c914c3f277c726bace52/Mono.Cecil/ModuleDefinition.cs#L1090

[0]: https://github.com/mono/cecil/blob/dfee11e80d59e1a3d6d9c914c3f277c726bace52/Mono.Cecil.Cil/Symbols.cs#L766
Comment 7 Sebastien Pouliot 2018-01-09 14:14:06 UTC
@Marek the broken case is the MDB. The same situation goes undetected.

I'm not sure we should, at least silently, ignore broken symbols (either with a new flag or the mdb not having a working check).

Such cases only lead to other bug reports complaining about the debugger is not working and they can't be diagnosed with build logs (silent). They might not be reproducible locally (from scratch) even if a test case is supplied.
Comment 8 Marek Safar 2018-01-09 14:17:36 UTC
I see, do we know which tool or how the broken mdb was created in the first place? I thought XI switched away from MDB few releases ago
Comment 9 Sebastien Pouliot 2018-01-09 14:22:06 UTC
Failure can be duplicated with the test from https://github.com/xamarin/xamarin-macios/pull/3079/files

I'm not sure if Rolf extracted the test from a bug report or just to test the out-of-date condition.

As for .mdb we're still using `mcs` for the platform assemblies (the removal of `pmcs` is in progress). The .mdb files can also exists in other binaries (e.g. nuget) packages so the support for .mdb will need to remain for a long time.
Comment 10 Rolf Bjarne Kvinge [MSFT] 2018-01-09 14:38:47 UTC
(In reply to Sebastien Pouliot from comment #9)

> I'm not sure if Rolf extracted the test from a bug report or just to test
> the out-of-date condition.

I ran into it myself with a personal test project (while working on something else)
Comment 11 Marek Safar 2018-01-09 16:43:37 UTC
@Sebastien in the case of MDB the situation is same as same code is executed and Cecil will throw if .mdb MVID is different to .dll MVID which I don't see as bad behaviour.

I didn't try Rolf's repro to see where this old mdb comes from (and it can easily be badly packaged nuget).
Comment 12 Rolf Bjarne Kvinge [MSFT] 2018-01-09 20:06:52 UTC
@Sebastien: my test case from https://github.com/xamarin/xamarin-macios/pull/3079 is manually creating an invalid mdb/pdb, and then verifying that the build doesn't fail.

IMHO the serious problem here is that projects that used to build may now stop building in d15-6, and there might not be an easy fix for customers (if the invalid mdb comes from a nuget for instance).
Comment 13 Sebastien Pouliot 2018-01-09 20:37:32 UTC
@Rolf yes, I linked to the PR above :)

@Marek so the only reason for the matching MVID in Rolf's test case (PR) is because the same source were used, right ? so rebuilds should not be an issue (but it makes the test inconsistent - but that's not really a problem now that we know why).


Considering that non-reproducible debugging issues are common and that might be (one of) the cause(s) then we better revert it (in d15-6) - just because it might be common.

For master I'll re-open the original bug and we'll see to switch this to a _useful_ warning (not a rather _generic_ error) so it won't block anyone.
Comment 14 Sebastien Pouliot 2018-01-09 20:45:02 UTC
d15-6 https://github.com/xamarin/xamarin-macios/pull/3188
Comment 15 Sebastien Pouliot 2018-01-11 15:31:07 UTC
*** Bug 61142 has been marked as a duplicate of this bug. ***
Comment 16 Sebastien Pouliot 2018-01-11 16:21:56 UTC
PR 3188 was merged in d15-6 with https://github.com/xamarin/xamarin-macios/commit/eaff169db3300f39b198c62253aa9922e586af73

A new issue [1] was created so this can be handled, as a warning, for 15.7.

[1] https://github.com/xamarin/xamarin-macios/issues/3200