Skip to content

Add ability to update the friend assembly set of an assembly with Reflection Emit - #47784

Merged
davidwrighton merged 5 commits into
dotnet:masterfrom
davidwrighton:dispatch-proxy-ref-emit-issue-repro
Feb 3, 2021
Merged

Add ability to update the friend assembly set of an assembly with Reflection Emit#47784
davidwrighton merged 5 commits into
dotnet:masterfrom
davidwrighton:dispatch-proxy-ref-emit-issue-repro

Conversation

@davidwrighton

Copy link
Copy Markdown
Member
  • Add routine/lock for updating friend assemblies
  • Unify all assembly handling custom attribute updates in the runtime
    • This adjusts the handling of the DebuggableAttribute, which in the past would have worked even if it was applied to anything via the CustomAttributeBuilder apis

Fixes the runtime level changes needed to address issues #43685 and #30917

krwq and others added 2 commits January 29, 2021 17:08
…lection Emit

- Add routine/lock for updating friend assemblies
- Unify all assembly handling custom attribute updates in the runtime
  - This adjusts the handling of the DebuggableAttribute, which in the past would have worked even if it was applied to anything via the CustomAttributeBuilder apis
Comment thread src/coreclr/vm/comdynamic.cpp Outdated
}
}

// Debuggable attribute processing

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy&paste mistake

Comment thread src/coreclr/vm/comdynamic.cpp Outdated

if (FAILED(hr))
{
// See if the metadata engine gave us any error information.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does the metadata engine ever set IErrorInfo?

(I know that this code was there even before your change.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll check. As you say, this code is old and has relics of a bygone era in it..

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nope. IErrorInfo isn't set by anything in metadata. I'll get rid of this stuff.

Comment thread src/coreclr/vm/assembly.hpp Outdated

void CacheFriendAssemblyInfo();
#ifndef DACCESS_COMPILE
ReleaseHolder<FriendAssemblyDescriptor> GetFriendAssemblyInfo(bool *pfNoFriendAssemblies);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are the copy constructors, etc. on ReleaseHolder setup correctly to make it safe to use as return value?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(We typically stay away from returning Holder types.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

At some point the ReleaseHolder type I'm using was updated to have the appropriate move constructor to make this work safely. I've verified that with the new test case, the type is freed at the appropriate moment, and not earler, or later. In some work I've done where I could use tools like unique_ptr, I have found the pattern of allowing such returns to be a nice upgrade to readability.

Comment thread src/coreclr/vm/assembly.cpp Outdated
{
pFriendAssemblies = NO_FRIEND_ASSEMBLIES_MARKER;
m_pFriendAssemblyDescriptor = pFriendAssemblies.Extract();
pFriendAssemblies.SuppressRelease();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Extract includes SuppressRelease. This should not be needed.

Comment thread src/coreclr/vm/comdynamic.cpp Outdated
}
}

void QCALLTYPE COMDynamicWrite::DefineCustomAttribute(QCall::ModuleHandle pModule, INT32 token, INT32 conTok, LPCBYTE pBlob, INT32 cbBlob, BOOL toDisk)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

toDisk argument is always false and can be deleted too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

GetOnDiskEmitter and related code can be deleted too

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point... all that code is gone now too.

Comment thread src/coreclr/vm/assembly.cpp Outdated
} // void Assembly::CacheFriendAssemblyInfo()
}

ReleaseHolder<FriendAssemblyDescriptor> Assembly::GetFriendAssemblyInfo(bool *pfNoFriendAssemblies)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

pfNoFriendAssemblies is not used in the method at all.

@davidwrighton
davidwrighton merged commit d779c59 into dotnet:master Feb 3, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 5, 2021
@davidwrighton
davidwrighton deleted the dispatch-proxy-ref-emit-issue-repro branch April 20, 2021 17:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants