Allow the CustomAttribute metadata table to be updated by ApplyChanges, with correct ordering #53066

davidwengier · 2021-05-21T07:04:01Z

There is a bug in Roslyn, dotnet/roslyn#52816, which results in Edit and Continue/Hot Reload duplicating attributes when emitting updates to a method/type/etc. Roslyn is currently adding rows to the CustomAttributes table with every delta, regardless of what was emitted in the original compilation.

To fix this I'm changing how we output the EncLog and EncMap tables, and from initial testing the runtime seems to be applying the updates correctly, updating existing CustomAttributes rows as necessary, and adding otherwise. Unfortunately the runtime expects the CustomAttributes table to be sorted by Parent, but does not currently maintain that sort order when applying updates.

From an investigation by @lambdageek:

For Mono:

We assume the CustomAttributes table is sorted on the Parent column (and do a binary search when looking for custom attributes). We assume the entries for a given parent are contiguous.
We could add a slow path and a separate lookaside table for any affected parents, but at the moment things will be broken.

For CoreCLR (please correct me if I’m wrong, this is just what I learned trying to do the Mono impl):

Some tables that are sorted by parent and assumed contiguous, have indirect Pointer variants. When updates are applied CoreCLR will actually convert from direct to indirect under some circumstances even if the indirect table wasn’t present in the baseline assembly in order to slot a new row for a parent in the right spot. (See CMiniMdRW::AddChildRowDirectForParent and its callers)

There is no indirect custom attributes table, however, as far as I know.

Before we can fix the Roslyn issue we probably need the runtime to be updated, otherwise it is unknown what things will break.

It's worth noting that this bug exists right now, and the CustomAttributes table will already be getting out of order with updates, but fixing the Roslyn bug would make it worse. Right now each update to the CustomAttributes table at least includes all of the attributes for the thing being updated in order and in a contiguous block. If a binary search stumbled across a block of rows it would at least behave consistently. Presumably this hasn't caused any real problems in the past because of the reflection caches, however those are now being cleared. Also in the past updates to attributes would be rude edits, but we're also changing that by adding a new runtime capability.

/cc @tmat @lambdageek @mikem8361 @tommcdon

ghost · 2021-05-21T07:04:04Z

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.

davidwengier · 2021-05-21T07:13:38Z

For an example of the issue:

Given an initial compile with the following (partial) metadata:

Method (0x06, 0x1C):
==============================================================================================================================================================================================================================
   Name           Signature        RVA         Parameters  GenericParameters  Attributes                                                                ImplAttributes  ImportAttributes  ImportName  ImportModule     
==============================================================================================================================================================================================================================
	<... snip ...>
2: 'F' (#31)      string () (#21)  0x00002054  nil         nil                0x00000091 (PrivateScope, Private, Static, HideBySig)                     0               0                 nil         nil (ModuleRef)  
	<... snip ...>

CustomAttribute (0x0c):
====================================================================================================================================================
   Parent                  Constructor             Value                                                                                            
====================================================================================================================================================
	<... snip ...>
4: 0x06000002 (MethodDef)  0x0a000004 (MemberRef)  01-00-00-00 (#56)

When method F() is modified (in any way that causes it to be emitted) we get the following:

CustomAttribute (0x0c):
=========================================================================
   Parent                  Constructor             Value                 
=========================================================================
1: 0x06000002 (MethodDef)  0x0a000004 (MemberRef)  01-00-00-00 (#56)    

EnC Log (0x1e):
===========================================
   Entity                        Operation  
===========================================
	<... snip ...>
9: 0x0c000005 (CustomAttribute)  0

That 0x0c000005 in the EnC Log tells the runtime to add a row 5 to the CustomAttribute table, meaning it ends up as:

CustomAttribute (0x0c):
====================================================================================================================================================
   Parent                  Constructor             Value                                                                                            
====================================================================================================================================================
	<... snip ...>
4: 0x06000002 (MethodDef)  0x0a000004 (MemberRef)  01-00-00-00 (#56)    
5: 0x06000002 (MethodDef)  0x0a000004 (MemberRef)  01-00-00-00 (#56)

If a type were to be updated before (or at the same time, since types are sorted first) the method then the CustomAttribute table emitted as a delta might look like this instead:

CustomAttribute (0x0c):
====================================================================================================================================================
   Parent                  Constructor             Value                                                                                            
====================================================================================================================================================
1: 0x02000003 (TypeDef)    0x0a000004 (MemberRef)  01-00-00-00 (#56)    
2: 0x06000002 (MethodDef)  0x0a000004 (MemberRef)  01-00-00-00 (#56)

Now when those rows are added to the table, it ends up out of order:

CustomAttribute (0x0c):
====================================================================================================================================================
   Parent                  Constructor             Value                                                                                            
====================================================================================================================================================
	<... snip ...>
4: 0x06000002 (MethodDef)  0x0a000004 (MemberRef)  01-00-00-00 (#56)    
5: 0x02000003 (TypeDef)    0x0a000004 (MemberRef)  01-00-00-00 (#56)
6: 0x06000002 (MethodDef)  0x0a000004 (MemberRef)  01-00-00-00 (#56)

ghost · 2021-05-21T15:43:50Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

There is a bug in Roslyn, dotnet/roslyn#52816, which results in Edit and Continue/Hot Reload duplicating attributes when emitting updates to a method/type/etc. Roslyn is currently adding rows to the CustomAttributes table with every delta, regardless of what was emitted in the original compilation.

To fix this I'm changing how we output the EncLog and EncMap tables, and from initial testing the runtime seems to be applying the updates correctly, updating existing CustomAttributes rows as necessary, and adding otherwise. Unfortunately the runtime expects the CustomAttributes table to be sorted by Parent, but does not currently maintain that sort order when applying updates.

From an investigation by @lambdageek:

For Mono:

We assume the CustomAttributes table is sorted on the Parent column (and do a binary search when looking for custom attributes). We assume the entries for a given parent are contiguous.
We could add a slow path and a separate lookaside table for any affected parents, but at the moment things will be broken.

For CoreCLR (please correct me if I’m wrong, this is just what I learned trying to do the Mono impl):

Some tables that are sorted by parent and assumed contiguous, have indirect Pointer variants. When updates are applied CoreCLR will actually convert from direct to indirect under some circumstances even if the indirect table wasn’t present in the baseline assembly in order to slot a new row for a parent in the right spot. (See CMiniMdRW::AddChildRowDirectForParent and its callers)

There is no indirect custom attributes table, however, as far as I know.

Before we can fix the Roslyn issue we probably need the runtime to be updated, otherwise it is unknown what things will break.

It's worth noting that this bug exists right now, and the CustomAttributes table will already be getting out of order with updates, but fixing the Roslyn bug would make it worse. Right now each update to the CustomAttributes table at least includes all of the attributes for the thing being updated in order and in a contiguous block. If a binary search stumbled across a block of rows it would at least behave consistently. Presumably this hasn't caused any real problems in the past because of the reflection caches, however those are now being cleared. Also in the past updates to attributes would be rude edits, but we're also changing that by adding a new runtime capability.

/cc @tmat @lambdageek @mikem8361 @tommcdon

Author:	davidwengier
Assignees:	-
Labels:	`area-Diagnostics-coreclr`, `untriaged`
Milestone:	-

mikem8361 · 2021-05-25T20:49:53Z

@davidwengier, this might be a dumb question, but is there anyway that Roslyn can order the CustomAttributes by Parent?

tmat · 2021-05-25T20:52:47Z

@mikem8361 I don't think we should do that as that would require emitting the entire CustomAttribute table on every change. The goal of EnC is to emit minimal delta, not reemit whole metadata tables.

davidwengier · 2021-05-25T21:16:35Z

is there anyway that Roslyn can order the CustomAttributes by Parent?

Roslyn is doing that, and within each delta the CustomAttributes table is ordered by parent. The issue is that after applying the updates (with info from EncLog) the resulting table ends up out of order.

As Tomas says the only way for Roslyn to deal with this would be for it to emit the entire CustomAttrbutes table and have the runtime simply forget any previous data it had. This would be potentially very large, and very expensive though.

mikem8361 · 2021-05-26T17:30:53Z

I asked for some help from @jkotas on this and he says:

I am not sure that I understand the problem.

The custom attributes table should be sorted on demand. Here is how I think it should work:

A new custom attribute entry gets added at the end of the custom attribute table with EnC update. The table is not sorted by Parent after this operation and that is expected. This operation should clear IsSorted(TBL_CustomAttribute) and IsTableVirtualSorted(TBL_CustomAttribute) flags.

When the runtime needs to enumerate the custom attributes for given item, it will find that the table is not sorted and it will create new sorted virtual index lazily. The subsequent operations are going to reuse the sorted index. Here is the callgraph:
MDInternalRW::EnumInit
* CONVERT_READ_TO_WRITE_LOCK() (first time only)
* m_MiniMd.GetCustomAttributeForToken-> LookUpTableByCol-> creates a new sorted virtual index (first time only)
* HENUMInternal::InitDynamicArrayEnum + HENUMInternal::AddElementToEnum (creates snapshot from the given virtual sorted index)

Where does this break?

tmat · 2021-05-26T17:32:50Z

What Jan is saying is what I'd expect to happen as well.

mikem8361 · 2021-05-26T17:34:03Z

So there isn't actual an issue here?

tmat · 2021-05-26T17:36:34Z

I don't know. Do we have a test that validates this works?

Also, Mono does not have this mechanism so that needs to be addressed in Mono.

mikem8361 · 2021-05-26T17:47:11Z

Neither the runtime or the diagnosticstest have any ENC tests. Aleksey add some infrastructure and few simple hot reload tests. Nothing that would exercise this.

tmat · 2021-05-26T18:06:54Z

Right, so the issue is that we need a test, otherwise we don't know if this works or not.

lambdageek · 2021-05-26T18:29:49Z

I don't know. Do we have a test that validates this works?

Also, Mono does not have this mechanism so that needs to be addressed in Mono.

Yep, that's tracked by #52993

mikem8361 · 2021-05-26T18:32:40Z

Before we can add tests for this PR dotnet/roslyn#53507 needs to get merged.

lambdageek · 2021-05-26T18:34:13Z

Right, so the issue is that we need a test, otherwise we don't know if this works or not.

for C#-level tests the following needs to happen:

Roslyn merges the changes (to the main-vs-deps branch) to support CustomAttribute updates. Gated by a new capability.
The roslyn changes propagate to https://github.com/dotnet/hotreload-utils (should happen daily).
We need to add the new capability to hotreload-utils (here and here)
hotreload-utils changes propagate to dotnet/runtime (also daily, I believe).
We write a new C# test-case in dotnet/runtime in src/library/System.Runtime.Loader/tests/
Right now the testcases aren't gated by runtime capabilities, so the tests will just start running. Disable Mono with a normal ActiveIssue attribute.
If the runtime behaves correctly, we would update GetApplyUpdateCapabilities() to return the new capability and light up the feature in the debugger and dotnet watch

davidwengier · 2021-05-27T00:26:56Z

That is excellent news if this is not a problem.

Roslyn merges the changes (to the main-vs-deps branch) to support CustomAttribute updates. Gated by a new capability.

This shouldn't be necessary to validate current behaviour. An example that would manifest the issue now:

Initial code:

[System.Obsolete]
class C
{
    [System.Obsolete]
    static void M() { }
}

Updated code (add a class and method with attributes):

[System.Obsolete]
class C
{
    [System.Obsolete]
    static void M() { }
}
[System.Obsolete]
class D
{
    [System.Obsolete]
    static void M() { }
}

This results in two custom attributes tables being emitted that look like this:

CustomAttribute (0x0c):
====================================================================================================================================================
   Parent                  Constructor             Value                                                                                            
====================================================================================================================================================
1: 0x06000001 (MethodDef)  0x0a000004 (MemberRef)  01-00-00-00 (#4e)                                                                                
2: 0x20000001 (Assembly)   0x0a000001 (MemberRef)  01-00-08-00-00-00-00-00 (#1d)                                                                    
3: 0x20000001 (Assembly)   0x0a000002 (MemberRef)  01-00-01-00-54-02-16-57-72-61-70-4E-6F-6E-45-78-63-65-70-74-69-6F-6E-54-68-72-6F-77-73-01 (#26)  
4: 0x20000001 (Assembly)   0x0a000003 (MemberRef)  01-00-07-01-00-00-00-00 (#45)                                                                    
5: 0x02000002 (TypeDef)    0x0a000004 (MemberRef)  01-00-00-00 (#4e)

and

CustomAttribute (0x0c):
=========================================================================
   Parent                  Constructor             Value                 
=========================================================================
1: 0x06000003 (MethodDef)  0x0a000006 (MemberRef)  01-00-00-00 (#66/12)  
2: 0x02000003 (TypeDef)    0x0a000006 (MemberRef)  01-00-00-00 (#66/12)

If those two tables are concatenated without being re-sorted then that should be possible to validate in a test, right?

lambdageek · 2021-05-27T21:14:57Z

@mikem8361 @davidwengier I added a draft PR with a new test like David suggested - should know how it goes pretty soon:
#53387

lambdageek · 2021-06-02T14:28:17Z

The test in #53387 passes on CoreCLR (and is now merged). So I don't think there's anything to be done for CoreCLR for this issue. (Adding Mono support is tracked in #52993)

tmat · 2021-06-02T19:52:24Z

Thanks!

mikem8361 · 2021-06-02T23:19:29Z

Can we close this issue now? Adding and changing custom attributes works, we have tests and a separate issue on "delete".

davidwengier · 2021-06-02T23:22:17Z

Sounds good to me. Thanks for verifying @lambdageek

ghost added the untriaged New issue has not been triaged by the area owner label May 21, 2021

jeffschwMSFT added the area-Diagnostics-coreclr label May 21, 2021

tommcdon assigned mikem8361 May 23, 2021

tommcdon removed the untriaged New issue has not been triaged by the area owner label May 23, 2021

tommcdon added this to the 6.0.0 milestone May 23, 2021

davidwengier mentioned this issue May 23, 2021

EnC - Update CustomAttributes table instead of just always adding to it dotnet/roslyn#53507

Merged

lambdageek mentioned this issue May 25, 2021

[metadata-update] Add support for deltas with CustomAttributes modifications #52993

Closed

lambdageek mentioned this issue May 28, 2021

Add hot reload test that adds custom attributes #53387

Merged

lambdageek mentioned this issue Jun 2, 2021

Skip the zeroed parent token (deleted) Custom Attribute entries in Reflection (and elsewhere) #53635

Open

davidwengier closed this as completed Jun 2, 2021

ghost locked as resolved and limited conversation to collaborators Jul 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow the CustomAttribute metadata table to be updated by ApplyChanges, with correct ordering #53066

Allow the CustomAttribute metadata table to be updated by ApplyChanges, with correct ordering #53066

davidwengier commented May 21, 2021

ghost commented May 21, 2021

davidwengier commented May 21, 2021 •

edited

Loading

ghost commented May 21, 2021

mikem8361 commented May 25, 2021

tmat commented May 25, 2021

davidwengier commented May 25, 2021

mikem8361 commented May 26, 2021

tmat commented May 26, 2021

mikem8361 commented May 26, 2021

tmat commented May 26, 2021 •

edited

Loading

mikem8361 commented May 26, 2021

tmat commented May 26, 2021

lambdageek commented May 26, 2021

mikem8361 commented May 26, 2021

lambdageek commented May 26, 2021 •

edited

Loading

davidwengier commented May 27, 2021 •

edited

Loading

lambdageek commented May 27, 2021

lambdageek commented Jun 2, 2021

tmat commented Jun 2, 2021

mikem8361 commented Jun 2, 2021

davidwengier commented Jun 2, 2021

Allow the CustomAttribute metadata table to be updated by ApplyChanges, with correct ordering #53066

Allow the CustomAttribute metadata table to be updated by ApplyChanges, with correct ordering #53066

Comments

davidwengier commented May 21, 2021

ghost commented May 21, 2021

davidwengier commented May 21, 2021 • edited Loading

ghost commented May 21, 2021

mikem8361 commented May 25, 2021

tmat commented May 25, 2021

davidwengier commented May 25, 2021

mikem8361 commented May 26, 2021

tmat commented May 26, 2021

mikem8361 commented May 26, 2021

tmat commented May 26, 2021 • edited Loading

mikem8361 commented May 26, 2021

tmat commented May 26, 2021

lambdageek commented May 26, 2021

mikem8361 commented May 26, 2021

lambdageek commented May 26, 2021 • edited Loading

davidwengier commented May 27, 2021 • edited Loading

lambdageek commented May 27, 2021

lambdageek commented Jun 2, 2021

tmat commented Jun 2, 2021

mikem8361 commented Jun 2, 2021

davidwengier commented Jun 2, 2021

davidwengier commented May 21, 2021 •

edited

Loading

tmat commented May 26, 2021 •

edited

Loading

lambdageek commented May 26, 2021 •

edited

Loading

davidwengier commented May 27, 2021 •

edited

Loading