Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More cleanups in Assembly/Loader/Binder area #58462

Merged
merged 11 commits into from
Sep 8, 2021
Merged

More cleanups in Assembly/Loader/Binder area #58462

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
01b0567
fixes some identation changes
VSadov Aug 31, 2021
2de7d84
Fold native AssemblyLoadContext into the base AssemblyBinder
VSadov Aug 31, 2021
fa55ed4
fix Linux build
VSadov Aug 31, 2021
5bc00c6
IsDefault
VSadov Aug 31, 2021
155eb7e
bunch of renames: TPABinderContext -> DefaultBinder, FallbackLoadCont…
VSadov Sep 1, 2021
3908648
rename: NativeAssemblyLoadContext -> NativeAssemblyBinder
VSadov Sep 1, 2021
b10f324
remove COM stuff from CoreBindResult
VSadov Sep 1, 2021
8897692
remove CoreBindResult entirely.
VSadov Sep 1, 2021
31e4ad6
rename pBinderContext, pPrivHostBinder, etc to be just pBinder
VSadov Sep 7, 2021
42518b2
fix a bug - there are two allocators here, rename made them into one.…
VSadov Sep 8, 2021
91cbe85
"override" is all-or-none
VSadov Sep 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/coreclr/vm/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,8 +396,6 @@ list(REMOVE_ITEM VM_SOURCES_WKS codeman.cpp)
set(VM_HEADERS_WKS
${VM_HEADERS_DAC_AND_WKS_COMMON}
../inc/jithelpers.h
coreclr/corebindresult.h
coreclr/corebindresult.inl
appdomainnative.hpp
assemblyname.hpp
assemblynative.hpp
Expand Down
14 changes: 5 additions & 9 deletions src/coreclr/vm/appdomain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3695,15 +3695,12 @@ PEAssembly * AppDomain::BindAssemblySpec(
{

{
// Use CoreClr's fusion alternative
CoreBindResult bindResult;
ReleaseHolder<BINDER_SPACE::Assembly> bindResult;
hrBindResult = pSpec->Bind(this, &bindResult);

pSpec->Bind(this, FALSE /* fThrowOnFileNotFound */, &bindResult);
hrBindResult = bindResult.GetHRBindResult();

if (bindResult.Found())
if (bindResult)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I would rename the local variable to something like boundAssembly - basically include the word "Assembly" in it. It feels weird to read the code below and use a "result" as an assembly. (Not that it was better before, it was even worse)

Copy link
Member Author

@VSadov VSadov Sep 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Right. I actually thought that boundAssembly could be a better name here.

I think I will make another pass over variables typed as AssemblyBinder*. We have a variety of [ load | context | binder ] combinations when naming these. Perhaps calling them binder or pBinder, in majority of cases at least, would make it more consistent.

{
if (SystemDomain::SystemFile() && bindResult.IsCoreLib())
if (SystemDomain::SystemFile() && bindResult->GetAssemblyName()->IsCoreLib())
{
// Avoid rebinding to another copy of CoreLib
result = SystemDomain::SystemFile();
Expand All @@ -3712,8 +3709,7 @@ PEAssembly * AppDomain::BindAssemblySpec(
else
{
// IsSystem on the PEFile should be false, even for CoreLib satellites
result = PEAssembly::Open(&bindResult,
FALSE);
result = PEAssembly::Open(bindResult, FALSE);
}

// Setup the reference to the binder, which performed the bind, into the AssemblySpec
Expand Down
5 changes: 2 additions & 3 deletions src/coreclr/vm/assemblyspec.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,10 +185,9 @@ class AssemblySpec : public BaseAssemblySpec
HRESULT EmitToken(IMetaDataAssemblyEmit *pEmit,
mdAssemblyRef *pToken);

VOID Bind(
HRESULT Bind(
AppDomain* pAppDomain,
BOOL fThrowOnFileNotFound,
CoreBindResult* pBindResult);
BINDER_SPACE::Assembly** ppAssembly);

Assembly *LoadAssembly(FileLoadLevel targetLevel,
BOOL fThrowOnFileNotFound = TRUE);
Expand Down
2 changes: 0 additions & 2 deletions src/coreclr/vm/assemblyspecbase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
#ifndef __ASSEMBLY_SPEC_BASE_H__
#define __ASSEMBLY_SPEC_BASE_H__

#include "coreclr/corebindresult.h"
#include "coreclr/corebindresult.inl"
#include "../binder/inc/assembly.hpp"

#include "baseassemblyspec.h"
Expand Down
22 changes: 5 additions & 17 deletions src/coreclr/vm/coreassemblyspec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,25 +68,20 @@ static VOID ThrowLoadError(AssemblySpec * pSpec, HRESULT hr)
EEFileLoadException::Throw(name, hr);
}

VOID AssemblySpec::Bind(AppDomain *pAppDomain,
BOOL fThrowOnFileNotFound,
CoreBindResult *pResult)
HRESULT AssemblySpec::Bind(AppDomain *pAppDomain, BINDER_SPACE::Assembly** ppAssembly)
{
CONTRACTL
{
INSTANCE_CHECK;
STANDARD_VM_CHECK;
PRECONDITION(CheckPointer(pResult));
PRECONDITION(CheckPointer(ppAssembly));
PRECONDITION(CheckPointer(pAppDomain));
PRECONDITION(IsCoreLib() == FALSE); // This should never be called for CoreLib (explicit loading)
}
CONTRACTL_END;

ReleaseHolder<BINDER_SPACE::Assembly> result;
HRESULT hr=S_OK;

pResult->Reset();

// Have a default binding context setup
AssemblyBinder *pBinder = GetBinderFromParentAssembly(pAppDomain);

Expand Down Expand Up @@ -125,20 +120,13 @@ VOID AssemblySpec::Bind(AppDomain *pAppDomain,
&pPrivAsm);
}

pResult->SetHRBindResult(hr);
if (SUCCEEDED(hr))
{
_ASSERTE(pPrivAsm != nullptr);

result = pPrivAsm.Extract();
_ASSERTE(result != nullptr);

pResult->Init(result);
}
else if (FAILED(hr) && (fThrowOnFileNotFound || (!Assembly::FileNotFound(hr))))
{
ThrowLoadError(this, hr);
*ppAssembly = pPrivAsm.Extract();
}

return hr;
}


Expand Down
40 changes: 0 additions & 40 deletions src/coreclr/vm/coreclr/corebindresult.h
View file
Open in desktop

This file was deleted.

86 changes: 0 additions & 86 deletions src/coreclr/vm/coreclr/corebindresult.inl
View file
Open in desktop

This file was deleted.

14 changes: 5 additions & 9 deletions src/coreclr/vm/pefile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -973,7 +973,7 @@ void PEAssembly::Attach()

#ifndef DACCESS_COMPILE
PEAssembly::PEAssembly(
CoreBindResult* pBindResultInfo,
BINDER_SPACE::Assembly* pBindResultInfo,
IMetaDataEmit* pEmit,
PEFile *creator,
BOOL system,
Expand Down Expand Up @@ -1038,7 +1038,8 @@ PEAssembly::PEAssembly(
{
// Cannot have both pHostAssembly and a coreclr based bind
_ASSERTE(pHostAssembly == nullptr);
pBindResultInfo->GetBindAssembly(&m_pHostAssembly);
pBindResultInfo = clr::SafeAddRef(pBindResultInfo);
m_pHostAssembly = pBindResultInfo;
}

#if _DEBUG
Expand Down Expand Up @@ -1124,18 +1125,13 @@ PEAssembly *PEAssembly::DoOpenSystem()
CONTRACT_END;

ETWOnStartup (FusionBinding_V1, FusionBindingEnd_V1);
CoreBindResult bindResult;
ReleaseHolder<BINDER_SPACE::Assembly> pPrivAsm;
IfFailThrow(BINDER_SPACE::AssemblyBinderCommon::BindToSystem(&pPrivAsm));
if(pPrivAsm != NULL)
{
bindResult.Init(pPrivAsm);
}

RETURN new PEAssembly(&bindResult, NULL, NULL, TRUE);
RETURN new PEAssembly(pPrivAsm, NULL, NULL, TRUE);
}

PEAssembly* PEAssembly::Open(CoreBindResult* pBindResult,
PEAssembly* PEAssembly::Open(BINDER_SPACE::Assembly* pBindResult,
BOOL isSystem)
{

Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/pefile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,7 @@ class PEAssembly : public PEFile
#endif

static PEAssembly *Open(
CoreBindResult* pBindResult,
BINDER_SPACE::Assembly* pBindResult,
BOOL isSystem);

static PEAssembly *Create(
Expand Down Expand Up @@ -605,7 +605,7 @@ class PEAssembly : public PEFile

#ifndef DACCESS_COMPILE
PEAssembly(
CoreBindResult* pBindResultInfo,
BINDER_SPACE::Assembly* pBindResultInfo,
IMetaDataEmit *pEmit,
PEFile *creator,
BOOL system,
Expand Down