[EnC] CustomAttribute rows in delta seem out of order

[lambdageek]

Version Used: using https://github.com/dotnet/hotreload-utils 1.0.2-alpha.0.22157.2 which uses roslyn 4.2.0-3.22156.4

Steps to Reproduce:

Compile the following baseline and apply changes below, then look at the metadata delta with mdv

Baseline:

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
using System;


namespace System.Reflection.Metadata.ApplyUpdate.Test
{
    public class AddNestedClass
    {
        public AddNestedClass()
        {
        }

        public string TestMethod()
        {
            return "123";
        }

    }
}

Modification:

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
using System;


namespace System.Reflection.Metadata.ApplyUpdate.Test
{
    public class AddNestedClass
    {
        public AddNestedClass()
        {
        }

        public string TestMethod()
        {
            var n = new Nested();
	    n.Evt += new Action<string> (n.DefaultHandler);
            return n.buf;
        }

        private class Nested {
            public Nested() { }
	    public event Action<string> Evt;

	    public void RaiseEvt () {
		Evt ("789");
	    }

	    public string buf;

	    public void DefaultHandler (string s) {
		this.buf = s;
	    }
        }
    }
}

Compile with nullability enabled.

The mdv output is attached:
add-event.mdv.txt

Expected Behavior:

the custom attribute with parent 0x06000007 in both the baseline and Generation 1 will be at row 0x1a

Actual Behavior:

The EnCMap says:

1c: 0x0c00001a (CustomAttribute)  0    0x00001a  update  

So row 0x1a of the Custom Attributes table is updated by row 1 from Generation 1:

1: 0x04000004 (Field)      0x0a000012 (MemberRef)  01-00-00-00 (#356/7e)              

which has a completely different parent and .ctor

The actual update to Parent 0x06000007 is in row 3 of the custom attribute table:

3: 0x06000007 (MethodDef)  0x06000004 (MethodDef)  01-00-01-00-00 (#350/78)           

Note that the EnCLog is in a kind of funny order (field attributes before method attributes?)

2f: 0x0c00001b (CustomAttribute)  0                          
30: 0x0c00001c (CustomAttribute)  0                          
31: 0x0c00001a (CustomAttribute)  0                          
32: 0x0c00001d (CustomAttribute)  0                          
33: 0x0c00001e (CustomAttribute)  0                          

---

Why it's a problem:

I think CoreCLR will accept this as is, but for Mono we expect row updates (like Gen 1 row 1) to have the same Parent as the previous generation, or to set the Parent to 0 (for a deletion). Updates (as opposed to row additions) shouldn't invalidate our assumption that all custom attributes for a given Parent are contiguous.

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[lambdageek]

/cc @tmat @davidwengier

[lambdageek]

In case it's not clear, I think the custom attribute rows in Gen 1 should be like this:

1: 0x06000007 (MethodDef)  0x06000004 (MethodDef)  01-00-01-00-00 (#350/78)           
2: 0x04000004 (Field)      0x0a000012 (MemberRef)  01-00-00-00 (#356/7e)              
3: 0x04000004 (Field)      0x0a000013 (MemberRef)  01-00-00-00-00-00-00-00 (#35b/83)  
4: 0x06000009 (MethodDef)  0x0a000012 (MemberRef)  01-00-00-00 (#356/7e)              
5: 0x0600000a (MethodDef)  0x0a000012 (MemberRef)  01-00-00-00 (#356/7e)              

with EnCLog and EnCMap in the current order.

So row 1 is an update to row 0x1a of Gen0 and rows 2-5 define the added rows 0x1b-0x1e.

[davidwengier]

our assumption that all custom attributes for a given Parent are contiguous.

I can see how this could be thought to be wrong, but even if we changed the order of the CA table in Gen 1 to what you suggested, what would happen if you add another attribute to TestMethod (0x06000007) in Gen 2? Or if there where two methods in the original, each with one attribute? Or an attribute on the class, and then you added another attribute in Gen 1? I don't think its possible to emit deltas that can be applied without re-sorting the table, and keep all parents in the CA table contiguous. We would have to leave huge gaps in the CA table that we fill in later, or something.

Additionally the custom attribute table in Gen 1 still has to adhere to the rules of the custom attribute table, namely that it's sorted by parent, so your suggested layout violates the spec AFAIK.

[davidwengier]

TL;DR isn't this an accidentally contrived example, because you only have one method and it just happens to be at the end of the custom attribute table in gen 0?

[lambdageek]

it's sorted by parent, so your suggested layout violates the spec AFAIK.

Thinking about this a little more, I don't think there's any layout that won't violate some aspect of the spec - as long as CA additions are allowed, we either lose contiguity or the sorting property or both. So I think once there are any CA table edits (either additions or modifications) the runtime has to fall back to a linear scan of the whole table in order to accurately discover all the attributes.

So I think roslyn behavior is probably okay here, however I was surprised that CA table modifications can change the parent of a row from one generation to the next. But it's probably just something I'll have to deal with in the runtime.