Remove stripped code #9269

danmoseley · 2017-02-02T06:55:18Z

Contributes to https://github.com/dotnet/coreclr/issues/4651

This applies all the changes that BCLRewriter does to our actual code, with the aim of making it possible to remove the rewriting step. I did this mostly with a Roslyn rewriter but some hand edits as well. Not ready for detailed review but perhaps someone can glance and see I'm not on the wrong track.

do the delete
diff x64 windows debug against regular rewrite output using decompiler and using apireviewer
diff unix, x86, and release binaries
review the changes, delete empty files
ideally, check the tests pass .. although the diff should be enough, this is a big change

Fixes #9089

danmoseley · 2017-02-02T07:01:35Z

There are some diffs that I can't avoid, principally the rewriter removes compiler generated default ctor from internal classes, so this is now present (eg., LocalDataStore()). Rewriter also adds [CompilerGenerated] on some private fields. Rewriter also inlines some constants, which I avoided to keep the code clear.

danmoseley · 2017-02-02T07:02:09Z

@jkotas

jkotas · 2017-02-02T07:04:18Z

src/mscorlib/corefx/SR.cs

-internal static class SR
+internal static class SR 
 {
-    public static string Arg_ArrayZeroError


This will cause build break on Unix #Resolved

jkotas · 2017-02-02T07:08:25Z

src/mscorlib/src/System/Diagnostics/Eventing/EventSource.cs

-
        /// <summary>
        /// Returns a session mask representing all sessions in which the activity 
        /// associated with the current thread is allowed  through the activity filter. 


Folks would prefer the EventSource and friends to be left intact (the other cleanup was rolled back in these files...) #Resolved

Should I introduce an #if CORECLR for these? Eg WriteEvent(..) are quite large. Don't know how much we care about size.

In reply to: 99062226 [](ancestors = 99062226)

To the extent possible, I'd rather have fewer ifdefs in this code simply because we'd like EventSource to be as close to the same as possible on all platforms. Are there particular methods that you're concerned about? I believe that all of the WriteEvent methods should be available on all platforms and should not get removed by the rewriter. Is this not the case?

jkotas · 2017-02-02T07:24:35Z

src/mscorlib/corefx/System/Threading/ClrThreadPoolBoundHandle.cs

    ///     Represents an I/O handle that is bound to the system thread pool and enables low-level
    ///     components to receive notifications for asynchronous I/O operations.
    /// </summary>
-    public sealed partial class ThreadPoolBoundHandle : IDisposable


This should stay as is - this type was moved from corefx, but the redundant copy in corefx was not removed yet. I have pending change to clean it up. #Resolved

jkotas · 2017-02-02T07:24:50Z

src/mscorlib/corefx/System/Threading/ClrThreadPoolPreAllocatedOverlapped.cs

    /// </summary>
    /// <seealso cref="ThreadPoolBoundHandle.AllocateNativeOverlapped(PreAllocatedOverlapped)"/>
-    public sealed class PreAllocatedOverlapped : IDisposable, IDeferredDisposable
+    internal sealed class PreAllocatedOverlapped : IDisposable, IDeferredDisposable


Same here #Resolved

jkotas · 2017-02-02T07:29:20Z

Looks reasonable to me so far

stephentoub · 2017-02-02T10:55:48Z

src/mscorlib/corefx/Interop/Unix/Interop.Libraries.cs

-    {
-        internal const string GlobalizationInterop = "System.Globalization.Native"; // CoreFX wrappers for ICU
-        // Shims
-        internal const string SystemNative = "System.Native";


Won't deleting these break the Unix build? #Resolved

Yes, I just hadn't got to that.

In reply to: 99095195 [](ancestors = 99095195)

stephentoub · 2017-02-02T10:57:55Z

src/mscorlib/corefx/System/Globalization/SortKey.cs

            _string   = str;
        }
-
-        [OnSerializing]


Anything on exposed types like this marked as [OnSerializing], [OnDeserializing], etc. should not be removed, as it's used via reflection as part of serialization/formatting. If it was being removed before, it was a bug and should have been listed in the model.xml to prevent them from being removed. #Resolved

Unix only, didn't diff those yet. I"ll revert any.

In reply to: 99095564 [](ancestors = 99095564)

Looks like yes it was previously stripping it. I'll open a bug in corefx to add tests for serializing those.

In reply to: 99196301 [](ancestors = 99196301,99095564)

Yes, rewriter was stripping some of them. I found all the ones getting stripped and reverted them, also opened this to add tests https://github.com/dotnet/corefx/issues/15731

stephentoub · 2017-02-02T10:58:27Z

src/mscorlib/corefx/System/IO/PathInternal.Unix.cs

-        /// </summary>
-        internal static bool IsDirectoryTooLong(string fullPath)
-        {
-            return fullPath.Length >= Interop.Sys.MaxPath;


Do we have any goal of keeping files consistent between CoreCLR and CoreFx? Or are these not used in CoreFx either?

@JeremyKuhne why do we have a copy of this in PathInternal in CoreFX?

We're trying to remove the pre-emptive max path checks.

@JeremyKuhne I interpret that to mean there is a good reason for both, and I can continue to delete what's not used by coreclr.

stephentoub · 2017-02-02T11:00:30Z

src/mscorlib/src/Microsoft/Win32/SafeHandles/SafeFileMappingHandle.cs


 namespace Microsoft.Win32.SafeHandles
 {
-    internal sealed class SafeFileMappingHandle : SafeHandleZeroOrMinusOneIsInvalid


Can you just delete the whole file instead?

I'll delete empty files

stephentoub · 2017-02-02T11:00:39Z

src/mscorlib/src/Microsoft/Win32/SafeHandles/SafeLocalAllocHandle.cs

    using System.Runtime.InteropServices;
    using System.Runtime.ConstrainedExecution;
-
-    internal sealed class SafeLocalAllocHandle : SafeBuffer {


stephentoub · 2017-02-02T11:01:03Z

src/mscorlib/src/Microsoft/Win32/SafeHandles/SafeViewOfFileHandle.cs


 namespace Microsoft.Win32.SafeHandles
 {
-    internal sealed class SafeViewOfFileHandle : SafeHandleZeroOrMinusOneIsInvalid


Ditto. Anywhere all of the code is being removed, we should be able to instead just remove the whole file.

Yes I just hadn't done that yet, you're too fast.

stephentoub · 2017-02-02T11:01:52Z

src/mscorlib/src/Microsoft/Win32/SafeHandles/Win32SafeHandles.cs

        }

        // A default constructor is needed to satisfy CoreCLR inheritence rules. It should not be called at runtime
-        protected SafeHandleZeroOrMinusOneIsInvalid()


Why are these being changed from protected to protected internal?

stephentoub · 2017-02-02T11:03:46Z

src/mscorlib/src/System/AggregateException.cs

-        /// 
-        /// See http://msdn.microsoft.com/en-us/library/x810d419.aspx
-        /// </summary>
-        private int InnerExceptionCount


Removing this will break the DebuggerDisplayAttribute (it should not have been removed by the rewriter). See the above comment.

Fixed, and checked all the other such attributes.

Thanks. This is also something it'd be good to have some tests in CoreFx for. We have them in a few libraries, with tests that look at types we expect to have debugger attributes, and then use some basic parsing and reflection to verify that the expected members exist and can be invoked. But it'd be good to effectively cover all of our types with such attributes; it's so easy to accidentally remove such members, either manually or via automatic tools like this.

stephentoub · 2017-02-02T11:06:52Z

src/mscorlib/src/System/AppDomainUnloadedException.cs

-        //
-        //This constructor is required for serialization.
-        //
-        protected AppDomainUnloadedException(SerializationInfo info, StreamingContext context) : base(info, context) {


I assume that an instance of this exception will never be thrown to user code? If it could be, removing this ctor will break code if that code tries to serialize and deserialize the exception it caught.

Brought back as part of bringing back all the serialization.

stephentoub · 2017-02-02T11:07:10Z

src/mscorlib/src/System/ApplicationId.cs

    using System.Security.Util;
    using System.Text;
    using System.Diagnostics.Contracts;
-


The whole file can be removed.

stephentoub · 2017-02-02T11:08:36Z

src/mscorlib/src/System/CfgParser.cs

-            }
-        }
-#endif
-    }


Whole file can be removed

stephentoub · 2017-02-02T11:10:30Z

src/mscorlib/src/System/Collections/CollectionBase.cs

            return InnerList.GetEnumerator();
        }

-        protected virtual void OnSet(int index, Object oldValue, Object newValue) { 


I'm not sure why all of these protecteds are being changed to protected internals. If this is what the rewriter is doing, seems like a bug in the rewriter. protected internal is more accessible than protected, not less, as protected internal is an or, not an and.

I don't know why either -- I rolled the back.

stephentoub · 2017-02-02T11:12:20Z

src/mscorlib/src/System/Collections/Comparer.cs

            m_compareInfo = culture.CompareInfo;
        }
-
-        private Comparer(SerializationInfo info, StreamingContext context) {            


Removing this will break deserialization. Do CoreFx tests pass with this change? If so, that's a gap in our tests.

This whole file should be deleted. The shipping public implementation is in corefx. It is just that something is keeping alive in CoreLib.

It's used in several places in Array and in some other types so I believe I have to keep it.

I brought back this with the other serialiation stuff.

It is used as internal implementation detail that cannot leak and thus cannot be serialized. It was renamed to LowLevelComparer (with more cruft stripped) in CoreRT. We should get it eventually as we are unifying the CoreCLR and CoreRT impls.

stephentoub · 2017-02-02T11:13:56Z

src/mscorlib/src/System/Collections/Concurrent/ConcurrentQueue.cs

-        /// <returns>true if the object was added successfully; otherwise, false.</returns>
-        /// <remarks>For <see cref="ConcurrentQueue{T}"/>, this operation will always add the object to the
-        /// end of the <see cref="ConcurrentQueue{T}"/>
-        /// and return true.</remarks>


This should break the build: without these members, the type won't properly implement the interface it claims to implement.

(Oh, I see, the interface was modified as well.)

stephentoub · 2017-02-02T11:15:23Z

src/mscorlib/src/System/Collections/Concurrent/OrderablePartitioner.cs

 namespace System.Collections.Concurrent
 {

-    /// <summary>


Delete the whole file

stephentoub · 2017-02-02T11:15:38Z

src/mscorlib/src/System/Collections/Concurrent/Partitioner.cs


 namespace System.Collections.Concurrent
 {
-    /// <summary>


Delete the whole file

stephentoub · 2017-02-02T11:17:19Z

src/mscorlib/src/System/Collections/Generic/Comparer.cs

        }

-        // Used by the serialization engine.
-        private Int32EnumComparer(SerializationInfo info, StreamingContext context) { }


These deserialization ctors should not be removed. We have tests for this in CoreFx (I know because I recently added them)... they pass with this change?

stephentoub · 2017-02-02T11:18:23Z

src/mscorlib/src/System/Collections/Hashtable.cs

-        // To avoid bad behavior you want it to be the case that the increment is large even for small values (because small 
-        // values tend to happen more in practice). Thus we multiply seed by a number that will make these small values
-        // bigger (and not hurt large values). We picked HashPrime (101) because it was prime, and if hashSize-1 is not a multiple of HashPrime
+        // While this works well for �uniformly distributed� keys, in practice, non-uniformity is common. 


Some characters are messed up

stephentoub · 2017-02-02T11:18:43Z

src/mscorlib/src/System/Collections/KeyValuePairs.cs


 namespace System.Collections {
    using System.Diagnostics;
-


Delete the whole file

stephentoub · 2017-02-02T11:19:01Z

src/mscorlib/src/System/Collections/SortedList.cs

    using System.Diagnostics.CodeAnalysis;
    using System.Diagnostics.Contracts;
    using System.Globalization;
-


Delete the whole file

stephentoub · 2017-02-02T11:19:32Z

src/mscorlib/src/System/Collections/Stack.cs

-    [DebuggerDisplay("Count = {Count}")]
-[System.Runtime.InteropServices.ComVisible(true)]
-    [Serializable]
-    public class Stack : ICollection, ICloneable {


Delete the whole file

…aning

danmoseley · 2017-02-05T05:00:17Z

Fixed binary .props file.
All CoreFX tests pass against release x64.

If you get to it first, please squash before commit...

danmoseley · 2017-02-05T05:20:14Z

@ericstj --

D:\j\workspace\debug_windows---17180f1b\Tools\Packaging.targets(1061,5): error MSB4018: The "GeneratePackageReport" task failed unexpectedly. [D:\j\workspace\debug_windows---17180f1b\src\.nuget\Microsoft.NETCore.Runtime.CoreCLR\Microsoft.NETCore.Runtime.CoreCLR.pkgproj]
21:09:19 D:\j\workspace\debug_windows---17180f1b\Tools\Packaging.targets(1061,5): error MSB4018: System.IO.IOException: The process cannot access the file 'D:\j\workspace\debug_windows---17180f1b\bin\Product\Windows_NT.x64.Debug\mscordaccore_amd64_amd64_4.6.25005.00.dll' because it is being used by another process. [D:\j\workspace\debug_windows---17180f1b\src\.nuget\Microsoft.NETCore.Runtime.CoreCLR\Microsoft.NETCore.Runtime.CoreCLR.pkgproj]
21:09:19 D:\j\workspace\debug_windows---17180f1b\Tools\Packaging.targets(1061,5): error MSB4018:    at System.IO.__Error.WinIOError(Int32 errorCode, String maybeFullPath) [D:\j\workspace\debug_windows---17180f1b\src\.nuget\Microsoft.NETCore.Runtime.CoreCLR\Microsoft.NETCore.Runtime.CoreCLR.pkgproj]
21:09:19 D:\j\workspace\debug_windows---17180f1b\Tools\Packaging.targets(1061,5): error MSB4018:    at System.IO.FileStream.Init(String path, FileMode mode, FileAccess access, Int32 rights, Boolean useRights, FileShare share, Int32 bufferSize, FileOptions options, SECURITY_ATTRIBUTES secAttrs, String msgPath, Boolean bFromProxy, Boolean useLongPath, Boolean checkHost) [D:\j\workspace\debug_windows---17180f1b\src\.nuget\Microsoft.NETCore.Runtime.CoreCLR\Microsoft.NETCore.Runtime.CoreCLR.pkgproj]
21:09:19 D:\j\workspace\debug_windows---17180f1b\Tools\Packaging.targets(1061,5): error MSB4018:    at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access, FileShare share) [D:\j\workspace\debug_windows---17180f1b\src\.nuget\Microsoft.NETCore.Runtime.CoreCLR\Microsoft.NETCore.Runtime.CoreCLR.pkgproj]
21:09:19 D:\j\workspace\debug_windows---17180f1b\Tools\Packaging.targets(1061,5): error MSB4018:    at Microsoft.DotNet.Build.Tasks.Packaging.VersionUtility.GetAssemblyVersion(String assemblyPath) [D:\j\workspace\debug_windows---17180f1b\src\.nuget\Microsoft.NETCore.Runtime.CoreCLR\Microsoft.NETCore.Runtime.CoreCLR.pkgproj]

danmoseley · 2017-02-05T05:20:27Z

@dotnet-bot test Windows_NT x64 Debug Build and Test please (as above)

mattwarren · 2017-02-09T13:17:22Z

@danmosemsft Just a small thing I noticed, when you made the changes to SharedStatics.cs (diff from #9269), it stripped out the public/internal properties/methods, but left the private backing fields behind you can see that they are still there

Should they be removed as well?

danmoseley · 2017-02-15T17:35:38Z

@mattwarren thanks, did you notice anything else? I can remove those.

mattwarren · 2017-02-15T17:45:47Z

No sorry, I was only looking at SharedStatics.cs when I noticed the issue.

We're these changes done with a tool and is there a chance that the tool isn't properly stripping out private code that was only used by the public code it removed?

danmoseley · 2017-02-15T20:35:20Z

I diffed assemblies before and after BCLRewriter then used Roslyn to remove all those types. I did some manual work as the rewriter had bugs (as did my Roslyn code). Then I diffed the API manually including privates. There's enough noise at the level of privates though that I can imagine some thing slipped through. I do'nt plan to do another pass unless there's some big mistake I made.

Delete essentially all the code that the BCL rewriter is removing. Added back some code it shouldn't have been removing, eg., serialization code, debugger visualizers, and some changing of 'protected' to 'protected internal'

Delete essentially all the code that the BCL rewriter is removing. Added back some code it shouldn't have been removing, eg., serialization code, debugger visualizers, and some changing of 'protected' to 'protected internal' Commit migrated from dotnet/coreclr@ee5862c

Remove SIMD dead code

089dbcc

Fixes #9089

dnfclas added the cla-already-signed label Feb 2, 2017

jkotas reviewed Feb 2, 2017

View reviewed changes

stephentoub reviewed Feb 2, 2017

View reviewed changes

src/mscorlib/src/System/CfgParser.cs

}

}

#endif

}

Copy link

Member

stephentoub Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whole file can be removed

stephentoub reviewed Feb 2, 2017

View reviewed changes

danmoseley added 10 commits February 4, 2017 20:00

more empty

8e52ba9

more empty

1c7ba74

Remove empty files

9ef1f77

Rollback Eventing

8ed099c

Revert protected internal to protected

5665eb3

Fix Unix build

62da3db

Bit of #if for unix

e5e11cb

Revert model.xml line

e29a1ab

Merge branch 'cleaning' of https://github.com/dotnet/coreclr into cle…

e0c6243

…aning

Fix binary flag

3ce54dc

danmoseley merged commit ee5862c into master Feb 5, 2017

danmoseley deleted the cleaning branch February 5, 2017 06:36

This was referenced Feb 6, 2017

Add-Type reference assembly bug PowerShell/PowerShell#1616

Closed

(WIP)(MODULES-3945) Cross-platform PowerShell Support puppetlabs/puppetlabs-powershell#152

Closed

mattwarren mentioned this pull request Feb 15, 2017

Remove code corresponding to always defined symbols #9542

Merged

danmoseley mentioned this pull request Feb 15, 2017

Remove dead code #9613

Merged

lzybkr mentioned this pull request Feb 27, 2017

Add WithTopLevelBinderFlags call hack to Add-Type PowerShell/PowerShell#2704

Closed

karelz modified the milestone: 2.0.0 Aug 28, 2017

This was referenced Jan 31, 2020

Cleanup __Error vs Win32Marshal duplication dotnet/runtime#7355

Closed

Remove BCL rewriter dotnet/runtime#7357

Closed

Remove stripped code #9269

Remove stripped code #9269

Uh oh!

Conversation

danmoseley commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Feb 2, 2017

Uh oh!

danmoseley commented Feb 2, 2017

Uh oh!

jkotas Feb 2, 2017 • edited by danmoseley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Feb 2, 2017 • edited by danmoseley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Feb 2, 2017 • edited by danmoseley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Feb 2, 2017 • edited by danmoseley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Feb 2, 2017

Uh oh!

stephentoub Feb 2, 2017 • edited by danmoseley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Feb 2, 2017 • edited by danmoseley Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

danmoseley commented Feb 2, 2017 •

edited

Loading

jkotas Feb 2, 2017 •

edited by danmoseley

Loading

jkotas Feb 2, 2017 •

edited by danmoseley

Loading

jkotas Feb 2, 2017 •

edited by danmoseley

Loading

jkotas Feb 2, 2017 •

edited by danmoseley

Loading

stephentoub Feb 2, 2017 •

edited by danmoseley

Loading

stephentoub Feb 2, 2017 •

edited by danmoseley

Loading

stephentoub Feb 2, 2017 •

edited

Loading