CoreCLR runtime fixes for composite R2R build with shared framework #34432
Conversation
In jithelpers, switch the query over to use a ReadyToRunInfo method to make it work in composite mode. Other than that these are mostly tiny fixes for the special case of composite image with just 1 assembly where we don't have the component assembly table and a few things must collapse. Thanks Tomas
src/coreclr/src/vm/nativeimage.cpp
Outdated
| @@ -53,7 +53,7 @@ void NativeImage::Initialize(READYTORUN_HEADER *pHeader, LoaderAllocator *pLoade | |||
|
|
|||
| m_pReadyToRunInfo = new ReadyToRunInfo(/*pModule*/ NULL, m_pImageLayout, pHeader, /*compositeImage*/ NULL, pamTracker); | |||
| m_pComponentAssemblies = m_pReadyToRunInfo->FindSection(ReadyToRunSectionType::ComponentAssemblies); | |||
| m_componentAssemblyCount = m_pComponentAssemblies->Size / sizeof(READYTORUN_COMPONENT_ASSEMBLIES_ENTRY); | |||
| m_componentAssemblyCount = (m_pComponentAssemblies != NULL ? m_pComponentAssemblies->Size / sizeof(READYTORUN_COMPONENT_ASSEMBLIES_ENTRY) : 1); | |||
There was a problem hiding this comment.
It does not sound right to set this to 1 when there is none.
There was a problem hiding this comment.
Test for null in the place where this is used instead?
There was a problem hiding this comment.
Well, I think that to some extent this flows from our discussion when I was reviewing the R2R format doc update. I was under the impression that you were suggesting we shouldn't be creating the component assembly table when there's just one component assembly in the composite image; that's what I ended up merging in as part of the format doc and implementing in the Crossgen2 compiler. This runtime change is just a counterpart stating that, when the component assembly table doesn't exist, there's just one assembly (represented by the master R2R header) in the native image. In hindsight I'm wondering whether this special case is truly worth the effort especially as the special casing is a typical bug farm.
There was a problem hiding this comment.
I have simplified the change by removing the special-casing of composite images with just 1 component assembly. This largely simplified the relevant code and fixed some issues I was hitting in local testing.
src/coreclr/src/vm/nativeimage.cpp
Outdated
| @@ -53,7 +53,7 @@ void NativeImage::Initialize(READYTORUN_HEADER *pHeader, LoaderAllocator *pLoade | |||
|
|
|||
| m_pReadyToRunInfo = new ReadyToRunInfo(/*pModule*/ NULL, m_pImageLayout, pHeader, /*compositeImage*/ NULL, pamTracker); | |||
| m_pComponentAssemblies = m_pReadyToRunInfo->FindSection(ReadyToRunSectionType::ComponentAssemblies); | |||
There was a problem hiding this comment.
Related:
We should either only have IMAGE_DATA_DIRECTORY *m_pComponentAssemblies; field and always fetch the count for it (ie do not cache it in the field);
or we should have both the pointer resolved in the field, like:
PTR_READYTORUN_COMPONENT_ASSEMBLIES_ENTRY m_pComponentAssemblies;
DWORD m_nComponentAssemblies;
| @@ -706,9 +711,9 @@ ReadyToRunInfo::ReadyToRunInfo(Module * pModule, PEImageLayout * pLayout, READYT | |||
| } | |||
|
|
|||
| // For format version 4.1 and later, there is an optional inlining table | |||
| if (IsImageVersionAtLeast(4, 1)) | |||
| if (pModule != NULL && IsImageVersionAtLeast(4, 1)) | |||
There was a problem hiding this comment.
We need comment on the m_pModule field about what situations can it be null.
Also, there is a ton of other places in this file where m_pModule is used. Are all of them safe? It feels like a bug farm to allow m_pModule to be null.
There was a problem hiding this comment.
I believe the case is relatively well isolated but I agree we should have stronger means to enforce this, I'll try to figure out some type hierarchy representing this. The case where m_pModule is null only happens for the "pseudo-ReadyToRunInfo" representing the composite image as such (as contrast to ReadyToRunInfo instances representing its component assemblies). It shouldn't leak anywhere into other runtime code and is only internally used by the "component-level" ReadyToRunInfo instances.
|
Merging in as the apparent "failures" seem to be predecessors of later identical jobs that succeeded with my latest commit and I doubt it's realistic to wait anymore for the remaining jobs that have been running for more than 12 hours. |
ghost
locked as resolved and limited conversation to collaborators
In jithelpers, switch the query over to use a ReadyToRunInfo method
to make it work in composite mode. Other than that these are mostly
tiny fixes for the special case of composite image with just 1
assembly where we don't have the component assembly table and a few
things must collapse.
Thanks
Tomas