[linker] Inline BinaryCompatibility.TargetsAtLeast_Desktop_V4_5[_1] - #1518
Merged
Conversation
This is a more interesting case since the inlining of 2 property getters
has a deeper impact and removes other types/methods from the final app.
Here we don't care what's inside the code we replace - only that it
always return a constant boolean after evaluation. That _constant_ is
something we can add tests to ensure we continue to behave identically.
```
--- before 2017-01-16 14:58:12.000000000 -0500
+++ after 2017-01-16 14:58:09.000000000 -0500
@@ -1580,7 +1580,6 @@
System.String System.AppDomain::get_BaseDirectory()
System.String System.AppDomain::get_FriendlyName()
System.String System.AppDomain::getFriendlyName()
-System.String System.AppDomain::GetTargetFrameworkName()
System.String System.AppDomain::ToString()
System.UnhandledExceptionEventHandler System.AppDomain::UnhandledException
System.UnhandledExceptionEventHandler System.AppDomain::UnhandledException
@@ -11949,54 +11948,12 @@
System.Reflection.FieldInfo System.Runtime.Serialization.ValueTypeFixupInfo::ParentField()
System.Runtime.Serialization.ValueTypeFixupInfo
System.Void System.Runtime.Serialization.ValueTypeFixupInfo::.ctor(System.Int64,System.Reflection.FieldInfo,System.Int32[])
-System.Runtime.Versioning.BinaryCompatibility
-System.Boolean System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap::TargetsAtLeast_Desktop_V4_5
-System.Boolean System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap::TargetsAtLeast_Desktop_V4_5_1
-System.Boolean System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap::TargetsAtLeast_Desktop_V4_5_2
-System.Boolean System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap::TargetsAtLeast_Desktop_V4_5_3
-System.Boolean System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap::TargetsAtLeast_Desktop_V4_5_4
-System.Boolean System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap::TargetsAtLeast_Desktop_V5_0
-System.Boolean System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap::TargetsAtLeast_Phone_V7_1
-System.Boolean System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap::TargetsAtLeast_Phone_V8_0
-System.Boolean System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap::TargetsAtLeast_Silverlight_V4
-System.Boolean System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap::TargetsAtLeast_Silverlight_V5
-System.Boolean System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap::TargetsAtLeast_Silverlight_V6
-System.Boolean System.Runtime.Versioning.BinaryCompatibility::get_TargetsAtLeast_Desktop_V4_5()
-System.Boolean System.Runtime.Versioning.BinaryCompatibility::get_TargetsAtLeast_Desktop_V4_5_1()
-System.Boolean System.Runtime.Versioning.BinaryCompatibility::ParseTargetFrameworkMonikerIntoEnum(System.String,System.Runtime.Versioning.TargetFrameworkId&,System.Int32&)
-System.Boolean System.Runtime.Versioning.BinaryCompatibility::TargetsAtLeast_Desktop_V4_5()
-System.Boolean System.Runtime.Versioning.BinaryCompatibility::TargetsAtLeast_Desktop_V4_5_1()
-System.Int32 System.Runtime.Versioning.BinaryCompatibility::AppWasBuiltForVersion()
-System.Int32 System.Runtime.Versioning.BinaryCompatibility::get_AppWasBuiltForVersion()
-System.Int32 System.Runtime.Versioning.BinaryCompatibility::s_AppWasBuiltForVersion
-System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap
-System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap System.Runtime.Versioning.BinaryCompatibility::s_map
-System.Runtime.Versioning.TargetFrameworkId System.Runtime.Versioning.BinaryCompatibility::AppWasBuiltForFramework()
-System.Runtime.Versioning.TargetFrameworkId System.Runtime.Versioning.BinaryCompatibility::get_AppWasBuiltForFramework()
-System.Runtime.Versioning.TargetFrameworkId System.Runtime.Versioning.BinaryCompatibility::s_AppWasBuiltForFramework
-System.Void System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap::.ctor()
-System.Void System.Runtime.Versioning.BinaryCompatibility/BinaryCompatibilityMap::AddQuirksForFramework(System.Runtime.Versioning.TargetFrameworkId,System.Int32)
-System.Void System.Runtime.Versioning.BinaryCompatibility::.cctor()
-System.Void System.Runtime.Versioning.BinaryCompatibility::ParseFrameworkName(System.String,System.String&,System.Int32&,System.String&)
-System.Void System.Runtime.Versioning.BinaryCompatibility::ReadTargetFrameworkId()
-System.Runtime.Versioning.CompatibilitySwitch
-System.String System.Runtime.Versioning.CompatibilitySwitch::GetValueInternal(System.String)
System.Runtime.Versioning.TargetFrameworkAttribute
System.String System.Runtime.Versioning.TargetFrameworkAttribute::_frameworkDisplayName
System.String System.Runtime.Versioning.TargetFrameworkAttribute::_frameworkName
System.String System.Runtime.Versioning.TargetFrameworkAttribute::FrameworkDisplayName()
System.Void System.Runtime.Versioning.TargetFrameworkAttribute::.ctor(System.String)
System.Void System.Runtime.Versioning.TargetFrameworkAttribute::set_FrameworkDisplayName(System.String)
-System.Int32 System.Runtime.Versioning.TargetFrameworkId::value__
-System.Runtime.Versioning.TargetFrameworkId
-System.Runtime.Versioning.TargetFrameworkId System.Runtime.Versioning.TargetFrameworkId::NetCore
-System.Runtime.Versioning.TargetFrameworkId System.Runtime.Versioning.TargetFrameworkId::NetFramework
-System.Runtime.Versioning.TargetFrameworkId System.Runtime.Versioning.TargetFrameworkId::NotYetChecked
-System.Runtime.Versioning.TargetFrameworkId System.Runtime.Versioning.TargetFrameworkId::Phone
-System.Runtime.Versioning.TargetFrameworkId System.Runtime.Versioning.TargetFrameworkId::Portable
-System.Runtime.Versioning.TargetFrameworkId System.Runtime.Versioning.TargetFrameworkId::Silverlight
-System.Runtime.Versioning.TargetFrameworkId System.Runtime.Versioning.TargetFrameworkId::Unrecognized
-System.Runtime.Versioning.TargetFrameworkId System.Runtime.Versioning.TargetFrameworkId::Unspecified
System.IntPtr System.RuntimeArgumentHandle::args
System.RuntimeArgumentHandle
System.Boolean System.RuntimeFieldHandle::Equals(System.Object)
```
```
Statistics
Native subtotal 35,729,800 35,728,760 -1,040 0.00 %
Executable 29,270,272 29,270,240 -32 0.00 %
AOT data *.aotdata 6,459,528 6,458,520 -1,008 -0.02 %
Managed *.dll/exe 4,537,344 4,532,736 -4,608 -0.10 %
TOTAL 40,581,685 40,576,037 -5,648 -0.01 %
```
Contributor
|
Build success |
Member
|
Instead of hard-coding the return value for the inlined functions, wouldn't it be possible to parse the actual IL of the inlined function? That would eliminate the need for the tests (and any updates whenever these values changes - which doesn't seem all that unlikely for some of these values). |
Contributor
Author
|
@rolfbjarne this case is unlike the previous one, where a bool constant was returned. Here the IL of the property is doing a lot of job (thru other types/methods) to parse That's why several symbols are removed by inlining the properties (nice) but why analyzing the IL becomes much harder (would need an interpreter). |
spouliot
commented
Jan 18, 2017
| var bc = Type.GetType ("System.Runtime.Versioning.BinaryCompatibility, mscorlib"); | ||
| Assert.IsNotNull (bc, "BinaryCompatibility"); | ||
| AssertCallStaticReturnBool (bc, "get_TargetsAtLeast_Desktop_V4_5", true); | ||
| AssertCallStaticReturnBool (bc, "get_TargetsAtLeast_Desktop_V4_5_1", false); |
Contributor
Author
There was a problem hiding this comment.
This sounds like a bug in mono -> https://bugzilla.xamarin.com/show_bug.cgi?id=51626
rolfbjarne
approved these changes
Jan 19, 2017
marek-safar
added a commit
to marek-safar/xamarin-macios
that referenced
this pull request
Feb 8, 2017
…_5[_1] (dotnet#1518)" This reverts commit e72b654.
marek-safar
added a commit
to marek-safar/xamarin-macios
that referenced
this pull request
Feb 8, 2017
…_5[_1] (dotnet#1518)" This reverts commit e72b654. because it's no longer needed with latest mono
spouliot
added a commit
that referenced
this pull request
Mar 29, 2017
* Bump to mono master * [build] Profiles cleanup * Remove 4.x profile dependency * Replace mdbrebase tool with pathmap and fix broken debug symbols mapping in public builds * Bump mono and linker submodule * Add netstandard to the build * [build] Register linker for submodule checks * Bump mono * [build] Add .NET Core open source key * [build] Update debug symbols to pdb * [msbuild] Remove smcs compiler overrides * Update after another Cecil breaking change * Fixes mmp execution during build to use correct Mono.Cecil. For some reason setting MONO_PATH does not work * [linker] Update list of Facade assemblies * [mtouch] Copy pdb debug symbols when available * [msbuild] Update test to reflect Mono.Dynamic.Interpreter.dll removal * [tuner] Update System.Core rule * [msbuild] Update few test expected debug symbols * Fixes missing Mono.Security dependency * Updates mtouch.csproj * Switch to Mono 2017-02 branch * Adds pdb handling to mtouch * Bump linker * Wire up configuration to MonoTouchSweepStep * Mono branch name update * Remove mdbrebase which is no longer needed (compiler does it now) * Update monomac tuner * Revert "[linker] Inline BinaryCompatibility.TargetsAtLeast_Desktop_V4_5[_1] (#1518)" This reverts commit e72b654. because it's no longer needed with latest mono * Bump maccore for mdoc path fix * Add missing reference * Bump linker * Updates mmp.csproj files path * [tests][mac] Terminate test processes with extreme prejudice. Fixes #52604. https://bugzilla.xamarin.com/show_bug.cgi?id=52604 * Bump Xamarin Studio to get fix for bug #52648. Bump Xamarin Studio to get fix for bug #52648 causing build failures. * Update system Mono to 2017-02 * More mdb to pdb checks update * Update tests to work with csc compiler * Another system mono bump * [tests] Fix mlaunch path, it's no longer shipped with Xamarin Studio. (#1842) * Fixes tuner IL optimization for get_Size to work with short branches * Update IntPtrSizeOptimization test to consider Debug mode IL output * Xamarin.Mac.FSharp.targets - Fix imports to work correctly with .. .. MSBuildExtensionsPath* . `MSBuildExtensionsPath*` supports search paths only if the string `$(MSBuildExtensionsPath)` (or *32/*64) appear in the `Project` attribute of the `Import` element. Otherwise the value of the property or the default value is used. So, restructure to conditionally import rather than a import using a property with the final path. * [msbuild] Index into the correct item array when printing an error message (#1792) Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=52860 When the ImageAssets contain an item that doesn't live within a *.xcassets directory, index into ImageAssets[] rather than items[] which hasn't been populated yet. Also fixed the tagsList for-loop to use tagsList.Count instead for correctness (even though tags.Count and tagsList.Count should always be identical). * [msbuild] Ignore .DS_Store files when cloning asset catalogs (#1776) Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=52851 The problem here is that the .DS_Store file was included in the .csproj file but did not exist on disk, so when we went to clone that file into the obj/ dir before running actool on it, File.Copy() would fail because the file did not actually exist. Since these files are worthless anyway, we can safely ignore them. Also added logic to verify that files exist before copying them in order to report a better error than an exception stack trace. * [msbuild] Make a build error more clearly worded (#1794) Related to https://bugzilla.xamarin.com/show_bug.cgi?id=52847 * [msbuild] Remove an unused enum that causes confusion (#1798) * [msbuild] sanity check TargetiOSDevice property for conflicts (#1799) Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=52847 If the device archs do not match the configuration, don't do a device-specific build. * [msbuild] Make sure to codesign appex dylibs (#1812) Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=52745 Added a new CodesignNativeLibraries task that scans for and then codesigns each *.dylib and *.metallib in the app bundle (minus those in the PlugIns and Watch dirs). * [msbuild] Run the _ComputeTargetArchitectures target before cleaning (#1828) Some of the Clean steps need the _ComputeTargetArchitectures target to run before they can properly do their thing because they depend on DeviceSpecific paths. Also added logic to rm -rf extension *.dSYM and framework *.dSYM dirs in the container app bin dir (which fails on xbuild due to a bug in xbuild... yay!) Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=53007 * [msbuild] ALWAYS log ibtool/actool exitCode != 0 as an error (#1840) Log this as an error even if the log file does not exist and/or is parseable. Previously, it was possible that if the log file didn't exist or it was perfectly validly formatted, we would never log that error code and so it's possible that we wouldn't log *any* error at all (e.g. empty log file). * [msbuild] Added logging for all of the overridden MSBuild default tasks (#1829) * [msbuild] Fixed PathUtils.AbsoluteToRelative() logic (#1861) * [msbuild] Fixed PathUtils.AbsoluteToRelative() logic Don't require the 'absolute' path argument to be a FullPath. The code already calls Path.GetFullPath() on the 'absolute' path anyway. Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=53176 * Fixed ResolveSymbolicLinks() to return the input path if realpath() fails * [msbuild] Use stamp files to force container app's _CompileToNative (#1878) Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=53303 * Bump mono * Another mono bump * Mono bump * Update .gitignore with pdb files * Bump mono * Mono bump * Revert "Bump maccore for mdoc path fix" This reverts commit 50f4564. * Bump maccore * Bump mono * Remove TlsProviderFactoryCallback * Update linker tests after appletls merge * Mono bump * Bump mono to include fix for bug #53481 * Bump mono * Bump system mono to get fix for #53481. https://bugzilla.xamarin.com/show_bug.cgi?id=53481 * Bump mono * Bump mono * Remove Mono.Dynamic.Interpreter from mtouch tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a more interesting case since the inlining of 2 property getters
has a deeper impact and removes other types/methods from the final app.
Here we don't care what's inside the code we replace - only that it
always return a constant boolean after evaluation. That constant is
something we can add tests to ensure we continue to behave identically.