More cleanups in assembly loader area #63157

VSadov · 2021-12-27T23:30:58Z

The goal is to simplify DomainFile/DomainAssembly and merge them into a single entity.
Coreclr does not have multimodule assemblies. With such assumption things can be simpler.

ghost · 2021-12-27T23:31:07Z

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

In progress.

The goal is to simplify DomainFile/DomainAssembly and merge them into a single entity.
Coreclr does not have multimodule assemblies. With such assumption things can be simpler.

Author:	VSadov
Assignees:	VSadov
Labels:	`area-AssemblyLoader-coreclr`
Milestone:	-

src/coreclr/vm/domainassembly.h

vitek-karas · 2022-01-03T14:52:29Z

src/coreclr/vm/ceeload.cpp

@@ -3284,7 +3264,7 @@ Module::GetAssemblyIfLoaded(
                }

                if (pDomainAssembly && pDomainAssembly->IsLoaded())
-                    pAssembly = pDomainAssembly->GetCurrentAssembly(); // <NOTE> Do not use GetAssembly - that may force the completion of a load
+                    pAssembly = pDomainAssembly->GetAssembly(); // <NOTE> Do not use GetAssembly - that may force the completion of a load


The comment now contradicts the code... ?

Right. I think the comment was not correct for quite a while. There were no sideeffects with GetCurrentAssembly either. Perhaps some time ago there was a distinction.

vitek-karas · 2022-01-03T19:45:30Z

I haven't looked through everything fully yet, but I'm wondering - can you point me to the place where we already have a check which fails to load assemblies with multiple modules?

I'm also wondering if we should somehow adapt managed reflection APIs accordingly - like obsolete some of the APIs which handle multi-module assemblies and such.
Also - what about Mono?

VSadov · 2022-01-03T19:52:54Z

LoadModule never returns an actual value -

runtime/src/coreclr/vm/ceeload.cpp

Line 3488 in daa2965

DomainFile *Module::LoadModule(AppDomain *pDomain, mdFile kFile)

All code paths throw some kind of exception. The most "normal" outcome is COR_E_MULTIMODULEASSEMBLIESDIALLOWED

vitek-karas · 2022-01-03T19:55:25Z

It's interesting that we go through the trouble of trying to load it - only to get a name to include in the exception. For another time 😄

VSadov · 2022-01-03T20:30:27Z

Adapting managed API would be an interesting task. The abstraction of a module kind of still exists. Assembly has identity and module contain implementation. It is just that we allow only one module per assembly now, which means assembly and module are always the same file.

In some reflection APIs like in dynamic codegen the layering is very clear - an assembly contains modules (only one now, I assume) and that contains code.
We will need to be careful with what scenarios would be deprecated, since from API perspective modules are not completely dead.

vitek-karas · 2022-01-03T20:45:35Z

Maybe there is nothing to do on the managed APIs (other than updating docs)... I was just thinking about all of the other things this may impact.

VSadov · 2022-01-03T20:56:20Z

BTW DomainAssembly here is not the best name IMO, but the least confusing for now. The thing roughly represents a "module for a given assembly when [being] loaded into a domain". We have another thing called Module, which also represents a loaded module.

It feels like the responsibilities of Assembly/ DomainAssembly/ Module could be rearranged a bit and maybe even fit into fewer classes and then renamed appropriately. If we do that, it would not be in this PR.

VSadov · 2022-01-11T03:00:45Z

I think the PR is at the logical point where it makes sense to review/merge.
@elinor-fung @vitek-karas - please take a look. Thanks!!

VSadov · 2022-01-21T22:25:29Z

Rebased onto recent main and fixed accumulated merge conflicts.

VSadov · 2022-01-24T18:12:28Z

I think the PR is at the logical point where it makes sense to review/merge.
@elinor-fung @vitek-karas - please take a look. Thanks!!

elinor-fung

where we already have a check which fails to load assemblies with multiple modules

Do we have tests for this? Haven't made it through everything yet, but if we don't have existing tests, it might be good to add something to verify the expected (failure) behaviour.

Maybe there is nothing to do on the managed APIs (other than updating docs)... I was just thinking about all of the other things this may impact.

+1 for updating docs. I think the APIs should behave the same before/after this PR (right?), but it is a good opportunity to think through what docs might need updating. Some possible non-documented cases that I believe are not supported and will error:

AssemblyBuilder.DefineDynamicModule: if a module is already defined (even if it is a different name)
Assembly.GetModules: if not reflection-only and has multiple modules
DllImportAttribute: using assembly display name as part of DLL name per remarks

src/coreclr/vm/methoditer.h

src/coreclr/vm/appdomain.hpp

src/coreclr/vm/assembly.hpp

src/coreclr/vm/appdomain.cpp

src/coreclr/vm/appdomain.hpp

src/coreclr/nativeaot/Runtime/eventtrace.cpp

src/coreclr/vm/appdomain.cpp

src/coreclr/debug/inc/dbgipcevents.h

src/coreclr/debug/daccess/dacimpl.h

elinor-fung · 2022-01-25T20:20:17Z

src/coreclr/debug/ee/debugger.h

+    // The runtime has the notion of "DomainAssembly", which is 1:1 with DebuggerModule
    // and thus 1:1 with CordbModule.  The CordbModule hash table on the RS now uses
-    // the DomainFile as the key instead of DebuggerModule.  This is a temporary
+    // the DomainAssembly as the key instead of DebuggerModule.  This is a temporary
    // workaround to facilitate the removal of DebuggerModule.
    // </TODO>


Does this TODO still make sense now that we don't have shared/multiple domains?
cc @dotnet/dotnet-diag

A TODO from 2002 is most certainly obsolete. I would not know what needs to be done here though. I just tried not to mention things that no longer exist.

I like the part that TODO is tagged by a date. :-)

DebuggerModule has had a few more things that started hanging off it. It'd be nice to clean but it hasn't happened. I think this crumble is a good hint of what to update in the case of such a transition.

src/coreclr/debug/ee/debugger.cpp

VSadov · 2022-01-27T02:41:16Z

+1 for updating docs. I think the APIs should behave the same before/after this PR (right?), but it is a good opportunity to think through what docs might need updating. Some possible non-documented cases that I believe are not supported and will error:

AssemblyBuilder.DefineDynamicModule: if a module is already defined (even if it is a different name)
Assembly.GetModules: if not reflection-only and has multiple modules
DllImportAttribute: using assembly display name as part of DLL name per remarks

I think this is better handled as a separate workitem just to separate it from mostly mechanical refactoring.

I think this change does not change any behavior. It is a halting problem to prove that, but from the nature of the changes the chances are relatively low.
That is not to say we do not have bugs or behavior is indeed as we think it is. It would definitely make sense to review scenarios related to multimodule assemblies, fix documentation, write some tests, fix bugs.

VSadov · 2022-01-27T02:50:50Z

I have added a workitem #64367 to track reviewing behaviors related to multimodule assemblies.

VSadov · 2022-01-29T00:59:45Z

Thanks!!!

ghost assigned VSadov Dec 27, 2021

dotnet-issue-labeler bot added the area-AssemblyLoader-coreclr label Dec 27, 2021

jkoritzinsky reviewed Dec 29, 2021

View reviewed changes

src/coreclr/vm/domainassembly.h Outdated Show resolved Hide resolved

VSadov force-pushed the aloader06 branch from dae598a to 1ae8f55 Compare December 30, 2021 16:12

vitek-karas reviewed Jan 3, 2022

View reviewed changes

VSadov force-pushed the aloader06 branch from 1ae8f55 to 71c7a32 Compare January 11, 2022 00:11

VSadov marked this pull request as ready for review January 11, 2022 02:58

VSadov requested a review from MichalStrehovsky as a code owner January 11, 2022 02:58

VSadov requested review from vitek-karas and elinor-fung January 11, 2022 02:58

VSadov added 12 commits January 21, 2022 14:20

GetManifestFile() --> GetPEAssembly()

7c3b570

GetManifestImport() -> GetMDImport()

1bfed85

removed GetCurrentAssembly(), GetLoadedAssembly()

b99316e

removed GetCurrentModule(), GetLoadedModule()

b907403

fix

0f1c732

removed DomainFile::ModuleIterator

82f2ffa

renamed Assembly::GetManifestModule() --> GetModule()

bc797ba

Removed Assembly::ModuleIterator

f7dae8d

move most fields off DomainAssembly

d02dc08

DomainAssembly is a complete proxy

0248e3a

some DomainFile --> DomainAssembly renames.

c793d4e

deleted DomainFile class

c5df253

VSadov added 10 commits January 21, 2022 14:20

renamed files

04b7e7e

a few minor cleanups

2365789

DomainAssembly does not need to have VPTR_. Couple other changes.

987aa5d

More DomainFile --> DomainAssembly renames

0c7926f

More DomainFile --> DomainAssembly renames

b359988

a few more renames

4359b0e

removed domainassembly.inl

91a5f93

domainassembly.h cleanup

9222df3

make gcc happy

8ea72ed

couple more changes

ca767c1

VSadov force-pushed the aloader06 branch from 71c7a32 to ca767c1 Compare January 21, 2022 22:24

fixed build after rebasing

47db770

elinor-fung reviewed Jan 25, 2022

View reviewed changes

PR feedback

be348b9

more PR feedback

0caa838

elinor-fung approved these changes Jan 28, 2022

View reviewed changes

VSadov merged commit d603356 into dotnet:main Jan 29, 2022

VSadov deleted the aloader06 branch January 29, 2022 01:00

tommcdon mentioned this pull request Jan 31, 2022

Fix debugger initialization #64523

Merged

ghost locked as resolved and limited conversation to collaborators Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More cleanups in assembly loader area #63157

More cleanups in assembly loader area #63157

VSadov commented Dec 27, 2021 •

edited

Loading

ghost commented Dec 27, 2021

vitek-karas Jan 3, 2022

VSadov Jan 3, 2022

vitek-karas commented Jan 3, 2022

VSadov commented Jan 3, 2022

vitek-karas commented Jan 3, 2022

VSadov commented Jan 3, 2022

vitek-karas commented Jan 3, 2022

VSadov commented Jan 3, 2022

VSadov commented Jan 11, 2022

VSadov commented Jan 21, 2022

VSadov commented Jan 24, 2022

elinor-fung left a comment

elinor-fung Jan 25, 2022

VSadov Jan 27, 2022

hoyosjs Jan 27, 2022

VSadov commented Jan 27, 2022

VSadov commented Jan 27, 2022 •

edited

Loading

VSadov commented Jan 29, 2022

More cleanups in assembly loader area #63157

More cleanups in assembly loader area #63157

Conversation

VSadov commented Dec 27, 2021 • edited Loading

ghost commented Dec 27, 2021

vitek-karas Jan 3, 2022

Choose a reason for hiding this comment

VSadov Jan 3, 2022

Choose a reason for hiding this comment

vitek-karas commented Jan 3, 2022

VSadov commented Jan 3, 2022

vitek-karas commented Jan 3, 2022

VSadov commented Jan 3, 2022

vitek-karas commented Jan 3, 2022

VSadov commented Jan 3, 2022

VSadov commented Jan 11, 2022

VSadov commented Jan 21, 2022

VSadov commented Jan 24, 2022

elinor-fung left a comment

Choose a reason for hiding this comment

elinor-fung Jan 25, 2022

Choose a reason for hiding this comment

VSadov Jan 27, 2022

Choose a reason for hiding this comment

hoyosjs Jan 27, 2022

Choose a reason for hiding this comment

VSadov commented Jan 27, 2022

VSadov commented Jan 27, 2022 • edited Loading

VSadov commented Jan 29, 2022

VSadov commented Dec 27, 2021 •

edited

Loading

VSadov commented Jan 27, 2022 •

edited

Loading