Initial support for preinitialized array in CoreRT by yizhang82 · Pull Request #3558 · dotnet/corert

yizhang82 · 2017-05-09T07:20:11Z

Added PreInitDataField field in FieldDesc/EcmaField to return the corresponding RVA static field pointing to preinitialized data blob (as indicated by [PreInitialized] and [InitDataBlobAttribute]
Added FrozenStaticFieldArrayNode for frozen arrays with data extracted from preinit RVA data field
Added GCStaticsPreInitDataNode (CoreRT only) to hold GC pointers for GC static fields. Pre-init data fields points to frozen data, while other data initialized to null. They are stored in a new R2R section but perhaps this isn't really necessary (I was using it as parallel data structure but that isn't really necessary anymore). I'll remove the R2R section.
Changed GCStaticsNode to emit a pointer reloc to GCStaticspreInitDataNode and mask 0x2 for the GC static EEType
Changed InitializedStatics in StartupCodeHelper to check for 0x2 and memcpy the GC field data as needed

Program.cs and Repro.il are simply there for my testing convenience. I'll remove them before merging.

A few TODOs

Add support for fixups (FixupRuntimeTypeHandle, __addrof fixups, etc)
Enable this in ProjectX (should be easy)
Test support - not sure if we have IL support in test infra and this isn't really testable in C# (at least not without the IL transform)
I also want to see if we can easily hook up IL compilation in the repro project (probably a reproIL project), for future feature work that requires playing with IL in repro project (I had to compile repro.il manually and copy it over)

@davidwrighton @MichalStrehovsky @nattress PTAL
/cc @fadimounir @hoyMS

MichalStrehovsky · 2017-05-09T16:53:01Z

src/Common/src/Internal/Runtime/ModuleHeaders.cs

        StringTable = 200, // Unused
-        GCStaticRegion = 201,
-        ThreadStaticRegion = 202,
+        GCStaticsRegion = 201,


These renames are unnecessary and expand the already big diff to include unrelated files. This slows down the code reviews, but maybe it's just me.

MichalStrehovsky · 2017-05-09T16:55:53Z

src/Common/src/Internal/Runtime/CompilerHelpers/StartupCodeHelpers.cs

+                            // Skip SyncBlock + EEType when calculating field size
+                            int fieldSize = (int)obj.EETypePtr.BaseSize - IntPtr.Size * 2;
+
+                            RuntimeAugments.BulkMoveWithWriteBarrier(


This should use RuntimeImports directly. RuntimeAugments is for use outside of CoreLib. This will also fix the build break.

Seems like RhBulkMoveWithWriteBarrier also doesn't require the buffers to be pinned (it takes ref parameters), so the line below can be changed from new IntPtr((byte*)pEEType + IntPtr.Size) to ref obj.GetRawData()

MichalStrehovsky · 2017-05-09T16:57:39Z

src/Common/src/Internal/Runtime/CompilerHelpers/StartupCodeHelpers.cs

-                    object obj = RuntimeImports.RhNewObject(new EETypePtr(new IntPtr((*pBlock).ToInt64() & ~0x1L)));
+                    object obj = RuntimeImports.RhNewObject(new EETypePtr(new IntPtr(blockAddr & ~0x3L)));
+
+                    if ((blockAddr & 0x2L) == 2)


This is starting to be way too many bits and masking. Can you add the constants and masks to Common\src\Internal\Runtime\RuntimeConstants.cs? That file is included both in the class library and in the compiler.

Sure. Added GCStaticRegionConstants

MichalStrehovsky · 2017-05-09T17:07:03Z

src/Common/src/TypeSystem/Common/FieldDesc.cs

+        /// whose RVA points to the contents of the value (such as array).
+        /// Returns the corresponding field that contains preinitialized data.
+        /// </summary>
+        public virtual FieldDesc PreInitDataField => null;


We haven't been putting things that don't exist in the ECMA-335 spec into the core type system.

Getting at the preinitialized data should ideally be a service provided by something else, not by the field itself.

All the code that assumes a field knows how to provide it's preinitialized data will need to be rewritten as soon as we start writing our own StaticInitDataTransform equivalent in the compiler. I would prefer we have a centralized place that provides preinitialized data blobs for a given type right away instead of having to rewrite all of this. The mechanism by which the preinitialized data is calculated (be it custom attributes, or interpreting the cctors) should be an implementation detail of that component and shouldn't leak into many components.

Great point. I'm also a bit unsure about whether I should put the preinitialized information (and learning things as I go) so putting on FieldDesc seems to be the most natural choice at that moment :) But you are right and I agree - moving this into its own helper class seems to be a better choice, and abstracting out the details of where the preinitialized data are is also very nice . I was mostly worry about caching the result but it looks like a natural place to avoid the duplicate calculation is in EETypeNode. I'll make the change

MichalStrehovsky · 2017-05-09T17:20:50Z

src/Common/src/TypeSystem/Common/TypeDesc.cs

                case WellKnownType.Array:
                case WellKnownType.MulticastDelegate:
                case WellKnownType.Exception:
+                case WellKnownType.Type:


The well known types mechanism is used to access types that the type system itself needs to be able to get at in a quick fashion. I don't think this qualifies. You can just rewrite:

if (decodedValue.FixedArguments[0].Type != field.Context.GetWellKnownType(WellKnownType.Type))

into

if (!(decodedValue.FixedArguments[0].Value is TypeDesc))

Or use the check we use elsewhere.

OK. Changed to use TypeDesc instead.

MichalStrehovsky · 2017-05-09T18:02:21Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GCStaticsNode.cs

            _type = type;
+
+            // Go through and find all fields with preinitialized data
+            foreach (var field in _type.GetFields())


Why can't GCStaticsPreInitDataNode go over the field list and sort it? It's rather odd to do sorting here even though it's a different class that actually needs the sorted field list...

agreed. Forgot to clean this up. Moved to GCStaticsPreInitDataNode

MichalStrehovsky · 2017-05-09T18:08:50Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GCStaticsPreInitDataNode.cs

+
+        protected override DependencyList ComputeNonRelocationBasedDependencies(NodeFactory factory)
+        {
+            // relocs has all the dependencies we need


Then don't override the method :).

MichalStrehovsky · 2017-05-09T18:13:16Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GCStaticsPreInitDataNode.cs

+
+            return GetDataForPreInitDataField(
+                this, _type, _sortedPreInitFields, 
+                factory.Target.LayoutPointerSize.AsInt,     // CoreRT static size calculation includes EEType - skip it


factory.Target.PointerSize

MichalStrehovsky · 2017-05-09T18:18:34Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GCStaticsPreInitDataNode.cs

+        {
+            ObjectDataBuilder builder = new ObjectDataBuilder(factory, relocsOnly);
+
+            builder.RequireInitialAlignment(_type.GCStaticFieldAlignment.AsInt);


Should this be just 1? We are going to bitblt the contents of this into a GC-allocated memory region anyway. Aligning to anything else seems like a waste of space.

The gc safe memory copying routine is permitted to rely on aligned buffers to determine if it needs to operate in a GC safe manner. Its best to keep this aligned.

MichalStrehovsky · 2017-05-09T18:22:07Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GCStaticsPreInitDataNode.cs

+
+            int staticOffset = startOffset;
+            int staticOffsetEnd = _type.GCStaticFieldSize.AsInt;
+            int idx = 0;


This will always be zero. Should we increment it sometimes?

Oops. Forgot to increase. Fixed.

MichalStrehovsky · 2017-05-09T20:13:00Z

src/Common/src/Internal/Runtime/CompilerHelpers/StartupCodeHelpers.cs

+                        fixed (IntPtr* pEEType = &obj.m_pEEType)
+                        {
+                            // Skip SyncBlock + EEType when calculating field size
+                            int fieldSize = (int)obj.EETypePtr.BaseSize - IntPtr.Size * 2;


(int)obj.EETypePtr.BaseSize - (sizeof(ObjHeader) + sizeof(EEType*)) would be more declarative.

I'm wondering if this should be an API on EETypePtr.

I'm wondering if this should be an API on EETypePtr.

On a second thought, we might want to make this an internal method on System.Object itself. This complements the existing GetRawData method on System.Object: it gives you the size of the raw data. I would call this GetRawDataSize(). That way we avoid hardcoding assumptions about System.Object's layout in here. which just feels dirty.

I've added both on Object and EEType.

davidwrighton

Needs a bit more work. Most important to me is to leave the api surface for encoding data such that the backend compiler is able to provide pre-initialized state.

davidwrighton · 2017-05-10T22:54:08Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GCStaticsPreInitDataNode.cs

+        {
+            ObjectDataBuilder builder = new ObjectDataBuilder(factory, relocsOnly);
+
+            builder.RequireInitialAlignment(_type.GCStaticFieldAlignment.AsInt);


The gc safe memory copying routine is permitted to rely on aligned buffers to determine if it needs to operate in a GC safe manner. Its best to keep this aligned.

davidwrighton · 2017-05-10T22:54:48Z

src/ILCompiler/repro/Program.cs

 // See the LICENSE file in the project root for more information.

 using System;
+using System.Runtime.InteropServices;


Please don't check in changes to the standard repro program.

This is only for my convenience. Will remove before check-in.

davidwrighton · 2017-05-10T22:55:08Z

src/ILCompiler/repro/repro.il

@@ -0,0 +1,218 @@
+
+//  Microsoft (R) .NET Framework IL Disassembler.  Version 4.6.1055.0


Please don't check in a new repro il file.

davidwrighton · 2017-05-10T22:57:24Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/FrozenArrayNode.cs

+                throw new NotSupportedException();
+            }
+
+            FieldDesc arrayDataField = _arrayField.PreInitDataField;


Encoding of data should be flexible to allow the backend compiler to compute preinitialized data as well as accepting input from the IL dll that describes the preinitialized data.

fadimounir · 2017-05-11T23:14:50Z

src/Common/src/Internal/Runtime/ModuleHeaders.cs

+
+        /// <summary>
+        /// Contains pointer relocs to preinitialized data in FrozenObjectRegion
+        /// </summary>


Nit: comment can be omitted. The enum name seems descriptive enough

jkotas · 2017-05-16T15:10:27Z

src/System.Private.CoreLib/src/System/Runtime/RuntimeImports.cs

        [RuntimeImport(RuntimeLibrary, "RhBulkMoveWithWriteBarrier")]
        internal static extern unsafe void RhBulkMoveWithWriteBarrier(ref byte dmem, ref byte smem, nuint size);

+        internal static unsafe void RhBulkMoveWithWriteBarrier(IntPtr dmem, IntPtr smem, int size)


We prefer to use pointers when dealing with pointers, not IntPtr.

MichalStrehovsky · 2017-05-16T21:25:20Z

src/Common/src/Internal/Runtime/CompilerHelpers/StartupCodeHelpers.cs

+                        // which are pointer relocs to GC objects in frozen segment. 
+                        // It actually has all GC fields including non-preinitialized fields and we simply copy over the
+                        // entire blob to this object, overwriting everything. 
+                        fixed (void* pEEType = &obj.m_pEEType)


Why do you need the pinning here?

The existing import of RhBulkMoveWithWriteBarrier that takes refs will work fine with the interor pointer returned by calling obj.GetRawData(). It will also avoid another hardcoding of Object's layout (in the + IntPtr.Size part down below). It looks like a net win not to pin and avoid adding the new API to RuntimeImports...

MichalStrehovsky · 2017-05-16T21:28:14Z

src/Common/src/Internal/Runtime/EEType.cs

+        {
+            get
+            {
+                return BaseSize - (UInt32) sizeof(ObjHeader) - (UInt32) sizeof(EEType*);


As I wrote in my previous comment, I would prefer this to be implemented in System.Object. The sizeof(EEType*) is an implementation detail of System.Object that is hardcoding that System.Object has one pointer-sized field.

Oh, I see your pointer now. That makes sense. Fixed.

MichalStrehovsky · 2017-05-16T21:29:21Z

src/Common/src/Internal/Runtime/RuntimeConstants.cs

+        /// </summary>
+        public const int HasPreInitializedData = 0x2; 
+
+        public const int All = Initialized + HasPreInitializedData; 


Nit: call it Mask or something like that?

MichalStrehovsky · 2017-05-16T21:29:29Z

src/Common/src/TypeSystem/Common/WellKnownType.cs


        TypedReference,
-        ByReferenceOfT,
+        ByReferenceOfT


Unnecessary change?

MichalStrehovsky · 2017-05-16T21:31:17Z

src/Common/src/TypeSystem/Ecma/EcmaField.cs

        {
            _type = type;
            _handle = handle;
-


Changes to this file don't seem necessary anymore and leave EcmaField APIs inconsistent with EcmaMethod and EcmaType. Revert?

MichalStrehovsky · 2017-05-16T21:43:14Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/FrozenArrayNode.cs

+        private IEETypeNode GetEETypeNode(NodeFactory factory)
+        {
+            var fieldType = _preInitFieldInfo.Field.FieldType;
+            Debug.Assert(factory.IsLocalTypeSymbol(fieldType));


Please delete the IsLocalTypeSymbol helper and inline it here. It's not doing anything sophisticated and results in an extra hashtable lookup to find the EEType (which you get on the line just below this).

Makes sense. Changed to assert on !RepresentsIndirectionCell.

MichalStrehovsky · 2017-05-16T22:17:54Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GCStaticsNode.cs

            if (factory.Target.Abi == TargetAbi.CoreRT)
            {
-                builder.EmitPointerReloc(GetGCStaticEETypeNode(factory), 1);
+                int delta = 1;


Add using GCStaticRegionConstants = Internal.Runtime.GCStaticRegionConstants in the using blocks above and make this GCStaticRegionConstants.Initialized

Find out that the name of the constant is misleading.

(Can you use the named constant here and rename it to Uninitialized? Same for the 3 below - just | it with the other constant.)

Oh, yeah, that was backwards. Changed to Uninitialized and switched to use GCStaticRegionConstants.

Didn't realize we could use the runtime constants in the compiler. Looks like we just include it. that's pretty neat.

Didn't realize we could use the runtime constants in the compiler. Looks like we just include it. that's pretty neat.

Yup, I started pushing for it because it makes it way easier to connect the places in the classlib that use something with the place in the compiler that emits it.

MichalStrehovsky · 2017-05-16T22:22:32Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GCStaticsPreInitDataNode.cs

+            return nameMangler.NodeMangler.GCStatics(type) + "__PreInitData";
+        }
+
+        public virtual bool IsExported(NodeFactory factory) => factory.CompilationModuleGroup.ExportsType(Type);


Why do we need to export this?

Not really. This is a copy/paste mistake.

MichalStrehovsky · 2017-05-16T22:22:52Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GCStaticsPreInitDataNode.cs

+
+        public override bool StaticDependenciesAreComputed => true;
+
+        public override ObjectNodeSection Section => ObjectNodeSection.DataSection;


Should this be in the read only data section on Windows?

Yes. Changed to ReadOnlyDataSection.

MichalStrehovsky · 2017-05-16T22:25:48Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GCStaticsPreInitDataNode.cs

+    public class GCStaticsPreInitDataNode : ObjectNode, IExportableSymbolNode
+    {
+        private MetadataType _type;
+        private List<PreInitFieldInfo> _sortedPreInitFields;


It might be better to wrap this in a class that can write itself into an ObjectDataBuilder you pass to it instead of passing around a MetadataType and a list of PreInitFieldInfos.

You'll find that handy when you port this to Project X because we won't be allocating this node there, but you'll want most of what you put in this node's logic (the sorting, etc.).

A scheme like this:

class TypePreinitializationInfo { public bool HasInitializedGCStaticData { get; } public void WriteGCStaticData(ref ObjectDataBuilder builder, NodeFactory factory) { // Foreach field, call Write(). Insert padding where needed. } // Future: public void WriteNonGCStaticData(ref ObjectDataBuilder builder, NodeFactory factory) } abstract class FieldPreinitializationInfo { public FieldDesc Field { get; } public abstract void Write(ref ObjectDataBuilder builder, NodeFactory factory); } class ArrayFieldPreinitializationInfo : FieldPreinitializationInfo { public override void Write(ref ObjectDataBuilder builder, NodeFactory factory) { builder.EmitPointerReloc(factory.SerializedFrozenArray(this)); } }

These can be allocated in a separate class (PreinitializationManager?) that is accessible from NodeFactory.

This will put us into a perfect place for introducing a StaticInitDataTransform later.

Sounds reasonable but this change is getting pretty big as it is. I'll address this in my next change.

yizhang82 · 2017-05-18T23:44:57Z

@MichalStrehovsky @davidwrighton I believe I have addessed all feedback. PTAL. I'm planning to add IL test and a IL repro project in my next change (acccording to @MichalStrehovsky this might require CLI change so might not be possible yet).

MichalStrehovsky

Looks good, but there are a few opportunities to delete unused code that shouldn't be missed.

MichalStrehovsky · 2017-05-19T01:55:00Z

src/Common/src/TypeSystem/Ecma/EcmaField.cs

        {
-            return !MetadataReader.GetCustomAttributeHandle(MetadataReader.GetFieldDefinition(_handle).GetCustomAttributes(),
-                attributeNamespace, attributeName).IsNil;
+            return !MetadataReader.GetCustomAttributeHandle(CustomAttributes, attributeNamespace, attributeName).IsNil;


Could you revert changes to this file?

MichalStrehovsky · 2017-05-19T01:56:38Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/FrozenStringNode.cs

-            //
-            IEETypeNode stringSymbol = factory.ConstructedTypeSymbol(systemStringType);
-
-            if (stringSymbol.RepresentsIndirectionCell)


Could you revert these changes too?

MichalStrehovsky · 2017-05-19T01:58:55Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GCStaticsPreInitDataNode.cs

+// See the LICENSE file in the project root for more information.
+
+using System.Collections.Generic;
+using System.Linq;


Do you use Linq anywhere here? We generally avoid it in the compiler because it tends to bring generic virtual methods into the picture that nobody likes.

This is dead code. Removed

MichalStrehovsky · 2017-05-19T02:00:10Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/GCStaticsPreInitDataNode.cs

+            return nameMangler.NodeMangler.GCStatics(type) + "__PreInitData";
+        }
+
+        public virtual bool IsExported(NodeFactory factory) => false; 


If this always returns false, this shouldn't implement the IExportableSymbolNode interface.

ok. removed

MichalStrehovsky · 2017-05-19T02:03:10Z

src/ILCompiler.Compiler/src/Compiler/DependencyAnalysis/NodeFactory.cs

+        /// Get a direct reference to this EEType (for example, as required by GC).
+        /// If this will be compiled into a separate binary, it must be cloned into this one
+        /// </summary>
+        public IEETypeNode GetLocalTypeSymbol(TypeDesc type)


Could you revert the LocalTypeSymbol part?

MichalStrehovsky · 2017-05-19T02:05:53Z

src/Common/src/Internal/Runtime/ModuleHeaders.cs

+        /// <summary>
+        /// Contains pointer relocs to preinitialized data in FrozenObjectRegion
+        /// </summary>
+        GCStaticsPreInitDataRegion = 211,              


Is this used for anything? It seems like you allocate an ArrayOfEmbeddedObjects for this, but nothing gets actually emitted in the section.

Not used. Removed

MichalStrehovsky · 2017-05-19T02:07:04Z

src/ILCompiler.Compiler/src/Compiler/PreInitFieldInfo.cs

+{
+    public class PreInitFieldInfo
+    {
+        public FieldDesc Field { private set; get; }


Nit: you could remove the private set part to make the backing fields readonly.

that's better. changed to use readonly property.

MichalStrehovsky · 2017-05-19T02:07:39Z

src/ILCompiler/ILCompiler.sln

 Microsoft Visual Studio Solution File, Format Version 12.00
 # Visual Studio 15
-VisualStudioVersion = 15.0.26228.9
+VisualStudioVersion = 15.0.26430.6


Changes to this file seem unnecessary.

I see VS insisting on similar GUID changes whenever I flip configurations or do something similar. Submitted #3655

Thanks. I've reverted my change

MichalStrehovsky · 2017-05-19T02:09:21Z

src/ILCompiler/reproNative/reproNative.vcxproj

    <UseDebugLibraries>true</UseDebugLibraries>
-    <PlatformToolset>v141</PlatformToolset>
+    <!-- v140 is friendly to windbg, at the moment -->
+    <PlatformToolset>v140</PlatformToolset>


Changes like this should ideally be done in a separate pull request instead of sneaking them in a big change.

Is this because of the .pdbs have back pointers to .obj files with VS2017? It would be better to pass the linker option that avoids the back pointers to .obj files. @sandreenko Do you happen to know the option?

I would love to be able to always dogfood latest VS - I do not even have pre-VS2017 on most of my machines anymore.

I'll submit a separate PR for this after a bit more investigation (not my intention to put this in). Someone was suggesting fastlink becoming default as a possibility (not sure if it is the same as back pointers to .obj files) but I haven't got a chance to run more experiment yet.
BTW, this only means VS 2015 toolset is required but you can still use VS 2017. There is still a annoying dialog to ask you to upgrade, though.

…ng now

…ize of GCStaticEEType

… size (off by 1)

…ange

dnfclas added the cla-already-signed label May 9, 2017

MichalStrehovsky suggested changes May 9, 2017

View reviewed changes

MichalStrehovsky reviewed May 9, 2017

View reviewed changes

davidwrighton suggested changes May 10, 2017

View reviewed changes

fadimounir reviewed May 11, 2017

View reviewed changes

jkotas reviewed May 16, 2017

View reviewed changes

MichalStrehovsky reviewed May 16, 2017

View reviewed changes

MichalStrehovsky reviewed May 19, 2017

View reviewed changes

Yi Zhang and others added 12 commits May 19, 2017 00:50

initial set of changes

d6d8e36

FrozenArrayNode and hook up PreInitDataField in FieldDesc

341cbce

FrozenArrayNode

d80c986

Make it compile

9469f2a

PreInitDataNode support

5d296e3

Fix bugs

5a42023

use bitness trick to encode optional static initdata info. it's worki…

bdb2daf

…ng now

minor tweaks to code and add more comments

23960c3

delete unused program.il

f304569

Address feedback. There are still inconsistencies when we calculate s…

0d3aa9f

…ize of GCStaticEEType

Address more feedback and fix a bug in calculating GCStaticEETypeNode…

9e91205

… size (off by 1)

Address feedback and remove unused code after reviewing the entire ch…

816f7d7

…ange

yizhang82 force-pushed the preinit branch from d5e316c to 816f7d7 Compare May 21, 2017 03:17

yizhang82 merged commit 8eb9ffe into dotnet:master May 21, 2017

yizhang82 deleted the preinit branch May 27, 2017 01:09

		@@ -0,0 +1,218 @@

		// Microsoft (R) .NET Framework IL Disassembler. Version 4.6.1055.0


		public override bool StaticDependenciesAreComputed => true;

		public override ObjectNodeSection Section => ObjectNodeSection.DataSection;

Conversation

yizhang82 commented May 9, 2017

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

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!

davidwrighton left a comment

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!